Last Comment Bug 760013 - Split testSuggestHistoryBookmarks in 2 tests
: Split testSuggestHistoryBookmarks in 2 tests
Status: RESOLVED FIXED
[qa-]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Remus Pop (:RemusPop)
:
Mentors:
Depends on:
Blocks: 742550
  Show dependency treegraph
 
Reported: 2012-05-31 01:51 PDT by Remus Pop (:RemusPop)
Modified: 2012-08-21 01:37 PDT (History)
4 users (show)
hskupin: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed
wontfix


Attachments
patch v1(default) (13.08 KB, patch)
2012-05-31 04:43 PDT, Remus Pop (:RemusPop)
hskupin: review-
Details | Diff | Review
patch v1(aurora, beta, release) (12.56 KB, patch)
2012-05-31 04:49 PDT, Remus Pop (:RemusPop)
no flags Details | Diff | Review
Patch v2 (default) [checked-in] (12.98 KB, patch)
2012-05-31 07:17 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review
patch v2 (aurora-beta-release) [checked-in] (12.49 KB, patch)
2012-06-01 06:37 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review
patch v2 (esr10) [checked-in] (14.16 KB, patch)
2012-06-01 07:00 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review

Description Remus Pop (:RemusPop) 2012-05-31 01:51:42 PDT
testSuggestHistoryBookmarks contains 2 test functions so we'll need to split it into 2 files.
Comment 1 Remus Pop (:RemusPop) 2012-05-31 02:07:15 PDT
This will block bug 742550. So first we will split the tests and then do the fixes in bug 742550.
Comment 2 Remus Pop (:RemusPop) 2012-05-31 04:43:11 PDT
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.
Comment 3 Remus Pop (:RemusPop) 2012-05-31 04:49:25 PDT
Created attachment 628680 [details] [diff] [review]
patch v1(aurora, beta, release)

Manifest file has been removed so it applies on specified branches.
Comment 4 Henrik Skupin (:whimboo) 2012-05-31 06:43:21 PDT
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.
Comment 5 Henrik Skupin (:whimboo) 2012-05-31 06:44:09 PDT
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.
Comment 6 Remus Pop (:RemusPop) 2012-05-31 07:16:32 PDT
(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.
Comment 7 Remus Pop (:RemusPop) 2012-05-31 07:17:57 PDT
Created attachment 628720 [details] [diff] [review]
Patch v2 (default) [checked-in]

Addressed all requests.
Comment 8 Henrik Skupin (:whimboo) 2012-05-31 07:30:30 PDT
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.
Comment 9 Henrik Skupin (:whimboo) 2012-06-01 05:52:43 PDT
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.
Comment 10 Remus Pop (:RemusPop) 2012-06-01 06:37:15 PDT
Created attachment 629169 [details] [diff] [review]
patch v2 (aurora-beta-release) [checked-in]

Applies cleanly on all branches, but not esr10.
Comment 11 Henrik Skupin (:whimboo) 2012-06-01 06:51:01 PDT
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)
Comment 12 Henrik Skupin (:whimboo) 2012-06-01 06:53:08 PDT
Please check why it doesn't cleanly apply on esr10, and come up with another patch for that branch.
Comment 13 Remus Pop (:RemusPop) 2012-06-01 07:00:59 PDT
Created attachment 629174 [details] [diff] [review]
patch v2 (esr10) [checked-in]

Updated to work on Firefox ESR10.
Comment 14 Henrik Skupin (:whimboo) 2012-06-01 07:06:59 PDT
Landed on esr:
http://hg.mozilla.org/qa/mozmill-tests/rev/8d8247beaa6d
Comment 15 Henrik Skupin (:whimboo) 2012-06-01 07:07:36 PDT
Remus, please also update the appropriate litmus tests.
Comment 16 Remus Pop (:RemusPop) 2012-06-05 05:08:04 PDT
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
Comment 17 Vlad [QA] 2012-06-05 07:55:10 PDT
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.
Comment 18 Henrik Skupin (:whimboo) 2012-06-07 23:03:44 PDT
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 Vlad [QA] 2012-06-08 01:15:14 PDT
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
Comment 20 Henrik Skupin (:whimboo) 2012-06-08 01:33:30 PDT
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.
Comment 21 Henrik Skupin (:whimboo) 2012-06-27 05:48:51 PDT
Vlad, we are still waiting for a response on updating the litmus tests. Can you please give us a reply?
Comment 22 Henrik Skupin (:whimboo) 2012-08-15 02:25:49 PDT
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 Vlad [QA] 2012-08-20 01:30:31 PDT
Hi Henrik. 
I've modify the tests in litmus. Please take a look and tell me what you think.
Thanks
Comment 24 Henrik Skupin (:whimboo) 2012-08-20 13:28:30 PDT
(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 Vlad [QA] 2012-08-20 22:42:34 PDT
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.
Comment 26 Henrik Skupin (:whimboo) 2012-08-21 01:37:50 PDT
Looks fine. Thanks Vlad.

Note You need to log in before you can comment on or make changes to this bug.