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)
Firefox
General
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4c2f5b4beac
Assignee | ||
Comment 3•7 years 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 4•7 years ago
|
||
mozreview-review |
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 5•7 years ago
|
||
mozreview-review |
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•7 years 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 8•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8f3ed1d713b5
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.
Description
•