Move tests related to site identity from test/general to test/siteIdentity

RESOLVED FIXED in Firefox 54

Status

()

Firefox
General
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: johannh, Assigned: johannh)

Tracking

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

9 months ago
The new siteIdentity directory feels cold and empty and it would be immensely useful if we could run all site identity tests locally in reasonable time. (Not to mention the cleanups)
Comment hidden (mozreview-request)
(Assignee)

Comment 2

9 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c2f5b4beac
(Assignee)

Comment 3

9 months ago
This is an initial patch that moves some obvious choices like the mixed content blocking tests. There are definitely more, but I'd like to do this step by step to avoid huge rebase conflicts and too much chaos on try.
Comment on attachment 8832070 [details]
Bug 1335431 - Move site identity tests out of test/general.

https://reviewboard.mozilla.org/r/108468/#review109810

Congratulations, you found tests that depend on other tests... the trypush shows other mcb tests that are failing now. They probably want moving too, but ideally they should also be fixed so they're not, you know, broken if you change the order in which tests are run. :-\

Going to hold off reviewing until that's fixed.
Attachment #8832070 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8832070 [details]
Bug 1335431 - Move site identity tests out of test/general.

https://reviewboard.mozilla.org/r/108468/#review109812

Eh, changed my mind and had a look anyway. This generally looks very fine.

::: browser/base/content/test/siteIdentity/browser_bug1045809.js:23
(Diff revision 1)
> -    "https://test1.example.com/browser/browser/base/content/test/general/" +
> -    "file_bug1045809_1.html";
>    let tab = gBrowser.selectedTab = gBrowser.addTab();
>  
>    // Test 1: mixed content must be blocked
> -  yield promiseTabLoadEvent(tab, url);
> +  yield promiseTabLoadEvent(tab, URL);

Nit: please make this not overwrite the DOM global "URL" (so the TEST_URL you use elsewhere would be a better alternative, IMO)

::: browser/base/content/test/siteIdentity/browser_mcb_redirect.js:306
(Diff revision 1)
>    origBlockActive = Services.prefs.getBoolPref(PREF_ACTIVE);
>    origBlockDisplay = Services.prefs.getBoolPref(PREF_DISPLAY);
>    Services.prefs.setBoolPref(PREF_ACTIVE, true);
>    Services.prefs.setBoolPref(PREF_DISPLAY, true);
>  
> -  pushPrefs(["dom.ipc.processCount", 1]).then(() => {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.ipc.processCount", 1]]}, () => {

Can we add a comment about why this is necessary (I'm assuming you can find out from `hg annotate` or similar)

::: browser/base/content/test/siteIdentity/head.js:89
(Diff revision 1)
> +
> +  return loaded;
> +}
> +
> +// Compares the security state of the page with what is expected
> +function isSecurityState(browser, expectedState) {

I'm assuming this and the lower-down function are basically identical copies of the ones you removed in general/head.js , but I haven't checked. :-)
(Assignee)

Comment 6

9 months ago
Thanks so far! I'll fix these try failures soon. At least I wrote that test myself, so we know who to blame.

> I'm assuming this and the lower-down function are basically identical copies of the ones you removed in general/head.js , but I haven't checked. :-)

They are, though I changed some very minor details, e.g. added a browser argument to isSecurityState and replaced is_element_visible with ok(is_visible(..)).
Comment hidden (mozreview-request)
Comment on attachment 8832070 [details]
Bug 1335431 - Move site identity tests out of test/general.

https://reviewboard.mozilla.org/r/108468/#review110264

LGTM assuming try is happy :-)
Attachment #8832070 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)

Comment 10

9 months ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f3ed1d713b5
Move site identity tests out of test/general. r=Gijs

Comment 11

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f3ed1d713b5
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.