1276966 - Stop some tests from accidentally resetting browser.startup.page

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Our testing profile normally sets this pref to 0:

https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/testing/profiles/prefs_general.js#7

But we have some tests that set that pref, and then use clearUserPref, which resets the preference back to its default, which is 1.

We should probably not do that - we're adding inconsistency with how our tests function, since later tests might rely on browser.startup.page being the testing default of 0.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1276966 - Fix tests that were accidentally resetting browser.startup.page. r?mikedeboer

Review commit: https://reviewboard.mozilla.org/r/56600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56600/

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8758323 [details]
MozReview Request: Bug 1276966 - Fix tests that were accidentally resetting browser.startup.page. r?mikedeboer

https://reviewboard.mozilla.org/r/56600/#review53168

3

::: browser/components/sessionstore/test/browser_480893.js:21
(Diff revision 1)
>    let browser = tab.linkedBrowser;
> -  promiseBrowserLoaded(browser).then(() => {
> +  yield BrowserTestUtils.browserLoaded(browser, false, "about:sessionrestore");
> +
> -    let doc = browser.contentDocument;
> +  let doc = browser.contentDocument;
>  
> -    // click on the "Start New Session" button after about:sessionrestore is loaded
> +  // click on the "Close" button after about:sessionrestore is loaded

nit: while you're here, can you make it 'Click' and 'loaded.'?

::: browser/components/sessionstore/test/browser_480893.js:41
(Diff revision 1)
> -        // click on the "Start New Session" button after about:sessionrestore is loaded
> +  browser.loadURI("about:sessionrestore");
> +  yield BrowserTestUtils.browserLoaded(browser, false, "about:sessionrestore");
> +
> +  doc = browser.contentDocument;
> +
> +  // click on the "Close" button after about:sessionrestore is loaded

nit: same here...

::: browser/components/sessionstore/test/browser_480893.js:43
(Diff revision 1)
> +
> +  doc = browser.contentDocument;
> +
> +  // click on the "Close" button after about:sessionrestore is loaded
> -        doc.getElementById("errorCancel").click();
> +  doc.getElementById("errorCancel").click();
> -        promiseBrowserLoaded(browser).then(() => {
> +  yield BrowserTestUtils.browserLoaded(browser, false, homepage);

Isn't it friendlier to our infrastructure and our own sanity perhaps to not have a timeout determine a test failure - I mean, isn't there a more direct deterministic method that can tell us that the correct URL was loaded?

Perhaps using a ContentTask here isn't a bad idea after all, because then you can also circumvent using `browser.contentDocument` here, right?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/56600/#review53168

> nit: while you're here, can you make it 'Click' and 'loaded.'?

Sure, done.

> nit: same here...

Sure, done.

> Isn't it friendlier to our infrastructure and our own sanity perhaps to not have a timeout determine a test failure - I mean, isn't there a more direct deterministic method that can tell us that the correct URL was loaded?
> 
> Perhaps using a ContentTask here isn't a bad idea after all, because then you can also circumvent using `browser.contentDocument` here, right?

Okay, after talking with :mikedeboer, I'm going to change this to:

```JavaScript
yield BrowserTestUtils.browserLoaded(browser);
is(doc.URL, homepage, "loaded page is the homepage”);
```

To ensure that we're only checking for the first load of the browser.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8758323 [details]
MozReview Request: Bug 1276966 - Fix tests that were accidentally resetting browser.startup.page. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56600/diff/1-2/

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8758323 [details]
MozReview Request: Bug 1276966 - Fix tests that were accidentally resetting browser.startup.page. r?mikedeboer

https://reviewboard.mozilla.org/r/56600/#review53248

::: browser/components/sessionstore/test/browser_480893.js:48
(Diff revision 2)
>  
> -          is(doc.URL, homepage, "loaded page is the homepage");
> +  is(browser.currentURI.spec, homepage, "loaded page is the homepage");
>  
> -          // close tab, restore default values and finish the test
> -          gBrowser.removeTab(tab);
> -          gPrefService.clearUserPref("browser.startup.page");
> +  yield BrowserTestUtils.removeTab(tab);
> +
> +  Assert.ok(true, "We made it!");

I think it's already quite obvious that we're done here. If you'd still like to show a message, please change it to `info("We made it!");`

Comment 6

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/56600/#review53248

> I think it's already quite obvious that we're done here. If you'd still like to show a message, please change it to `info("We made it!");`

Ah - leftovers from when I didn't have the `is()`, since this test would fail if it didn't have any `is` or `ok` checks. I'll just remove this. Good eye!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8758323 [details]
MozReview Request: Bug 1276966 - Fix tests that were accidentally resetting browser.startup.page. r?mikedeboer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56600/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/827bd99aa998
Fix tests that were accidentally resetting browser.startup.page. r=mikedeboer

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/827bd99aa998