Split testSuggestHistoryBookmarks in 2 tests

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: RemusPop, Assigned: RemusPop)

Tracking

unspecified
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox12 fixed, firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox-esr10 fixed, status1.9.2 wontfix)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
testSuggestHistoryBookmarks contains 2 test functions so we'll need to split it into 2 files.
(Assignee)

Comment 1

5 years ago
This will block bug 742550. So first we will split the tests and then do the fixes in bug 742550.
Blocks: 742550
(Assignee)

Comment 2

5 years ago
Created attachment 628678 [details] [diff] [review]
patch v1(default)

Splitted into 2 tests and updated the manifest. testSuggestBookmarks has added content from testSuggestHistory so it meets the requirements.
Attachment #628678 - Flags: review?(hskupin)
(Assignee)

Comment 3

5 years ago
Created attachment 628680 [details] [diff] [review]
patch v1(aurora, beta, release)

Manifest file has been removed so it applies on specified branches.
Attachment #628680 - Flags: review?(hskupin)
(Assignee)

Updated

5 years ago
Summary: Split testSuggestHistoryBookmarks to in 2 tests → Split testSuggestHistoryBookmarks in 2 tests
Comment on attachment 628678 [details] [diff] [review]
patch v1(default)

+++ b/tests/functional/testAwesomeBar/testSuggestBookmarks.js

>+const TIMEOUT = 5000;

Please get rid of this unused constant.

>+  // We must open the blank page so the autocomplete result isn't "Swith to tab"

nit: 'doesn't show "Switch to tab"'

>+++ b/tests/functional/testAwesomeBar/testSuggestHistory.js

>+/**
>+ * Check history and bookmarked (done in testStarInAutocomplete()) items appear in autocomplete list.
>+ */
>+var testSuggestHistoryAndBookmarks = function() {

We do not test bookmarks here.

>+  // Wait for 4 seconds to work around Firefox LAZY ADD of items to the DB
>+  controller.sleep(4000);

Don't we need this for the other test? I would think so.
 
>+  // Define the path to the first auto-complete result
>+  var richlistItem = locationBar.autoCompleteResults.getResult(0);
>+
>+  // Get the visible results from the autocomplete list. Verify it is 1
>+  controller.waitFor(function () {
>+    return locationBar.autoCompleteResults.isOpened;
>+  }, "Autocomplete list has been opened");

When you change the order of those lines of code in the other test, please do the same here. I would like to have it consistent. If that's part of the fix for the failure then revert those lines in the other file.
Attachment #628678 - Flags: review?(hskupin) → review-
Comment on attachment 628680 [details] [diff] [review]
patch v1(aurora, beta, release)

As mentioned before, please wait with backport patches until the one for default has been fully reviewed. Marking as obsolete.
Attachment #628680 - Attachment is obsolete: true
Attachment #628680 - Flags: review?(hskupin)
(Assignee)

Comment 6

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 628678 [details] [diff] [review]
> >+  // Wait for 4 seconds to work around Firefox LAZY ADD of items to the DB
> >+  controller.sleep(4000);
> 
> Don't we need this for the other test? I would think so.

It works for me for the other test.
 
> >+  // Define the path to the first auto-complete result
> >+  var richlistItem = locationBar.autoCompleteResults.getResult(0);
> >+
> >+  // Get the visible results from the autocomplete list. Verify it is 1
> >+  controller.waitFor(function () {
> >+    return locationBar.autoCompleteResults.isOpened;
> >+  }, "Autocomplete list has been opened");
> 
> When you change the order of those lines of code in the other test, please
> do the same here. I would like to have it consistent. If that's part of the
> fix for the failure then revert those lines in the other file.

If I wouldn't change the order in testSuggestBookmarks, it would gracefully fail. It is a part of the fix in testSuggestHistory, but to have consistency I will change it in testSuggestBookmarks.
(Assignee)

Comment 7

5 years ago
Created attachment 628720 [details] [diff] [review]
Patch v2 (default) [checked-in]

Addressed all requests.
Attachment #628678 - Attachment is obsolete: true
Attachment #628720 - Flags: review?(hskupin)
Attachment #628720 - Flags: review?(hskupin) → review+
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/d4391abf9f5f

If no new regressions have been introduced please backport the patch to older branches tomorrow.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status1.9.2: --- → wontfix
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → fixed
Resolution: --- → FIXED
Comment on attachment 628720 [details] [diff] [review]
Patch v2 (default) [checked-in]

Remus, please come up with a patch for older branches so that we can backport the split. Thanks.
Attachment #628720 - Attachment description: Patch v2 (default) → Patch v2 (default) [checked-in]
(Assignee)

Comment 10

5 years ago
Created attachment 629169 [details] [diff] [review]
patch v2 (aurora-beta-release) [checked-in]

Applies cleanly on all branches, but not esr10.
Attachment #629169 - Flags: review?(hskupin)
Comment on attachment 629169 [details] [diff] [review]
patch v2 (aurora-beta-release) [checked-in]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/61a2c4e41512 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f182be061b3a (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/bb632d9e9e60 (release)
Attachment #629169 - Attachment description: patch v2 (all branches) → patch v2 (all branches) [checked-in]
Attachment #629169 - Flags: review?(hskupin) → review+
Please check why it doesn't cleanly apply on esr10, and come up with another patch for that branch.
status-firefox12: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
(Assignee)

Comment 13

5 years ago
Created attachment 629174 [details] [diff] [review]
patch v2 (esr10) [checked-in]

Updated to work on Firefox ESR10.
Attachment #629174 - Flags: review?(hskupin)
Attachment #629169 - Attachment description: patch v2 (all branches) [checked-in] → patch v2 (aurora-beta-release) [checked-in]
Attachment #629174 - Flags: review?(hskupin) → review+
Landed on esr:
http://hg.mozilla.org/qa/mozmill-tests/rev/8d8247beaa6d
status-firefox-esr10: affected → fixed
Remus, please also update the appropriate litmus tests.
Flags: in-litmus?(remus.pop)
(Assignee)

Updated

5 years ago
Attachment #629174 - Attachment description: patch v2 (esr10) → patch v2 (esr10) [checked-in]
(Assignee)

Comment 16

5 years ago
Cc-ing Vlad G as he is the owner of AwesomeBar tests.
Vlad, we need the litmus test splitted in two for all branches, including esr10.
For reference purposes here is the link to the test in esr10:
https://litmus.mozilla.org/show_test.cgi?id=41811
status-firefox-esr10: fixed → affected
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → fixed
Flags: in-litmus?(remus.pop) → in-litmus?(vlad.ghetiu)

Comment 17

5 years ago
Hi Henrik.
I've split the test in to two separate tests and you can see them here:
https://litmus.mozilla.org/show_test.cgi?id=41811
https://litmus.mozilla.org/show_test.cgi?id=63749

Please tell me I have to make more modifications. 
I've edited the original test and added a new one, that is why the numbers aren't consecutive.
Vlad, thanks for doing that. Here some notes:

* I don't think we should continue to mention a fresh profile as default action. Instead the tester should use the 'Clear Recent History' dialog and remove all browsing and history data. We should mention the data loss and offer the new profile as option.

* Both tests have a spelling error: 'highlated'

* Those tests will also have to land for the Firefox 13, 14, and Aurora branch on Litmus. I don't think we should care about Firefox 12.

Comment 19

5 years ago
I've modify a little bit the tests and also I've cloned them on Firefox 13, 14 ,Aurora branch.

The testcase ID are: 66525,66526 on Firefox 13
                     66527,66528 on Firefox 14
                     66529,66530 on Aurora
Tests are not synchronous and have different content in how to reset the bookmarks and history. I think it is not enough to say 'Start Firefox 'Clear Recent History' from Menu - Tools.' only but also to specify what has to be done there. Please contact me on IRC so we can sort this out.
Vlad, we are still waiting for a response on updating the litmus tests. Can you please give us a reply?
Whiteboard: [qa-]
Vlad, can you please make sure the Litmus test is updated? Otherwise this request will be obsolete soon when we fully switch over to moztrap.

Comment 23

5 years ago
Hi Henrik. 
I've modify the tests in litmus. Please take a look and tell me what you think.
Thanks
(In reply to Vlad [QA] from comment #23)
> I've modify the tests in litmus. Please take a look and tell me what you
> think.

Which tests are those? It would be very helpful if you could point out links to all of them. Thanks.

Comment 25

5 years ago
These are the split testcases:
https://litmus.mozilla.org/show_test.cgi?id=41811
https://litmus.mozilla.org/show_test.cgi?id=63749

(In reply to Henrik Skupin (:whimboo) from comment #24)
> (In reply to Vlad [QA] from comment #23)
> > I've modify the tests in litmus. Please take a look and tell me what you
> > think.
> 
> Which tests are those? It would be very helpful if you could point out links
> to all of them. Thanks.
Looks fine. Thanks Vlad.
Flags: in-litmus?(vlad.ghetiu) → in-litmus+
You need to log in before you can comment on or make changes to this bug.