Replace waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: johannh, Assigned: meghanagupta3, Mentored)

Tracking

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
The waitForCertErrorLoad[0] function is used in browser_captivePortal_certErrorUI.js[1] to wait until cert errors are loaded. We should replace it with the more reliable BrowserTestUtils.waitForErrorPage[2] function and make sure the affected tests still run correctly.

[0] http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/browser/base/content/test/captivePortal/head.js#173
[1] http://searchfox.org/mozilla-central/source/browser/base/content/test/captivePortal/browser_captivePortal_certErrorUI.js
[2] http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#689

You can run the test with

./mach mochitest browser/base/content/test/captivePortal/browser_captivePortal_certErrorUI.js

and learn more about Mochitest here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest
(Assignee)

Comment 1

2 years ago
Created attachment 8846582 [details] [diff] [review]
changes in browser_CaptivePortal_certErrorUI.js

Replaced waitForCertErrorLoad with BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Tests failing after changes.
Flags: needinfo?(jhofmann)
Attachment #8846582 - Flags: review?(jhofmann)
(Reporter)

Updated

2 years ago
Assignee: nobody → meghanagupta3
Status: NEW → ASSIGNED
Flags: needinfo?(jhofmann)
(Reporter)

Comment 2

2 years ago
After chatting with Nihanth about this it turns out that captive portal pages don't emit the event that BrowserTestUtils.waitForErrorPage is listening to (AboutNetErrorLoad).

So only the second waitForCertErrorLoad can be replaced by BrowserTestUtils.waitForErrorPage in that file.

You should probably still change the first call in line 31 to BrowserTestUtils.waitForContentEvent(browser, "DOMContentLoaded") so that we can get rid of the shared function in head.js

Let me know if anything is unclear :)

Thanks!
(Assignee)

Comment 3

2 years ago
Created attachment 8846629 [details] [diff] [review]
Replaced function instances and deleted obsolete function from main.js

Bug 1344726 - Replaced instances of waitForCertErrorLoad with BrowserTestUtils.waitForContentEvent and BrowserTestUtils.waitForErrorPage in browser_captivePortal_certErrorUI.js. Removed waitForCertErrorLoad function in head.js
Attachment #8846582 - Attachment is obsolete: true
Attachment #8846582 - Flags: review?(jhofmann)
Attachment #8846629 - Flags: review?(jhofmann)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8846629 [details] [diff] [review]
Replaced function instances and deleted obsolete function from main.js

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

Looks great, thank you!

Now this needs a run on our try server, to check if all tests are still green. (https://wiki.mozilla.org/ReleaseEngineering/TryServer). I just did that for you, but in the future you might want to get your own try access. I've also set the checkin-needed flag. This flag signals that we want the patch merged. If the try looks good, someone will come around and check it in for us.
Attachment #8846629 - Flags: review?(jhofmann) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e35e5f2b9dfb5539598e67e3b08502e111be353
Bug 1344726 - Replace instances of waitForCertErrorLoad with BTU.waitForContentEvent and BTU.waitForErrorPage in browser_captivePortal_certErrorUI.js. r=johannh
(Reporter)

Comment 7

2 years ago
Fixed the newline at the end of head.js and pushed to inbound.
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e35e5f2b9df
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1347602
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.