Closed Bug 1154464 Opened 9 years ago Closed 9 years ago

Add a dedicated test for BrowserTestUtils.browserLoaded

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

Details

Attachments

(1 file, 6 obsolete files)

      No description provided.
Summary: BrowserTestUtils.browserLoaded → Promise of BrowserTestUtils.browserLoaded should resolve with browser
Wasn't sure where to put the test file...
Attachment #8592489 - Flags: review?(smacleod)
Blocks: 1083410
Comment on attachment 8592489 [details] [diff] [review]
browserLoaded resolves with promise + tests.

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

The code change looks fine to me. I really don't know if this is the right place for this test though. Also, it seems you're using a lot of conventions I'm not familiar with in the test - I think someone else might be better suited to review this test stuff than me. Paolo, could you take a look?

::: testing/mochitest/tests/browser/browser_browserLoaded_resolves_with_browser.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*globals gBrowser, Cu, add_task, BrowserTestUtils, Assert  */
> +'use strict';
> +const imports = {};
> +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);

haven't seen this pre-amble stuff in tests before, is this a new convention I can read about somewhere?

@@ +4,5 @@
> +'use strict';
> +const imports = {};
> +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> +
> +// This test is designed to pass.

Shouldn't all our tests be designed to pass? I've never seen this convention before?

@@ +5,5 @@
> +const imports = {};
> +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> +
> +// This test is designed to pass.
> +// It checks if calling BrowserTestUtils.browserLoaded() yields 

nit: trailing space.

@@ +14,5 @@
> +  Assert.strictEqual(browser.nodeName, 'browser', 'Expect a browser');
> +  gBrowser.removeTab(tab);
> +});
> +
> +// This test is designed to pass.

Same, is this needed?

@@ +15,5 @@
> +  gBrowser.removeTab(tab);
> +});
> +
> +// This test is designed to pass.
> +// It checks that BrowserTestUtils.browserLoaded works well with 

nit: trailing space.
Attachment #8592489 - Flags: review?(smacleod)
Attachment #8592489 - Flags: review?(paolo.mozmail)
Attachment #8592489 - Flags: review+
(In reply to Steven MacLeod [:smacleod] from comment #2)
> Comment on attachment 8592489 [details] [diff] [review]
> browserLoaded resolves with promise + tests.
> 
> Review of attachment 8592489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code change looks fine to me. I really don't know if this is the right
> place for this test though. 

Yeah, I wasn't sure... it was the closest "tests" directory to the file, so I thought I would try in there :) 

> Also, it seems you're using a lot of conventions
> I'm not familiar with in the test - I think someone else might be better
> suited to review this test stuff than me. Paolo, could you take a look?
> 
> :::
> testing/mochitest/tests/browser/browser_browserLoaded_resolves_with_browser.
> js
> @@ +2,5 @@
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +/*globals gBrowser, Cu, add_task, BrowserTestUtils, Assert  */
> > +'use strict';
> > +const imports = {};
> > +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> 
> haven't seen this pre-amble stuff in tests before, is this a new convention
> I can read about somewhere?

If you mean "imports" bit... without it, running the tests in e10s complains that `BrowserTestUtils` was leaked (resulting in a test failure).

> @@ +4,5 @@
> > +'use strict';
> > +const imports = {};
> > +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> > +
> > +// This test is designed to pass.
> 
> Shouldn't all our tests be designed to pass?

heh, yeah.... I thought the same :) 
> I've never seen this convention
> before?

I copied it from some other test in those test directories. I'll remove that.

> @@ +5,5 @@
> > +const imports = {};
> > +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> > +
> > +// This test is designed to pass.
> > +// It checks if calling BrowserTestUtils.browserLoaded() yields 
> 
> nit: trailing space.
> 
> @@ +14,5 @@
> > +  Assert.strictEqual(browser.nodeName, 'browser', 'Expect a browser');
> > +  gBrowser.removeTab(tab);
> > +});
> > +
> > +// This test is designed to pass.
> 
> Same, is this needed?

will remove. 

> @@ +15,5 @@
> > +  gBrowser.removeTab(tab);
> > +});
> > +
> > +// This test is designed to pass.
> > +// It checks that BrowserTestUtils.browserLoaded works well with 
> 
> nit: trailing space.

Will fix.
Attachment #8592489 - Attachment is obsolete: true
Attachment #8592489 - Flags: review?(paolo.mozmail)
Attachment #8592886 - Flags: review?(paolo.mozmail)
(In reply to Marcos Caceres [:marcosc] from comment #3)
> (In reply to Steven MacLeod [:smacleod] from comment #2)
> > Comment on attachment 8592489 [details] [diff] [review]
> > browserLoaded resolves with promise + tests.
> > 
> > Review of attachment 8592489 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The code change looks fine to me. I really don't know if this is the right
> > place for this test though. 
> 
> Yeah, I wasn't sure... it was the closest "tests" directory to the file, so
> I thought I would try in there :) 
> 
> > Also, it seems you're using a lot of conventions
> > I'm not familiar with in the test - I think someone else might be better
> > suited to review this test stuff than me. Paolo, could you take a look?
> > 
> > :::
> > testing/mochitest/tests/browser/browser_browserLoaded_resolves_with_browser.
> > js
> > @@ +2,5 @@
> > > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > > +/*globals gBrowser, Cu, add_task, BrowserTestUtils, Assert  */
> > > +'use strict';
> > > +const imports = {};
> > > +Cu.import('resource://testing-common/BrowserTestUtils.jsm', imports);
> > 
> > haven't seen this pre-amble stuff in tests before, is this a new convention
> > I can read about somewhere?
> 
> If you mean "imports" bit... without it, running the tests in e10s complains
> that `BrowserTestUtils` was leaked (resulting in a test failure).

My bad! I just checked again - I didn't know that BrowserTestUtils was automatically imported into the global namespace. I will remove the Cu.import and send again.
Attached patch Removed redundant Cu.import (obsolete) — Splinter Review
Attachment #8592886 - Attachment is obsolete: true
Attachment #8592886 - Flags: review?(paolo.mozmail)
Attachment #8592892 - Flags: review?(paolo.mozmail)
Comment on attachment 8592892 [details] [diff] [review]
Removed redundant Cu.import

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

Thanks for taking the time of doing this! I think the location of the test file is fine, and it's good to have some internal tests for browserLoaded that check parallelism.

I'm not sure however about what kind of compelling "fluent" syntax would be enabled by returning the browser that was passed as an argument. Doing this would actually prevent us from resolving the promise with something more useful, like a subset of the information from msg.data. (I'm not saying we should return this information either, but we may find a use case in the future.)

Regardless, having the test is good and maybe we should check that we resolve with undefined instead, with a comment explaining why?

I've reviewed the test but I feel like it could be shorter and focus on parallelism. For example, the part that checks that addTab returns unique browsers using a Set doesn't belong here (something would be quite wrong if this wasn't the case). And also, we can rely on Array.map and Promise.all to both return an array of the same size as the one provided, so the size checks shouldn't be needed.

::: testing/mochitest/tests/browser/browser_browserLoaded_resolves_with_browser.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +/*globals gBrowser, add_task, BrowserTestUtils, Assert  */

nit: I'd say these annotations might get obsoleted quickly, as we don't have integrated in-tree lint tools yet.

@@ +17,5 @@
> +add_task(function*() {
> +  const tabURLs = [
> +    `http://example.org`,
> +    `http://mochi.test:8888`,
> +    `http://test:80`

nit: comma on last line of multi-line intializers

@@ +26,5 @@
> +
> +  // Open tabs, keep record of browsers and promises.
> +  let loadingPromises = tabURLs.map((url) => {
> +    const tab = gBrowser.addTab(url);
> +    const browser = tab.linkedBrowser;

nit: I think we prefer "let" for assignments even if the value won't change.

@@ +29,5 @@
> +    const tab = gBrowser.addTab(url);
> +    const browser = tab.linkedBrowser;
> +    browsers.add(browser);
> +    tabs.add(tab);
> +    return BrowserTestUtils.browserLoaded(browser);

In fact, changing this to:

  return BrowserTestUtils.browserLoaded(browser).then(() => browser);

would work, and doesn't add much complexity to the already long "map" function in this example.

@@ +51,5 @@
> +  const hasAllIntialBrowsers = Array.from(browsers).every(
> +    browser => promisedBrowsersSet.has(browser)
> +  );
> +
> +  Assert.strictEqual(hasAllIntialBrowsers, hasPromisedBrowsers, 'All expected browsers were returned.');

I suppose you actually wanted to assert both are true separately, and not succeed if both are false.

But, after looking at the test again, what everything after "yield Promise.all(loadingPromises);" is actually testing is Array.map and Promise.all rather than actual parallelism, and should be removed.

A better test would be to check synchronously the loading state of each document in each of the browsers after "yield" has returned, then clean up by removing the tabs.
Attachment #8592892 - Flags: review?(paolo.mozmail)
Attachment #8592892 - Flags: review-
Attachment #8592892 - Flags: feedback+
Thanks for the great feedback, Paolo! I've reduced the tests significantly based on it. Just need a few small clarifications below.

On April 17, 2015 at 10:20:08 AM, Bugzilla@Mozilla (bugzilla-daemon@mozilla.org) wrote:
> Review of attachment 8592892 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking the time of doing this! I think the location of the test file
> is fine, and it's good to have some internal tests for browserLoaded that check
> parallelism.

Great. 

> I'm not sure however about what kind of compelling "fluent" syntax would be
> enabled by returning the browser that was passed as an argument. Doing this
> would actually prevent us from resolving the promise with something more
> useful, like a subset of the information from msg.data. (I'm not saying we
> should return this information either, but we may find a use case in the
> future.)

I think maybe it might be better to have a different method for that. the semantics of `browserLoaded` are really self evident and nice. The motivation for me lodging this bug were that I naturally expected a "browser" to be returned. 

> Regardless, having the test is good and maybe we should check that we resolve
> with undefined instead, with a comment explaining why?

Sorry, I'm confused. Do you suggests not resolving with the browser object (i.e., not changing the current behavior)? I'm ok with that, but I still think it's really nice to resolve with the loaded browser. 

> I've reviewed the test but I feel like it could be shorter and focus on
> parallelism. For example, the part that checks that addTab returns unique
> browsers using a Set doesn't belong here (something would be quite wrong if
> this wasn't the case).

Agree. 

> And also, we can rely on Array.map and Promise.all to
> both return an array of the same size as the one provided, so the size checks
> shouldn't be needed.

Agree. Removed all that. 

> :::
> testing/mochitest/tests/browser/browser_browserLoaded_resolves_with_browser.js 
> @@ +1,3 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +/*globals gBrowser, add_task, BrowserTestUtils, Assert */
> 
> nit: I'd say these annotations might get obsoleted quickly, as we don't have
> integrated in-tree lint tools yet.

Removed. 

> @@ +17,5 @@
> > +add_task(function*() {
> > + const tabURLs = [
> > + `http://example.org`,
> > + `http://mochi.test:8888`,
> > + `http://test:80`
> 
> nit: comma on last line of multi-line intializers

Fixed.

> @@ +26,5 @@
> > +
> > + // Open tabs, keep record of browsers and promises.
> > + let loadingPromises = tabURLs.map((url) => {
> > + const tab = gBrowser.addTab(url);
> > + const browser = tab.linkedBrowser;
> 
> nit: I think we prefer "let" for assignments even if the value won't change.

Fixed globally. 

> @@ +29,5 @@
> > + const tab = gBrowser.addTab(url);
> > + const browser = tab.linkedBrowser;
> > + browsers.add(browser);
> > + tabs.add(tab);
> > + return BrowserTestUtils.browserLoaded(browser);
> 
> In fact, changing this to:
> 
> return BrowserTestUtils.browserLoaded(browser).then(() => browser);
> 
> would work, and doesn't add much complexity to the already long "map" function
> in this example.

I couldn't get the above to work. However, I did significantly reduce the code for the .map. Also, I think I just want the "promisedBrowsers" there to pass to promise.all. 

I hope that's ok. 

> @@ +51,5 @@
> > + const hasAllIntialBrowsers = Array.from(browsers).every(
> > + browser => promisedBrowsersSet.has(browser)
> > + );
> > +
> > + Assert.strictEqual(hasAllIntialBrowsers, hasPromisedBrowsers, 'All
> expected browsers were returned.');
> 
> I suppose you actually wanted to assert both are true separately, and not
> succeed if both are false.
> 
> But, after looking at the test again, what everything after "yield
> Promise.all(loadingPromises);" is actually testing is Array.map and Promise.all 
> rather than actual parallelism, and should be removed.

Removed. 

> A better test would be to check synchronously the loading state of each
> document in each of the browsers after "yield" has returned, then clean up by
> removing the tabs.

Agree. Made this change. Let me know what you think.
Attachment #8592892 - Attachment is obsolete: true
Attachment #8594563 - Flags: feedback?(paolo.mozmail)
Comment on attachment 8594563 [details] [diff] [review]
Fixes based on Paolo Amadini's feedback

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

(In reply to Marcos Caceres [:marcosc] from comment #8)
> The motivation for me lodging this bug were that I naturally expected a
> "browser" to be returned. 

I'm curious about this expectation. Is it because of the function name, or for this using a Promise in general? I wouldn't expect functions to return their first argument normally.

Regardless of the original motivation, it's good that you worked on this so we now have tests!

> Sorry, I'm confused. Do you suggests not resolving with the browser object
> (i.e., not changing the current behavior)?

Yeah, this is what I'm saying.

> I couldn't get the above to work.

I've tested that "return BrowserTestUtils.browserLoaded(browser).then(() => browser);" actually works even if browserLoaded returns undefined.

However, you can further simplify the code without relying on the return value in this way:

  let browsers = [for (u of tabURLs) gBrowser.addTab(u).linkedBrowser];
  yield Promise.all((for (b of browsers) BrowserTestUtils.browserLoaded(b)));
  Assert.ok(browsers.every(isDOMLoaded),
            "Expected all promised browsers to have loaded.");

(Note that the double parentheses are required for the generator comprehension.)
Attachment #8594563 - Flags: feedback?(paolo.mozmail) → feedback+
No longer blocks: 1083410
Assignee: nobody → mcaceres
Summary: Promise of BrowserTestUtils.browserLoaded should resolve with browser → Promise of BrowserTestUtils.browserLoaded should resolve with browser?
Attached patch Fixes and renamed the file. (obsolete) — Splinter Review
Thanks for the feedback. I've made the changes as suggested (+ dropped resolving with a browser object)... comments below.  

(In reply to :Paolo Amadini from comment #9)
> Comment on attachment 8594563 [details] [diff] [review]
> Fixes based on Paolo Amadini's feedback
> Review of attachment 8594563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Marcos Caceres [:marcosc] from comment #8)
> > The motivation for me lodging this bug were that I naturally expected a
> > "browser" to be returned.
> 
> I'm curious about this expectation. Is it because of the function name, or for
> this using a Promise in general? I wouldn't expect functions to return their
> first argument normally.

Both. My primary motivation was not having to keep the a list of the browsers I loaded potentially in addition to the list unresolved promises. However, using array comprehension elegantly solves this, as you show.

> Regardless of the original motivation, it's good that you worked on this so we
> now have tests!

Np. 

> > Sorry, I'm confused. Do you suggests not resolving with the browser object
> > (i.e., not changing the current behavior)?
> 
> Yeah, this is what I'm saying.

Np. Understood. 

> > I couldn't get the above to work.
> 
> I've tested that "return BrowserTestUtils.browserLoaded(browser).then(() =>
> browser);" actually works even if browserLoaded returns undefined.

Ok, I was confused about removing resolving with the browser object. 

> However, you can further simplify the code without relying on the return value
> in this way:
> 
> let browsers = [for (u of tabURLs) gBrowser.addTab(u).linkedBrowser];
> yield Promise.all((for (b of browsers) BrowserTestUtils.browserLoaded(b)));
> Assert.ok(browsers.every(isDOMLoaded),
> "Expected all promised browsers to have loaded.");
> 
> (Note that the double parentheses are required for the generator
> comprehension.)

Neat! I've been reluctant to use array comprehension because it's somewhat non-standard still. Used your code.
Attachment #8594563 - Attachment is obsolete: true
Attachment #8596013 - Flags: review?(paolo.mozmail)
Comment on attachment 8596013 [details] [diff] [review]
Fixes and renamed the file.

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

Looks good! Thanks for working on this.

(In reply to Marcos Caceres [:marcosc] from comment #10)
> Neat! I've been reluctant to use array comprehension because it's somewhat
> non-standard still. Used your code.

Yeah, we recently switched away from our non-standard comprehension syntax but I believe the new one here is pretty stable now.
Attachment #8596013 - Flags: review?(paolo.mozmail) → review+
Summary: Promise of BrowserTestUtils.browserLoaded should resolve with browser? → Add a dedicated test for BrowserTestUtils.browserLoaded
Sent to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd9aa2cc100

Will set to check in needed if it comes back happy green.
Attachment #8596013 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3addfacc7c48
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1270704
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: