1113158 - Refactor testAwesomeBar tests to use nsINavHistoryService.addObserver method instead of sleep

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Teodor Druta]

testAccessLocationBar, testCheckItemHighlight, testEscapeAutocomplete, testFaviconInAutocomplete, testSuggestHistory, testSwitchToTab, testVisibleItemsMax.

These tests are using a sleep to wait for history items to be added to the db.
These tests should be refactored to use "nsINavHistoryService.addObserver" "onVisit" method.

Comment 1

[Teodor Druta]

Attachment: fixawesomebar.patch

The attached patch is my fix proposition for replacing the "controller.sleep(*)" with the nsINavHistoryService.addObserver method in testAwesomeBar tests.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Do you have some timing information in terms of how many seconds those tests run with and without this change?

Comment 3

[Teodor Druta]

Attachment: test_awesomebar_time_results.txt

Tested on Ubuntu 14.04 x86_64 machine with the latest x86_64 Nightly (2015-01-04):

The total runtime for all of the testAwesomeBar tests:
* without the fix is 42192ms.
* with the fix applied is 14031ms.

It runs three times faster with the fix applied.
Besides runtime improvements, I think this patch should fix other intermittent failures reported for the testAwesomeBar tests.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We safe about 30s for those tests? Wow, this is amazing. With that we should certainly backport this to all the older branches to speed up our performance especially for release testing.

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8539270 [details] [diff] [review]
fixawesomebar.patch

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

This looks good to me, but I would like some small updates:
* update functions (setupModule, teardownModule, test function) declarations syntax to not use var anymore (in all files)
* remove the lines where you include the assertions module, it's not necessary anymore (in all files)
* I suggest you use "visited++" instead of "visited += 1", as it is shorter, simpler and easier to follow
* and a couple of nits

::: firefox/tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +63,5 @@
> +                   "Location bar value is correct, " +
> +                   "received: '" + locationBar.value + "', expected: '" + TEST_DATA.url + "'");
> +
> +    // Check if TEST_DATA page has been added to history
> +    assert.waitFor(() => visited, "'" + TEST_DATA.url + "' has been visited, received");

The message either is missing something after "received", or needs to have that word removed.

::: firefox/tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +31,5 @@
>  function teardownModule(aModule) {
>    aModule.tabBrowser.closeAllTabs();
>  }
>  
>  /*

nit: /**

Comment 6

[Teodor Druta]

Attachment: fixawesomebar.patch

Updated functions, removing the var declaration in all awesomebar tests.
Removed the lines for including the assert and expect modules in all awesomebar tests.
Updated "visited += 1" to "visited++".
Fixed all reported nits.

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8544546 [details] [diff] [review]
fixawesomebar.patch

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

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +40,4 @@
>  
> +  var visitListener = {
> +    onVisit : function(aURI) {
> +      // Check ig correct url is added to history

nit: if

@@ +64,5 @@
> +      }
> +    });
> +
> +    // Check if all TEST_DATA pages have been added to history
> +    assert.waitFor(() => (visited === 3), "All pages have been visited, received: " + visited);

I think this goes over 80chars, lets put the message on a new line.

@@ +68,5 @@
> +    assert.waitFor(() => (visited === 3), "All pages have been visited, received: " + visited);
> +  }
> +  finally {
> +    places.historyService.removeObserver(visitListener, false);
> +  }

Lets move this code to a separate method in places.js, would be cleaner for the tests. We keep observers separate and here we duplicate for each test, better to use a callback for the try block.

::: firefox/tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +59,5 @@
> +                     "received: '" + locationBar.value + "', expected: '" + aPage + "'");
> +    });
> +
> +    // Check if all TEST_DATA pages have been added to history
> +    assert.waitFor(() => (visited === 2), "All pages have been visited, received: " + visited);

Same here.

::: firefox/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +60,5 @@
> +
> +    // Wait for location bar to have the correct value
> +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> +                   "Location bar value is correct, " +
> +                   "received: '" + locationBar.value + "', expected: '" + TEST_DATA.url + "'");

Not sure we need all this, both received and expected and if received will be accurate time wise.

@@ +63,5 @@
> +                   "Location bar value is correct, " +
> +                   "received: '" + locationBar.value + "', expected: '" + TEST_DATA.url + "'");
> +
> +    // Check if TEST_DATA page have been added to history
> +    assert.waitFor(() => visited, "'" + TEST_DATA.url + "'' page has been visited");

And here.

::: firefox/tests/functional/testAwesomeBar/testGoButton.js
@@ +26,5 @@
>  
>  /**
>   * Test clicking location bar, typing a URL and clicking the GO button
>   */
> +function testAddressFieldAndGoButton() {

These changes are not the scope of this bug, especially if these tests don't use the changes the bug was filed for. Lets not touch them.

::: firefox/tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +59,5 @@
> +
> +    // Wait for location bar to have the correct value
> +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> +                   "Location bar value is correct, " +
> +                   "received: '" + locationBar.value + "', expected: '" + TEST_DATA.url + "'");

New line here too.

::: firefox/tests/functional/testAwesomeBar/testSwitchToTab.js
@@ +57,5 @@
> +      tabBrowser.openTab();
> +    });
> +
> +    // Check if all TEST_DATA pages have been added to history
> +    assert.waitFor(() => (visited === 3), "All pages have been visited, received: " + visited);

New line.

::: firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +70,5 @@
> +                     "received: '" + locationBar.value + "', expected: '" + aPage + "'");
> +    });
> +
> +    // Check if all TEST_DATA pages have been added to history
> +    assert.waitFor(() => (visited === 8), "All pages have been visited, received: " + visited);

Here too.

Comment 8

[Teodor Druta]

Attachment: fixawesomebarv2.patch

Reverted out of patch scope changes (renaming var functions declarations, removing assert and expect from included modules).
Added a new visitListener function to the places lib, changed all the tests to use the function. Fixed all nits.

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8544655 [details] [diff] [review]
fixawesomebarv2.patch

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

LGTM, with the nit bellow.

::: lib/places.js
@@ +153,5 @@
> + * Initiates an onVisit listener for the historyService observer
> + *
> + * @param [string] aURI
> + *        List of URIs to listen for when added to history
> + * @param {function} aCallback

The parameters should be ordered alphabetically

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8544655 [details] [diff] [review]
fixawesomebarv2.patch

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

::: lib/places.js
@@ +153,5 @@
> + * Initiates an onVisit listener for the historyService observer
> + *
> + * @param [string] aURI
> + *        List of URIs to listen for when added to history
> + * @param {function} aCallback

Why? This is not a dict and the order of parameters should depend on the importance and general API style. Lets compare with other methods to make sure it is correct.

Comment 11

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8544655 [details] [diff] [review]
fixawesomebarv2.patch

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

Please request from Henrik with these changed.

::: firefox/tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +41,5 @@
> +    // Open some local pages to set up the test environment
> +    TEST_DATA.forEach(aPage => {
> +      locationBar.loadURL(aPage);
> +      controller.waitForPageLoad();
> +      // Wait for location bar to have the correct value

Lets remove this comment from all tests, it's clear from the code and the message. Only in testAccessLocationBar makes sense as we're skipping about:blank there.

::: lib/places.js
@@ +151,5 @@
>  
> +/**
> + * Initiates an onVisit listener for the historyService observer
> + *
> + * @param [string] aURI

curly brackets for the type please

Comment 12

[Teodor Druta]

Attachment: fixawesomebarv3.patch

Removed the "Wait for location bar .." comment everywhere it applied.

> > +/**
> > + * Initiates an onVisit listener for the historyService observer
> > + *
> > + * @param [string] aURI
> 
> curly brackets for the type please

Updated this to the offical JSDoc markup, "{(string|string[])}", which literally means "a string or an array of strings".

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8545217 [details] [diff] [review]
fixawesomebarv3.patch

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

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +47,5 @@
> +
> +      // Skip location bar value check for 'about:blank'
> +      if (aIndex !== 3) {
> +        assert.waitFor(() => (locationBar.value === aPage),
> +                       "Location bar value is correct, expected: '" + aPage + "'");

Not sure why we need this extra check here. Please explain.

::: firefox/tests/functional/testAwesomeBar/testEscapeAutocomplete.js
@@ +43,5 @@
> +      locationBar.loadURL(aPage);
> +      controller.waitForPageLoad();
> +
> +      assert.waitFor(() => (locationBar.value === aPage),
> +                     "Location bar value is correct, expected: '" + aPage + "'");

Same as for the other test.

::: firefox/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
@@ +47,5 @@
> +    locationBar.loadURL(TEST_DATA.url);
> +    controller.waitForPageLoad();
> +
> +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> +                   "Location bar value is correct, expected: '" + TEST_DATA.url + "'");

Same es for the other test.

::: firefox/tests/functional/testAwesomeBar/testSuggestHistory.js
@@ +48,3 @@
>  
> +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> +                   "Location bar value is correct, expected: '" + TEST_DATA.url + "'");

Same as for the other test.

::: firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js
@@ +53,5 @@
> +      locationBar.loadURL(aPage);
> +      controller.waitForPageLoad();
> +
> +      assert.waitFor(() => (locationBar.value === aPage),
> +                     "Location bar value is correct, expected: '" + aPage + "'");

Same as for the other test.

::: lib/places.js
@@ +152,5 @@
> +/**
> + * Initiates an onVisit listener for the historyService observer
> + *
> + * @param {(string|string[])} aURI
> + *        URI or array of URIs to listen for changes to the history

We use URL everywhere else, right? Also URI != URL.

@@ +156,5 @@
> + *        URI or array of URIs to listen for changes to the history
> + * @param {function} aCallback
> + *        Callback for adding URIs to the history
> + */
> +function visitListener(aURI, aCallback) {

maybe waitForVisited?

@@ +165,5 @@
> +  // Initialize visited counter
> +  var visited = 0;
> +  var expectedVisits = URIs.length;
> +
> +  var visitListener = {

Don't override your method name. Also this is an observer, so visitObserver makes more sense.

@@ +166,5 @@
> +  var visited = 0;
> +  var expectedVisits = URIs.length;
> +
> +  var visitListener = {
> +    onVisit : function(aURI) {

nit: blank after function

@@ +169,5 @@
> +  var visitListener = {
> +    onVisit : function(aURI) {
> +      // Check if the correct URI is added to history
> +      assert.waitFor(() => (URIs.indexOf(aURI.spec) !== -1),
> +                     "Correct page has been visited, received: " + aURI.spec);

How do you figure out that each URL got a notification? With that check it could also be that all the notifications belong to the same URL. Best is to setup a hash. Personally I would only allow a single URL.

@@ +181,5 @@
> +
> +    // Check if all the URIs have been added to history
> +    assert.waitFor(() => (visited === expectedVisits),
> +                   "All pages have been visited, " +
> +                   "expected: " + expectedVisits + ", received: " + visited);

As I have mentioned a couple of times in the past, there is no value in passing in visited because the message is evaluated when waitFor() is called. So visited will not contain what you expect and totally gives a wrong assumption.

@@ +184,5 @@
> +                   "All pages have been visited, " +
> +                   "expected: " + expectedVisits + ", received: " + visited);
> +  }
> +  finally {
> +    historyService.removeObserver(visitListener, false);

There is no 2nd parameter for this method.

Comment 14

[Teodor Druta]

Attachment: fixawesomebarv4.patch

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Comment on attachment 8545217 [details] [diff] [review]
> fixawesomebarv3.patch
> 
> Review of attachment 8545217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
> @@ +47,5 @@
> > +
> > +      // Skip location bar value check for 'about:blank'
> > +      if (aIndex !== 3) {
> > +        assert.waitFor(() => (locationBar.value === aPage),
> > +                       "Location bar value is correct, expected: '" + aPage + "'");
> 
> Not sure why we need this extra check here. Please explain.
> 
> ::: firefox/tests/functional/testAwesomeBar/testEscapeAutocomplete.js
> @@ +43,5 @@
> > +      locationBar.loadURL(aPage);
> > +      controller.waitForPageLoad();
> > +
> > +      assert.waitFor(() => (locationBar.value === aPage),
> > +                     "Location bar value is correct, expected: '" + aPage + "'");
> 
> Same as for the other test.
> 
> ::: firefox/tests/functional/testAwesomeBar/testFaviconInAutocomplete.js
> @@ +47,5 @@
> > +    locationBar.loadURL(TEST_DATA.url);
> > +    controller.waitForPageLoad();
> > +
> > +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> > +                   "Location bar value is correct, expected: '" + TEST_DATA.url + "'");
> 
> Same es for the other test.
> 
> ::: firefox/tests/functional/testAwesomeBar/testSuggestHistory.js
> @@ +48,3 @@
> >  
> > +    assert.waitFor(() => (locationBar.value === TEST_DATA.url),
> > +                   "Location bar value is correct, expected: '" + TEST_DATA.url + "'");
> 
> Same as for the other test.
> 
> ::: firefox/tests/functional/testAwesomeBar/testVisibleItemsMax.js
> @@ +53,5 @@
> > +      locationBar.loadURL(aPage);
> > +      controller.waitForPageLoad();
> > +
> > +      assert.waitFor(() => (locationBar.value === aPage),
> > +                     "Location bar value is correct, expected: '" + aPage + "'");
> 
> Same as for the other test.
> 

Actually without this check the tests will fail intermittently, because we need to wait for the locationbar to have the url value before loading the next page, in a forEach loop, as you may had noticed this check is missing from the tests where we have just a single page in TEST_DATA.

> ::: lib/places.js
> @@ +152,5 @@
> > +/**
> > + * Initiates an onVisit listener for the historyService observer
> > + *
> > + * @param {(string|string[])} aURI
> > + *        URI or array of URIs to listen for changes to the history
> 
> We use URL everywhere else, right? Also URI != URL.
> 

Updated to URL. URL == URI, but that doesn't mean that URI != URL ? IMHO in our specific case URI will always be URL.

> @@ +156,5 @@
> > + *        URI or array of URIs to listen for changes to the history
> > + * @param {function} aCallback
> > + *        Callback for adding URIs to the history
> > + */
> > +function visitListener(aURI, aCallback) {
> 
> maybe waitForVisited?
> 

updated the function name to waitForVisited

> @@ +165,5 @@
> > +  // Initialize visited counter
> > +  var visited = 0;
> > +  var expectedVisits = URIs.length;
> > +
> > +  var visitListener = {
> 
> Don't override your method name. Also this is an observer, so visitObserver
> makes more sense.
> 

Renamed to visitObserver

> 
> @@ +169,5 @@
> > +  var visitListener = {
> > +    onVisit : function(aURI) {
> > +      // Check if the correct URI is added to history
> > +      assert.waitFor(() => (URIs.indexOf(aURI.spec) !== -1),
> > +                     "Correct page has been visited, received: " + aURI.spec);
> 
> How do you figure out that each URL got a notification? With that check it
> could also be that all the notifications belong to the same URL. Best is to
> setup a hash. Personally I would only allow a single URL.
> 
> @@ +181,5 @@
> > +
> > +    // Check if all the URIs have been added to history
> > +    assert.waitFor(() => (visited === expectedVisits),
> > +                   "All pages have been visited, " +
> > +                   "expected: " + expectedVisits + ", received: " + visited);
> 
> As I have mentioned a couple of times in the past, there is no value in
> passing in visited because the message is evaluated when waitFor() is
> called. So visited will not contain what you expect and totally gives a
> wrong assumption.
> 

:( My first implementation of this was with a hash, I figured to use a counter to save some space in code lines.
Updated this code to use a key => boolean value object.

> @@ +184,5 @@
> > +                   "All pages have been visited, " +
> > +                   "expected: " + expectedVisits + ", received: " + visited);
> > +  }
> > +  finally {
> > +    historyService.removeObserver(visitListener, false);
> 
> There is no 2nd parameter for this method.

Fixed.

Comment 15

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8545951 [details] [diff] [review]
fixawesomebarv4.patch

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

::: lib/places.js
@@ +169,5 @@
> +    visitedURLs[aURL] = false;
> +  });
> +
> +  var visitObserver = {
> +    onVisit : function (aURI) {

aURL here too, since we agreed to use that.

@@ +189,5 @@
> +        if (!visitedURLs[aURL]) {
> +          ok = false;
> +          // Stop loop if URL has not been visited
> +          // in order to save waitFor timeout time
> +          arr.length = 0;

Couldn't we better use .some here and just return? We'll remove this arr completely.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #14)
> > > +      // Skip location bar value check for 'about:blank'
> > > +      if (aIndex !== 3) {
> > > +        assert.waitFor(() => (locationBar.value === aPage),
> > > +                       "Location bar value is correct, expected: '" + aPage + "'");
> > 
> > Not sure why we need this extra check here. Please explain.
> 
> Actually without this check the tests will fail intermittently, because we
> need to wait for the locationbar to have the url value before loading the
> next page, in a forEach loop, as you may had noticed this check is missing
> from the tests where we have just a single page in TEST_DATA.

We are loading a page here and wait for it to be loaded. By that time the locationbar has to show the URL. I would understand that without the waitForPageload. But this sounds scary to me. The locationbar should not update AFTER the page has been finished loading.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Also I still propose to get rid of the URL array completely and really only call this method for a single URL.

Comment 18

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> Also I still propose to get rid of the URL array completely and really only
> call this method for a single URL.

That will totally ruin the purpose of having an observer, and will add a lot of extra time to the runs.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What? How that? Please explain that in detail.

Comment 20

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #19)
> What? How that? Please explain that in detail.

Well for the start this would mean adding for each test page an observer, listen for it to trigger, and then removing it. Let's say we have around 10 test pages in a test, and for each of these 10 pages we are adding an observer, waiting for it to trigger and then removing it, instead of adding just 1 observer which will listen for all 10, imho this approach is redundant. This will mean that we will need to waitFor each observer to trigger for those theoretical 10 pages instead of waiting just for one observer.

Actually I quick tested this just now modifying testAwesomeBar/testVisibleItemsMax.js which has 8 test pages. The approach of instantiating a separate observer for each single page, added around ~20-350ms more than using a single observer for all the pages, I know it's not a really big difference, but it adds up for multiple test files.

Another thing I wanted to point out is that an API method implementation should offer its user (the coder) the possibility and the flexibility of it to be used with multiple approaches, in this case the user has the choice to use it with a single URL or with an array of URLs.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #20)
> Well for the start this would mean adding for each test page an observer,
> listen for it to trigger, and then removing it. Let's say we have around 10
> test pages in a test, and for each of these 10 pages we are adding an
> observer, waiting for it to trigger and then removing it, instead of adding
> just 1 observer which will listen for all 10, imho this approach is
> redundant. This will mean that we will need to waitFor each observer to
> trigger for those theoretical 10 pages instead of waiting just for one
> observer.

The number of notifications triggered for the registered topic is not different depending on which method you are using. It may only be the overlap with page loads you have here.

> Actually I quick tested this just now modifying
> testAwesomeBar/testVisibleItemsMax.js which has 8 test pages. The approach
> of instantiating a separate observer for each single page, added around
> ~20-350ms more than using a single observer for all the pages, I know it's
> not a really big difference, but it adds up for multiple test files.

Are you sure this is caused by the registration and removal of the requested topic? I would assume it's more about the overlap as mentioned above. Btw. when is the observer notification fired? I assume it's after the page has been finished loading? What is the delay?

> Another thing I wanted to point out is that an API method implementation
> should offer its user (the coder) the possibility and the flexibility of it
> to be used with multiple approaches, in this case the user has the choice to
> use it with a single URL or with an array of URLs.

An API is a clearly defined interface to be used by programs. It definitely exists to ease the work but also defines rules to make it not too easy to misuse. Please keep in mind that I'm currently forward thinking for the usage with Marionette tests. So with a general observer class (as I'm working on at https://github.com/mozilla/firefox-greenlight-tests/issues/38) we would have problems to tell the correct behavior in terms of how many times the observer has to fire for a specific object.

So for this specific case we might be able to keep what we have right now. I may have to modify it for the Marionette tests.

Comment 22

[Teodor Druta]

So, should I leave the code like it is now + Andreea's requests in the last review ?

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Leave the multiple URLs support, yes.

Comment 24

[Teodor Druta]

Attachment: fixawesomebarv5.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #15)
> Comment on attachment 8545951 [details] [diff] [review]
> fixawesomebarv4.patch
> 
> Review of attachment 8545951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/places.js
> @@ +169,5 @@
> > +    visitedURLs[aURL] = false;
> > +  });
> > +
> > +  var visitObserver = {
> > +    onVisit : function (aURI) {
> 
> aURL here too, since we agreed to use that.
> 

Actually this is the case where URI != URL, here we have a nsIURI object.

> @@ +189,5 @@
> > +        if (!visitedURLs[aURL]) {
> > +          ok = false;
> > +          // Stop loop if URL has not been visited
> > +          // in order to save waitFor timeout time
> > +          arr.length = 0;
> 
> Couldn't we better use .some here and just return? We'll remove this arr
> completely.

Updated this to use some instead of forEach.

I have also removed the check for locationbar from all respective tests, since I couldn't reproduce the failure with the latest nightly anymore, I had a few weeks ago when I initially written this code.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8548161 [details] [diff] [review]
fixawesomebarv5.patch

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

With these changes you can request from Henrik.

::: firefox/tests/functional/testAwesomeBar/testAccessLocationBar.js
@@ +40,5 @@
> +  places.waitForVisited(TEST_DATA.slice(0, -1), () => {
> +    // Open a few different sites to create a small history
> +    // NOTE: about:blank doesn't appear in history and clears the page
> +    //       for clean test arena
> +    TEST_DATA.forEach((aPage, aIndex) => {

You don't use aIndex.

::: lib/places.js
@@ +183,5 @@
> +    aCallback();
> +
> +    // Check if all required URLs have been added to the history
> +    assert.waitFor(() => {
> +      var ok = URLs.some((aURL, aIndex, aArray) => {

No need for aIndex anymore.

Comment 26

[Teodor Druta]

Attachment: fixawesomebarv6.patch

Removed unused aIndex and aArray parameters.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8548835 [details] [diff] [review]
fixawesomebarv6.patch

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

::: lib/places.js
@@ +151,5 @@
>  
> +/**
> + * Initiates an onVisit listener for the historyService observer
> + *
> + * @param {(string|string[])} aURLs

I don't think we need the rounded brackets here.

@@ +159,5 @@
> + */
> +function waitForVisited(aURLs, aCallback) {
> +  assert.ok(typeof aCallback, "function", "Callback has been specified");
> +
> +  var URLs = (typeof aURLs === "string") ? [aURLs] : aURLs;

The code will break if aURLs is not provided.

@@ +172,5 @@
> +  var visitObserver = {
> +    onVisit : function (aURI) {
> +      // Check if the correct URL was added to the history
> +      expect.equal(visitedURLs[aURI.spec], false,
> +                   "Correct page has been visited, received: " + aURI.spec);

I don't see why we have to check that a URL is visited twice. For us it is important that all the URLs have been marked as visited. So please remove those lines.

@@ +176,5 @@
> +                   "Correct page has been visited, received: " + aURI.spec);
> +      visitedURLs[aURI.spec] = true;
> +    }
> +  };
> +  historyService.addObserver(visitObserver, false);

nit: blank line above please.

@@ +186,5 @@
> +    assert.waitFor(() => {
> +      var ok = URLs.some(aURL => {
> +        return visitedURLs[aURL];
> +      });
> +      return ok;

You can directly return without the ok variable.

Comment 28

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Comment on attachment 8548835 [details] [diff] [review]
> fixawesomebarv6.patch
> 
> Review of attachment 8548835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/places.js
> @@ +151,5 @@
> >  
> > +/**
> > + * Initiates an onVisit listener for the historyService observer
> > + *
> > + * @param {(string|string[])} aURLs
> 
> I don't think we need the rounded brackets here.
> 

This is actually the official JSDoc syntax, as explained here: http://usejsdoc.org/tags-param.html

> 
> @@ +172,5 @@
> > +  var visitObserver = {
> > +    onVisit : function (aURI) {
> > +      // Check if the correct URL was added to the history
> > +      expect.equal(visitedURLs[aURI.spec], false,
> > +                   "Correct page has been visited, received: " + aURI.spec);
> 
> I don't see why we have to check that a URL is visited twice. For us it is
> important that all the URLs have been marked as visited. So please remove
> those lines.
> 

This is not used for checking if the URL has been visited, but rather if the onVisit observer have not been triggered for other URLs which have not been requested or if the onVisit observer has been triggered twice for the same URL. I've added this check in order to catch failures in our tests or possible regressions in firefox.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #28)
> > > + * Initiates an onVisit listener for the historyService observer
> > > + *
> > > + * @param {(string|string[])} aURLs
> > 
> > I don't think we need the rounded brackets here.
> 
> This is actually the official JSDoc syntax, as explained here:
> http://usejsdoc.org/tags-param.html

Oh, good to know! Then this is fine, thanks for the pointer!

> > @@ +172,5 @@
> > > +  var visitObserver = {
> > > +    onVisit : function (aURI) {
> > > +      // Check if the correct URL was added to the history
> > > +      expect.equal(visitedURLs[aURI.spec], false,
> > > +                   "Correct page has been visited, received: " + aURI.spec);
> > 
> > I don't see why we have to check that a URL is visited twice. For us it is
> > important that all the URLs have been marked as visited. So please remove
> > those lines.
> 
> This is not used for checking if the URL has been visited, but rather if the
> onVisit observer have not been triggered for other URLs which have not been
> requested or if the onVisit observer has been triggered twice for the same
> URL. I've added this check in order to catch failures in our tests or
> possible regressions in firefox.

We do not care if the onvisit observer has been triggered for urls not being requested by ourselves. This could happen at any time if a background task is going to do eg. a sync action. But not sure how this exactly works. So best is to simply ignore those urls and only update the array for known ones without doing checks with expect. This only adds unnecessary information to the report.

Comment 30

[Teodor Druta]

Attachment: fixawesomebarv7.patch

(In reply to Henrik Skupin (:whimboo))
> ::: lib/places.js
> @@ +159,5 @@
> > + */
> > +function waitForVisited(aURLs, aCallback) {
> > +  assert.ok(typeof aCallback, "function", "Callback has been specified");
> > +
> > +  var URLs = (typeof aURLs === "string") ? [aURLs] : aURLs;
> 
> The code will break if aURLs is not provided.
> 

Added a check for the aURLs argument.

> @@ +176,5 @@
> > +                   "Correct page has been visited, received: " + aURI.spec);
> > +      visitedURLs[aURI.spec] = true;
> > +    }
> > +  };
> > +  historyService.addObserver(visitObserver, false);
> 
> nit: blank line above please.
> 

Fixed.

> @@ +186,5 @@
> > +    assert.waitFor(() => {
> > +      var ok = URLs.some(aURL => {
> > +        return visitedURLs[aURL];
> > +      });
> > +      return ok;
> 
> You can directly return without the ok variable.

Fixed.

> We do not care if the onvisit observer has been triggered for urls not being
> requested by ourselves. This could happen at any time if a background task
> is going to do eg. a sync action. But not sure how this exactly works. So
> best is to simply ignore those urls and only update the array for known ones
> without doing checks with expect. This only adds unnecessary information to
> the report.

ok, then. Removed the lines for the check.

Comment 31

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8549521 [details] [diff] [review]
fixawesomebarv7.patch

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

LGTM.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8549521 [details] [diff] [review]
fixawesomebarv7.patch

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

::: lib/places.js
@@ +184,5 @@
> +    // Check if all required URLs have been added to the history
> +    assert.waitFor(() =>
> +      URLs.some(aURL => {
> +        return visitedURLs[aURL];
> +      }), "All pages have been visited");

Just checked the documentation for `some()`, and this will not work. Reason is that it will return for the very first true value. But we have to check all registered URLs for the value, and only return true if all entries are set to true.

It might be good to add unit tests for that method.

Comment 33

[Teodor Druta]

Attachment: fixawesomebarv8.patch

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Just checked the documentation for `some()`, and this will not work. Reason
> is that it will return for the very first true value. But we have to check
> all registered URLs for the value, and only return true if all entries are
> set to true.

Yes, you're right, what we need here is '.every()', which will return if all entries are set to true. Fixed.

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8549637 [details] [diff] [review]
fixawesomebarv8.patch

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

Correct, missed that.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8549637 [details] [diff] [review]
fixawesomebarv8.patch

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

Lets get this landed! I'm curious about the timing changes on our CI machines.

Comment 36

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/6af6620a09c1 (default)

We'll backport next days.

Comment 37

[Andreea Matei [:AndreeaMatei]]

Teodor, please check backporting. We want down to beta at least, if not all.

Comment 38

[Teodor Druta]

Comment on attachment 8549637 [details] [diff] [review]
fixawesomebarv8.patch

The patch applies cleanly and runs well on all branches down to release.

Comment 39

[Teodor Druta]

Attachment: wtfrvstd_esr31.patch

We decided to backport this down to esr31. Here is the patch.

Comment 40

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/13648e13eb62 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/59da2320506a (beta)

We need a follow up here, we have testAccessLocationBar.js failing intermitently with "All pages have been visited", on aurora and beta, osx only. Don't wanna backout, lets fix it asap today. Nightly ran fine.

Comment 41

[Teodor Druta]

Attachment: megafix_tstccslctnbr.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #40)
> http://hg.mozilla.org/qa/mozmill-tests/rev/13648e13eb62 (aurora)
> http://hg.mozilla.org/qa/mozmill-tests/rev/59da2320506a (beta)
> 
> We need a follow up here, we have testAccessLocationBar.js failing
> intermitently with "All pages have been visited", on aurora and beta, osx
> only. Don't wanna backout, lets fix it asap today. Nightly ran fine.

I don't really know what this test's problem is, every other awesomeBar test seems to run fine, waiting for the locationBar value in the forEach loop when the pages are open seems to help. Let's add this fix and I'll investigate this more to get to the root cause of this failure when I'll have a little bit more time on my hands.

Comment 42

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8553097 [details] [diff] [review]
megafix_tstccslctnbr.patch

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

Yes, this fixed the issue.

Lets investigate more even after landing, why this happens.

Comment 43

[Andreea Matei [:AndreeaMatei]]

http://hg.mozilla.org/qa/mozmill-tests/rev/b9871d57695b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/47e7631588c2 (beta)

Comment 44

[Andreea Matei [:AndreeaMatei]]

Please update the patch for release and esr31 to have the combined fixes.

Comment 45

[Teodor Druta]

Attachment: fixawesomebar_release.patch

This is be the combined patch for mozilla-release.

Comment 46

[Teodor Druta]

Attachment: fixawesomebar_esr31.patch

... and this is for esr31.

Comment 47

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8554521 [details] [diff] [review]
fixawesomebar_release.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/009f75500765 (release)

Comment 48

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8554522 [details] [diff] [review]
fixawesomebar_esr31.patch

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

https://hg.mozilla.org/qa/mozmill-tests/rev/05949a34120b (esr31)