Closed Bug 1408946 Opened 2 years ago Closed 2 years ago

Enable browser_webconsole_output_order.js in the new console frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: nchevobbe, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

No description provided.
Blocks: 1400847
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8931422 [details]
Bug 1408946 - enable browser_webconsole_output_order.js;

https://reviewboard.mozilla.org/r/202564/#review208108

Thanks Julian, I'm concerned about a race condition here, see my comments

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_output_order.js:18
(Diff revision 1)
> -  let jsterm = hud.jsterm;
> -
> -  jsterm.clearOutput();
> -  jsterm.execute("console.log('foo', 'bar');");
> -
> -  let [functionCall, consoleMessage, result] = yield waitForMessages({
> +  hud.jsterm.execute("console.log('foo', 'bar');");
> +
> +  let messages = ["console.log('foo', 'bar');", "foo bar", "undefined"];
> +  const [functionCall, consoleMessage, result] = await waitForMessages({
> +    hud,
> +    messages: messages.map(text => ({text}))
> -    webconsole: hud,
> -    messages: [{
> -      text: "console.log('foo', 'bar');",
> -      category: CATEGORY_INPUT,
> -    },
> -      {
> -        text: "foo bar",
> -        category: CATEGORY_WEBDEV,
> -        severity: SEVERITY_LOG,
> -      },
> -      {
> -        text: "undefined",
> -        category: CATEGORY_OUTPUT,
> -      }]
>    });

I think we open the door to intermittence here : waitForMessages is now listening for an event to be fired, and I guess if the jsterm execution is fast enough, we could miss it ? Unlinkely, but I'd still do : 

let messages = ["console.log('foo', 'bar');", "foo bar", "undefined"];
const onMessages = await waitForMessages({
    hud,
    messages: messages.map(text => ({text}))
});

hud.jsterm.execute("console.log('foo', 'bar');");
const [functionCall, consoleMessage, result] = await  onMessages;

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_output_order.js:26
(Diff revision 1)
> -  let fncallNode = [...functionCall.matched][0];
> -  let consoleMessageNode = [...consoleMessage.matched][0];
> -  let resultNode = [...result.matched][0];
> +  let fncallNode = functionCall.node.closest(".message");
> +  let consoleMessageNode = consoleMessage.node.closest(".message");
> +  let resultNode = result.node.closest(".message");

doesn't {waitForMessageResult}.node give us the .message element ?
Attachment #8931422 - Flags: review?(nchevobbe) → review-
Comment on attachment 8931422 [details]
Bug 1408946 - enable browser_webconsole_output_order.js;

https://reviewboard.mozilla.org/r/202564/#review208108

> I think we open the door to intermittence here : waitForMessages is now listening for an event to be fired, and I guess if the jsterm execution is fast enough, we could miss it ? Unlinkely, but I'd still do : 
> 
> let messages = ["console.log('foo', 'bar');", "foo bar", "undefined"];
> const onMessages = await waitForMessages({
>     hud,
>     messages: messages.map(text => ({text}))
> });
> 
> hud.jsterm.execute("console.log('foo', 'bar');");
> const [functionCall, consoleMessage, result] = await  onMessages;

Yep good point.

> doesn't {waitForMessageResult}.node give us the .message element ?

it does, not sure why I thought otherwise.
Comment on attachment 8931422 [details]
Bug 1408946 - enable browser_webconsole_output_order.js;

https://reviewboard.mozilla.org/r/202564/#review208394

Thanks for the changes Julian, let's land this

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_output_order.js:26
(Diff revision 2)
> -  let resultNode = [...result.matched][0];
> +  const [functionCall, consoleMessage, result] = await onMessages;
> +
> +  let fncallNode = functionCall.node;
> +  let consoleMessageNode = consoleMessage.node;
> +  let resultNode = result.node;

nit: we could clean this into 

const [fncallNode, consoleMessageNode, resultNode] = (await onMessages).map(msg => msg.node);
Attachment #8931422 - Flags: review?(nchevobbe) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd5a8a6bb54e
enable browser_webconsole_output_order.js;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/dd5a8a6bb54e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.