Closed Bug 1335431 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: johannh, Assigned: johannh)

Details

Attachments

(1 file)

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)
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. :-)
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 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+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f3ed1d713b5
Move site identity tests out of test/general. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/8f3ed1d713b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: