Closed Bug 1407178 Opened 3 years ago Closed 2 years ago

Enable browser_webconsole_certificate_messages.js in the new console

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 59

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Comment on attachment 8916914 [details]
Bug 1407178 - Enable browser_webconsole_certificate_messages.js in the new console; .

https://reviewboard.mozilla.org/r/187948/#review193554

Looks good!

R+ (green on Win10)

Honza
Attachment #8916914 - Flags: review?(odvarko) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f3dd84d4d22
Enable browser_webconsole_certificate_messages.js in the new console; r=Honza.
I can reproduce.
The thing is, the original test we copied fails to for `./mach test devtools/client/webconsole/test/browser_webconsole_certificate_messages.js --verify`

I think this has to do with how we don't want to show security messages again if you reload the page.
Should I find a way to make the test green when using test-verify, or is there some way to bypass test-verify since it hits a feature ?
Flags: needinfo?(nchevobbe) → needinfo?(aryx.bugmail)
Geoff, can you answer the questions in comment 6, please?

From the sheriffing point of view, having the test running in test-verify without special behavior would be the best. Is it possible to run a cleanup function after the test like clearing the cache so the issue goes away?
Flags: needinfo?(aryx.bugmail) → needinfo?(gbrown)
I can't think of a way to bypass test-verify (other than disabling the test, obviously not what you want here!).

It would be best to find a way to make the test green with test-verify, otherwise it will fail TV the next time the test is modified, and we'll have another conversation like this. :)

It looks like your test is failing test-verify on the first step, which runs the test with --repeat. That will cause trouble for tests that rely on an initial state that is not restored at the end of the test: Your security message hypothesis sounds right. It is tempting to argue, "just don't run the test with --repeat", but it leaves your test vulnerable to the possibility that someone will add a new test, run earlier in the manifest, that changes that state and causes your test to fail. Better to "fix" your test.

That said, I think the argument "This test failed verify before, and I think I am improving the test" is a reasonable argument for proceeding with landing with test-verify failing, if you really want to.
Flags: needinfo?(gbrown)
Comment on attachment 8916914 [details]
Bug 1407178 - Enable browser_webconsole_certificate_messages.js in the new console; .

Clearing the review flag since things changed a bit since the backout
Attachment #8916914 - Flags: review+ → review?(odvarko)
Comment on attachment 8916914 [details]
Bug 1407178 - Enable browser_webconsole_certificate_messages.js in the new console; .

https://reviewboard.mozilla.org/r/187948/#review203726

Looks reasonable
Honza
Attachment #8916914 - Flags: review?(odvarko) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4619d49c91f5
Enable browser_webconsole_certificate_messages.js in the new console; r=Honza.
https://hg.mozilla.org/mozilla-central/rev/4619d49c91f5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Mass won't fix for 58. Let it ride the train.
Duplicate of this bug: 1408925
Product: Firefox → DevTools
Duplicate of this bug: 1243990
You need to log in before you can comment on or make changes to this bug.