Last Comment Bug 787924 - Update tests in /testAwesomeBar to wait for the string in location bar to be fully typed
: Update tests in /testAwesomeBar to wait for the string in location bar to be ...
Status: RESOLVED FIXED
s=q3 u=failure c=awesomebar p=1
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: ---
Assigned To: Andreea Matei [:AndreeaMatei]
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on: 708582 784644
Blocks: 786183
  Show dependency treegraph
 
Reported: 2012-09-03 06:13 PDT by Andreea Matei [:AndreeaMatei]
Modified: 2012-09-25 00:28 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed


Attachments
patch v1 (11.12 KB, patch)
2012-09-04 08:20 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v2 (16.20 KB, patch)
2012-09-05 02:18 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v3 (16.80 KB, patch)
2012-09-07 08:08 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v3.1 (16.07 KB, patch)
2012-09-12 07:35 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
testcase (1.30 KB, patch)
2012-09-13 02:27 PDT, Andreea Matei [:AndreeaMatei]
no flags Details | Diff | Splinter Review
patch v4 (16.01 KB, patch)
2012-09-13 08:21 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.1 (16.00 KB, patch)
2012-09-14 08:11 PDT, Andreea Matei [:AndreeaMatei]
no flags Details | Diff | Splinter Review
patch v4.1 (16.12 KB, patch)
2012-09-14 09:29 PDT, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Splinter Review
patch v4.2 (16.09 KB, patch)
2012-09-14 15:47 PDT, Andreea Matei [:AndreeaMatei]
no flags Details | Diff | Splinter Review
patch v4.3 (13.16 KB, patch)
2012-09-18 01:17 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v4.4 (13.35 KB, patch)
2012-09-18 03:18 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v4.5 (13.33 KB, patch)
2012-09-18 03:35 PDT, Andreea Matei [:AndreeaMatei]
hskupin: review+
Details | Diff | Splinter Review

Description Andreea Matei [:AndreeaMatei] 2012-09-03 06:13:48 PDT
Given that we have more failures like 
"Underlined title matches entered title - got 'communit', expected 'community'"
all over the AwesomeBar tests, we need to add a waitFor on those which use locationBar.type to ensure the whole string is typed and matches the expected one.

Similar failure: 784644, 786183.
Comment 1 Andreea Matei [:AndreeaMatei] 2012-09-04 08:20:28 PDT
Created attachment 658099 [details] [diff] [review]
patch v1

I have added an waitFor after we type into the location bar, in which we check the location value matches the typed string.

As mentioned in comment 15, bug 784644, changed for each with forEach and updated doneButton.

The failure like "'grant' should equal 'grants'" was also affecting the callback we have in testCheckItemHighlight.js at the controller.assert line. After talking with Henrik on IRC, we agreed it's important to be handled right away, so I've replaced it with an waitFor.
Comment 2 Henrik Skupin (:whimboo) 2012-09-05 00:48:14 PDT
Comment on attachment 658099 [details] [diff] [review]
patch v1

Review of attachment 658099 [details] [diff] [review]:
-----------------------------------------------------------------

There are a couple of whitespace issues in this patch. Beside that ensure that we replace the for each (letter) in all of those tests. At least in visibleItemsMax I can still see it.
Comment 3 Andreea Matei [:AndreeaMatei] 2012-09-05 02:18:18 PDT
Created attachment 658420 [details] [diff] [review]
patch v2

Removed all whitespaces and added locationBar.type + waitFor in testVisibleItemsMax.js instead of for each letter.
Comment 4 Henrik Skupin (:whimboo) 2012-09-05 09:52:42 PDT
Comment on attachment 658420 [details] [diff] [review]
patch v2

Review of attachment 658420 [details] [diff] [review]:
-----------------------------------------------------------------

What I wonder is if the patch has been passed the internal review/feedback round. There are a way too many things I have to claim about on this patch. Please always do internal checks before uploading patches even for follow-up ones.

::: tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +15,4 @@
>    LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html',
>    'about:blank'
>  ];
>                            

nit: Here is a whitespace issue also one more later in this file.

@@ +60,5 @@
>    // Check that the first item in the drop down list is selected
>    controller.waitFor(function () {
>      return locationBar.autoCompleteResults.selectedIndex === 0;
> +  }, "The first item should be selected - expected 0 - got " +
> +    locationBar.autoCompleteResults.selectedIndex);

We can't use got here. Please also update it so we do not show wrong assumptions.

::: tests/functional/testAwesomeBar/testCheckItemHighlight.js
@@ +93,1 @@
>       underlined.length + "', expected '1'");

We can't use the got part here because it's evaluated before we enter waitFor().

::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +49,5 @@
>  
> +  // Wait for the location bar to contain the entire test string
> +  controller.waitFor(function () {
> +    return locationBar.value === TEST_STRING;
> +  }, "Location bar contains the typed data");

You want to show at least what value we expect.

::: tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +55,5 @@
>  
> +  // Wait for the location bar to contain the entire test string
> +  controller.waitFor(function () {
> +    return locationBar.value === LOCAL_TEST_PAGE.string;
> +  }, "Location bar contains the typed data");

Please show at least which value we expect.

@@ +60,5 @@
>  
>    // Ensure the autocomplete list is open
>    controller.waitFor(function () {
>      return locationBar.autoCompleteResults.isOpened;
>    }, "Autocomplete list has been opened");

Shouldn't we better wait for 1 auto-complete result shown?

::: tests/functional/testAwesomeBar/testGoButton.js
@@ +37,5 @@
>    locationBar.focus({type: "shortcut"});
>    locationBar.type(LOCAL_TEST_PAGES[1]);
> +  controller.waitFor(function () {
> +    return locationBar.value === LOCAL_TEST_PAGES[1];
> +  }, "Location bar contains the typed data");

Please show which value we expect.

::: tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +51,5 @@
>  
> +  // Wait for the location bar to contain the entire test string
> +  controller.waitFor(function () {
> +    return locationBar.value === LOCAL_TEST_PAGE.string;
> +  }, "Location bar contains the typed data");

Same here as for the other files. Show what we expect.

@@ +59,5 @@
>      return locationBar.autoCompleteResults.isOpened;
>    }, "Autocomplete list has been opened");
>  
>    expect.equal(locationBar.autoCompleteResults.visibleResults.length, 1,
>                 "There is one visible result in the autocomplete list");

Shouldn't we better wait for this instead of isOpened?

::: tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +59,5 @@
> +  // Type the test string into the location bar
> +  locationBar.type(testString);
> +  controller.waitFor(function () {
> +    return locationBar.value === testString;
> +  }, "Location bar contains the typed data");

Please show the expected value.
Comment 5 Andreea Matei [:AndreeaMatei] 2012-09-07 08:08:59 PDT
Created attachment 659262 [details] [diff] [review]
patch v3

Addressed all requests, had been passed to internal reviews.
Also changed to Aptana editor and now I see highlighted all the whitespaces.
I have to mention that some tests needed an empty line at the very end, as stated in Mozmill style guide. It's not a whitespace there.
Comment 6 Henrik Skupin (:whimboo) 2012-09-07 15:23:57 PDT
For the future please do not forget to mark a bug as assigned once you agreed to work on it.
Comment 7 Henrik Skupin (:whimboo) 2012-09-08 15:03:11 PDT
Comment on attachment 659262 [details] [diff] [review]
patch v3

Review of attachment 659262 [details] [diff] [review]:
-----------------------------------------------------------------

In general looks fine. Just some minor nits we have to solve before we can checkin the patch.

::: tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +37,1 @@
>    // NOTE: about:blank doesn't appear in history and clears the page 

nit: there is still an empty space at the end of the line.

@@ +37,3 @@
>    // NOTE: about:blank doesn't appear in history and clears the page 
>    //       for clean test arena
> +  LOCAL_TEST_PAGES.forEach(function (page) {

Parameters have to start with an 'a', so it should be 'aPage'. This applies to all files.

::: tests/functional/testAwesomeBar/testGoButton.js
@@ +37,5 @@
>    locationBar.focus({type: "shortcut"});
>    locationBar.type(LOCAL_TEST_PAGES[1]);
> +  controller.waitFor(function () {
> +    return locationBar.value === LOCAL_TEST_PAGES[1];
> +  }, "Location bar contains the typed data - expected '" + LOCAL_TEST_PAGE[1] + "'");

Missing 'S' in 'LOCAL_TEST_PAGE[1]'
Comment 8 Andreea Matei [:AndreeaMatei] 2012-09-12 07:35:50 PDT
Created attachment 660432 [details] [diff] [review]
patch v3.1

Updated all parameters to have aParameter name and waitFor's messages to not show current element, just the expected one. Checked the patch on Ubuntu and Mac OS X.

The whitespaces issue was solved in bug 789030.
Comment 9 Henrik Skupin (:whimboo) 2012-09-13 00:39:29 PDT
Comment on attachment 660432 [details] [diff] [review]
patch v3.1

Review of attachment 660432 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +49,5 @@
>  
> +  // Wait for the location bar to contain the entire test string
> +  controller.waitFor(function () {
> +    return locationBar.value === TEST_STRING;
> +  }, "Location bar contains the typed data - expected '" + TEST_STRING + "'");

Given that the same fix for checkitemhiglight didn't work I wonder if this is the right way to go. It gives us lesser information as before with the expect.contain() check.

I wonder if there is really something wrong with typing in the locationbar. It would be probably good to create minimized test which runs the typing of characters into the location bar in a loop, so we can check what's really wrong.

::: tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +50,5 @@
>    // Focus the locationbar, delete any contents there
>    locationBar.clear();
>  
> +  // Type the test string into the location bar
> +  locationBar.type(LOCAL_TEST_PAGE.string);

I don't think that we need this comment across files.
Comment 10 Andreea Matei [:AndreeaMatei] 2012-09-13 02:27:38 PDT
Created attachment 660763 [details] [diff] [review]
testcase

Testcase for typing the string "community" into the location bar.
Did two loops of 100 runs, one types the string step by step and checks it is the expected one, the other types continually and dumps in the end the location bar value to the console.
Looks like it is not an issue of typing indeed, considering we have no waitFor neither in the testcase nor in the locationBar.type() method from toolbars.js.

Checked this on Ubuntu and Mac OS X 10.7.4.

It is possible that while typing the full string, the test runs so fast that when checks the underlined text, the location bar is just at the "communit" stage?

In checkItemHighlight we verify underlined text on title and url. But since we fail there, the url is not checked anymore. I will check only the url to see if it's reproducible there too or just in the results title.

Also will look at the underlined method to see if the actually highlight in Nightly has the same properties as the underlined.
Comment 11 Andreea Matei [:AndreeaMatei] 2012-09-13 08:21:33 PDT
Created attachment 660848 [details] [diff] [review]
patch v4

Updated as requested, just something to mention:
Strangely, in testSuggestedHistory.js, if done the changes as requested in comment #4 about replacing the waitFor autocomplete.isOpened and expect:
>    expect.equal(locationBar.autoCompleteResults.visibleResults.length, 1,
>                 "There is one visible result in the autocomplete list");

> Shouldn't we better wait for this instead of isOpened?

I get this failure:
http://mozmill-crowd.blargon7.com/#/functional/failure?branch=18.0&platform=Linux&from=2012-09-06&to=2012-09-13&test=%2FtestAwesomeBar%2FtestSuggestHistory.js&func=testSuggestHistory.js%3A%3AtestSuggestHistoryAndBookmarks

So I didn't changed it and works as expected.
The controller.asserts in testCheckItemHighlight.js are taking care of in bug 782918.
Comment 12 Dave Hunt (:davehunt) 2012-09-14 08:01:00 PDT
Comment on attachment 660848 [details] [diff] [review]
patch v4

Review of attachment 660848 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +32,5 @@
>   * Check Escape key functionality during auto-complete process
>   */
>  var testEscape = function() {
>    // Open some local pages to set up the test environment
> +  LOCAL_TEST_PAGES.forEach(function (aLocalPage) {

nit: Rename to simply aPage to be consistent with other tests.

@@ +43,5 @@
>  
>    // Focus the locationbar and delete any content that is there
>    locationBar.clear();
>  
> +  locationBar.type(TEST_STRING);

We don't wait here for the location bar to contain the entire test string. To be consistent, shouldn't we replace the following expect.contain with the waitFor used in other tests.

::: tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +49,2 @@
>    locationBar.type(LOCAL_TEST_PAGE.string);
>  

nit: To be consistent with the other tests we should remove this blank line.

::: tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +55,5 @@
>  
>    // Focus the locationbar, delete any contents there
>    locationBar.clear();
>  
> +  // Type the test string into the location bar

We don't need this comment here.
Comment 13 Andreea Matei [:AndreeaMatei] 2012-09-14 08:11:29 PDT
Created attachment 661230 [details] [diff] [review]
patch v4.1

Addressed all requests.
Comment 14 Andreea Matei [:AndreeaMatei] 2012-09-14 09:29:57 PDT
Created attachment 661260 [details] [diff] [review]
patch v4.1

Updated the patch due to changes in bug 790174 which has landed.
Comment 15 Dave Hunt (:davehunt) 2012-09-14 09:55:38 PDT
Comment on attachment 661260 [details] [diff] [review]
patch v4.1

Review of attachment 661260 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +46,5 @@
>  
> +  locationBar.type(TEST_STRING);
> +  controller.waitFor(function () {
> +    return locationBar.value === LOCAL_TEST_PAGE.string;
> +  }, "Location bar contains the typed data - expected '" + LOCAL_TEST_PAGE.string + "'");

This fails for me with: "LOCAL_TEST_PAGE is not defined". You should be using TEST_STRING rather than LOCAL_TEST_PAGE.string.
Comment 16 Andreea Matei [:AndreeaMatei] 2012-09-14 15:47:36 PDT
Created attachment 661390 [details] [diff] [review]
patch v4.2

Sorry about that, modified accordingly.
Comment 17 Andreea Matei [:AndreeaMatei] 2012-09-17 03:13:45 PDT
Comment on attachment 661390 [details] [diff] [review]
patch v4.2

This will no longer apply cleanly since the back out on bug 790174. I will wait for Alex's patch on bug 782918 to get landed first.
Comment 18 Andreea Matei [:AndreeaMatei] 2012-09-18 01:17:29 PDT
Created attachment 662065 [details] [diff] [review]
patch v4.3

As Henrik suggested yesterday on Automation meeting, have made no changes to testCheckItemHighlight.js so it no longer depends on Alex's bug.
Comment 19 Henrik Skupin (:whimboo) 2012-09-18 02:59:10 PDT
Comment on attachment 662065 [details] [diff] [review]
patch v4.3

Review of attachment 662065 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine except two nits I would like to see fixed. Once done we can check-in. 

As a tip for the future, please take care of those unnecessary comment lines in internal reviews.

::: tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +59,5 @@
>  
>    // Check that the first item in the drop down list is selected
>    controller.waitFor(function () {
>      return locationBar.autoCompleteResults.selectedIndex === 0;
> +  }, "The first item should be selected");

Please check that you also remove all the unnecessary comments before waitFor calls. For this one specifically please add more details in the message param so we now what we check.

::: tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +59,2 @@
>    expect.equal(locationBar.autoCompleteResults.visibleResults.length, 1,
>                 "There is one visible result in the autocomplete list");

No need for this extra comment. Just make the message better to understand.
Comment 20 Andreea Matei [:AndreeaMatei] 2012-09-18 03:18:36 PDT
Created attachment 662094 [details] [diff] [review]
patch v4.4

Addressed all requests.
Comment 21 Henrik Skupin (:whimboo) 2012-09-18 03:31:59 PDT
Comment on attachment 662094 [details] [diff] [review]
patch v4.4

Review of attachment 662094 [details] [diff] [review]:
-----------------------------------------------------------------

::: tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +59,3 @@
>    controller.waitFor(function () {
>      return locationBar.autoCompleteResults.selectedIndex === 0;
> +  }, "The first item in the drop down list should be selected - expected '0'");

There is no need for the expect '0' part here. All the information we need is part of the message already.
Comment 22 Andreea Matei [:AndreeaMatei] 2012-09-18 03:35:28 PDT
Created attachment 662098 [details] [diff] [review]
patch v4.5

Changed the message.
Comment 23 Henrik Skupin (:whimboo) 2012-09-18 03:43:50 PDT
http://hg.mozilla.org/qa/mozmill-tests/rev/3a6fecfc03cd (default)

If it sticks lets get it backported.
Comment 24 Andreea Matei [:AndreeaMatei] 2012-09-19 07:04:32 PDT
I have checked all branches and the landed patch applies cleanly to all of them (aurora, beta, release, esr10).
Comment 26 Henrik Skupin (:whimboo) 2012-09-22 03:30:21 PDT
The push to esr10 caused a problem because "locationBar.editBookmarksPanel" doesn't exist on the esr10 branch yet.

http://mozmill-ci.blargon7.com/#/functional/report/fac9ae8444a3cbcc1dcf9cbaff160d3f

We can undo the change once the patch on bug 708582 landed on esr10.
Comment 27 Henrik Skupin (:whimboo) 2012-09-22 03:30:40 PDT
I pushed a bustage fix for now:
http://hg.mozilla.org/qa/mozmill-tests/rev/b1bbb946f8d1
Comment 28 Henrik Skupin (:whimboo) 2012-09-25 00:28:06 PDT
Backout of the bustage fix on esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/5cb3f518e990

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