Closed Bug 787924 Opened 9 years ago Closed 9 years ago

Update tests in /testAwesomeBar to wait for the string in location bar to be fully typed

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

defect

Tracking

(firefox15 fixed, firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox-esr10 fixed)

RESOLVED FIXED
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
firefox17 --- fixed
firefox18 --- fixed
firefox-esr10 --- fixed

People

(Reporter: AndreeaMatei, Assigned: AndreeaMatei)

References

()

Details

(Whiteboard: s=q3 u=failure c=awesomebar p=1)

Attachments

(2 files, 10 obsolete files)

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.
Assignee: nobody → andreea.matei
Depends on: 784644
Priority: -- → P1
Whiteboard: s=2012-8-27 u=failure c=awesomebar
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #658099 - Flags: review?(hskupin)
Attachment #658099 - Flags: review?(dave.hunt)
Whiteboard: s=2012-8-27 u=failure c=awesomebar → s=2012-8-27 u=failure c=awesomebar p=1
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.
Attachment #658099 - Flags: review?(hskupin)
Attachment #658099 - Flags: review?(dave.hunt)
Attachment #658099 - Flags: review-
Attached patch patch v2 (obsolete) — Splinter Review
Removed all whitespaces and added locationBar.type + waitFor in testVisibleItemsMax.js instead of for each letter.
Attachment #658099 - Attachment is obsolete: true
Attachment #658420 - Flags: review?(hskupin)
Attachment #658420 - Flags: review?(dave.hunt)
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.
Attachment #658420 - Flags: review?(hskupin)
Attachment #658420 - Flags: review?(dave.hunt)
Attachment #658420 - Flags: review-
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #658420 - Attachment is obsolete: true
Attachment #659262 - Flags: review?(hskupin)
Attachment #659262 - Flags: review?(dave.hunt)
For the future please do not forget to mark a bug as assigned once you agreed to work on it.
Status: NEW → ASSIGNED
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]'
Attachment #659262 - Flags: review?(hskupin)
Attachment #659262 - Flags: review?(dave.hunt)
Attachment #659262 - Flags: review-
Whiteboard: s=2012-8-27 u=failure c=awesomebar p=1 → s=q3 u=failure c=awesomebar p=1
Attached patch patch v3.1 (obsolete) — Splinter Review
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.
Attachment #659262 - Attachment is obsolete: true
Attachment #660432 - Flags: review?(hskupin)
Attachment #660432 - Flags: review?(dave.hunt)
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.
Attachment #660432 - Flags: review?(hskupin)
Attachment #660432 - Flags: review?(dave.hunt)
Attachment #660432 - Flags: review-
Attached patch testcaseSplinter Review
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.
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #660432 - Attachment is obsolete: true
Attachment #660848 - Flags: review?(hskupin)
Attachment #660848 - Flags: review?(dave.hunt)
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.
Attachment #660848 - Flags: review?(hskupin)
Attachment #660848 - Flags: review?(dave.hunt)
Attachment #660848 - Flags: review-
Attached patch patch v4.1 (obsolete) — Splinter Review
Addressed all requests.
Attachment #660848 - Attachment is obsolete: true
Attachment #661230 - Flags: review?(dave.hunt)
Attached patch patch v4.1 (obsolete) — Splinter Review
Updated the patch due to changes in bug 790174 which has landed.
Attachment #661230 - Attachment is obsolete: true
Attachment #661230 - Flags: review?(dave.hunt)
Attachment #661260 - Flags: review?(dave.hunt)
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.
Attachment #661260 - Flags: review?(dave.hunt) → review-
Attached patch patch v4.2 (obsolete) — Splinter Review
Sorry about that, modified accordingly.
Attachment #661260 - Attachment is obsolete: true
Attachment #661390 - Flags: review?(dave.hunt)
Depends on: 782918
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.
Attachment #661390 - Flags: review?(dave.hunt)
No longer depends on: 782918
Attached patch patch v4.3 (obsolete) — Splinter Review
As Henrik suggested yesterday on Automation meeting, have made no changes to testCheckItemHighlight.js so it no longer depends on Alex's bug.
Attachment #661390 - Attachment is obsolete: true
Attachment #662065 - Flags: review?(hskupin)
Attachment #662065 - Flags: review?(dave.hunt)
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.
Attachment #662065 - Flags: review?(hskupin)
Attachment #662065 - Flags: review?(dave.hunt)
Attachment #662065 - Flags: review-
Attached patch patch v4.4 (obsolete) — Splinter Review
Addressed all requests.
Attachment #662065 - Attachment is obsolete: true
Attachment #662094 - Flags: review?(hskupin)
Attachment #662094 - Flags: review?(dave.hunt)
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.
Attachment #662094 - Flags: review?(hskupin)
Attachment #662094 - Flags: review?(dave.hunt)
Attachment #662094 - Flags: review-
Attached patch patch v4.5Splinter Review
Changed the message.
Attachment #662094 - Attachment is obsolete: true
Attachment #662098 - Flags: review?(hskupin)
Attachment #662098 - Flags: review?(dave.hunt)
Attachment #662098 - Flags: review?(hskupin)
Attachment #662098 - Flags: review?(dave.hunt)
Attachment #662098 - Flags: review+
http://hg.mozilla.org/qa/mozmill-tests/rev/3a6fecfc03cd (default)

If it sticks lets get it backported.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I have checked all branches and the landed patch applies cleanly to all of them (aurora, beta, release, esr10).
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.
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.