Closed Bug 1404853 Opened 2 years ago Closed 2 years ago

Migrate browser_webconsole_bug_653531_highlighter_console_helper.js to the new frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 59

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

the test is always skipped on ci at the moment (because it requires direct access to content).
We should take this migration as an opportunity to rewrite the test in a way that does not require direct access
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8939576 [details]
Bug 1404853 - Enable browser_webconsole_highlighter_console_helper.js in new console frontend; .

https://reviewboard.mozilla.org/r/209888/#review215800

Looks good, just couple of nits.

Honza

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_highlighter_console_helper.js:56
(Diff revision 1)
> -  }
>  
> -  function onNodeUpdate(node) {
> -    isnot(node.textContent.indexOf("bug653531"), -1,
> -          "correct output for $0.textContent");
> -    is(inspector.selection.node.textContent, "bug653531",
> +  let onEvaluationResult = waitForMessage(hud, "<h1>");
> +  jsterm.execute("$0");
> +  await onEvaluationResult;
> +  ok(true, "correct output for $0");

Why not using just `info` API?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_highlighter_console_helper.js:65
(Diff revision 1)
> -  gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> -  gBrowser.selectedBrowser.addEventListener("load", function () {
> -    waitForFocus(createDocument, content);
> -  }, {capture: true, once: true});
> +  const newH1Content = "newH1Content";
> +  let onAssignmentResult = waitForMessage(hud, "<h1>");
> +  jsterm.execute(`$0.textContent = "${newH1Content}";$0`);
> +  await onAssignmentResult;
> +
> +  ok(true, "correct output for $0 after setting $0.textContent");

The same as above.
Attachment #8939576 - Flags: review?(odvarko) → review+
Comment on attachment 8939576 [details]
Bug 1404853 - Enable browser_webconsole_highlighter_console_helper.js in new console frontend; .

https://reviewboard.mozilla.org/r/209888/#review215800

> Why not using just `info` API?

because if we make it to this point this means the test case is correct (PASS).
I think I'm used to this because you can't have a test without assertions, and here, even if we don't assert a specific value, we assert that we made it to this point.
Does that make sense ? I don't feel too strong about it, so I'm also okay with changing it.
Flags: needinfo?(odvarko)
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> because if we make it to this point this means the test case is correct
> (PASS).
> I think I'm used to this because you can't have a test without assertions,
> and here, even if we don't assert a specific value, we assert that we made
> it to this point.
I see, sounds reasonable. Let's keep it as is.
Thanks,
Honza
Flags: needinfo?(odvarko)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e14fbde53491
Enable browser_webconsole_highlighter_console_helper.js in new console frontend; r=Honza.
https://hg.mozilla.org/mozilla-central/rev/e14fbde53491
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
It's too late for 58. Mark 58 won't fix.
Product: Firefox → DevTools
Duplicate of this bug: 1243973
You need to log in before you can comment on or make changes to this bug.