Closed Bug 1405245 Opened 3 years ago Closed 2 years ago

Migrate browser_webconsole_bug_1247459_violation.js to the new frontend

Categories

(DevTools :: Console, defect, P1)

defect

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(2 files, 1 obsolete file)

No description provided.
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Might have surfaced a small bug in PageError.js, regarding the handling of repeats.

Try with test migrated and enabled for new console: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8fb2c24725e2d7f92e5b43fb3c26d1214e72cc1

Additional windows specific try run to see if we can enable the test on win e10s: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02666c73d3bf5020b523b06faccc7a61fdb172dd 
(see Bug 1264955 ?)
Priority: P2 → P1
(xpcshell failures on my try run are linked to a mass breakage -> Bug 1435644)
We can land Bug 1428520 first and then land the test migration afterwards if needed.
Depends on: 1428520
Comment on attachment 8948388 [details]
Bug 1405245 - enable test-csp-violation.html for new webconsole;

https://reviewboard.mozilla.org/r/217852/#review223856

::: devtools/client/webconsole/new-console-output/test/mochitest/head.js:125
(Diff revision 1)
> +              let repeatNode = newMessage.node.querySelector(".message-repeats");
> +              repeatMatch = repeatNode &&
> +                            (parseInt(repeatNode.textContent, 10) == repeat);

I think we could have a race here. 
We are waiting for a message to come, then check the repeat bubble right away.
But maybe the second similar message did not came in yet.
I think it could be better to wait until we do have a repeat node with the correct number (using waitFor maybe ?).

Also, I'm unsure about addind more options to waitForMessages. The old console had a lot of options to the point it was difficult to remember what was doing what. 
For our test, maybe we could split this in 2 : 1 call to waitForMessages, and another to a new waitForMessageRepeat for example, where we would wait for the repeat bubble.
Or, we could have waitForMessageRepeat wait for both the message and the repeat, but at least it is more explicit.

What do you think about this Julian ?
Comment on attachment 8948389 [details]
Bug 1405245 - enable test-csp-violation.html on windows e10s;

https://reviewboard.mozilla.org/r/217854/#review223858

Nice fix ! :)
Attachment #8948389 - Flags: review?(nchevobbe) → review+
Attachment #8948389 - Attachment is obsolete: true
Comment on attachment 8948388 [details]
Bug 1405245 - enable test-csp-violation.html for new webconsole;

https://reviewboard.mozilla.org/r/217852/#review223892

Great, thanks Julian
Attachment #8948388 - Flags: review?(nchevobbe) → review+
Comment on attachment 8948387 [details]
Bug 1405245 - webconsole PageError component read repeat from props;

https://reviewboard.mozilla.org/r/217850/#review225470

Let's land this
Attachment #8948387 - Flags: review?(nchevobbe) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37f69ad9559f
webconsole PageError component read repeat from props;r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f349f28b9689
enable test-csp-violation.html for new webconsole;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/37f69ad9559f
https://hg.mozilla.org/mozilla-central/rev/f349f28b9689
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
Duplicate of this bug: 1243978
Duplicate of this bug: 1428520
You need to log in before you can comment on or make changes to this bug.