Closed Bug 1437850 Opened 4 years ago Closed 4 years ago

Enable browser_console_nsiconsolemessage.js in new frontend

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: nchevobbe, Assigned: miker)

References

(Blocks 1 open bug)

Details

(Whiteboard: [newconsole-mvp])

Attachments

(1 file)

No description provided.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Priority: -- → P2
Whiteboard: [newconsole-mvp]
Severity: normal → enhancement
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Comment on attachment 8950964 [details]
Bug 1437850 - Enable browser_console_nsiconsolemessage.js in new frontend

https://reviewboard.mozilla.org/r/220226/#review226104

if the new frontend is enabled for the browser console, I expect this test to fail.
Could you check the new frontend is indeed enabled ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:12
(Diff revision 1)
> -const TEST_URI = "data:text/html;charset=utf8,<title>bug859756</title>\n" +
> +const TEST_URI = "data:text/html;charset=utf8," +
> +                 "<title>browser_console_nsiconsolemessage.js</title>\n" +
>                   "<p>hello world\n<p>nsIConsoleMessages ftw!";

nit: use a template literal so we don't have to care about new lines.

```js
const TEST_URI = `data:text/html;charset=utf8,
  <title>browser_console_nsiconsolemessage.js</title>
  <p>hello world<p>
  nsIConsoleMessages ftw!`;
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:31
(Diff revision 1)
>  
> -    ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
> +  ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
> -      content.console.log("foobarz");
> +    content.console.log("foobarz");
> -    });
> +  });

it seems like we do this to have a signal that we can check for a given message to not be displayed. Could we add a comment to explain this ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:32
(Diff revision 1)
> -    ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
> +  ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
> -      content.console.log("foobarz");
> +    content.console.log("foobarz");
> -    });
> +  });
>  
> -    yield waitForMessages({
> +  await waitFor(() => findMessage(hud, "foobarz"));

here we usually use waitForMessage before spawning the log since it relies on an event being emitted and is a bit better than polling (IMHO).

Also, we don't need the ContentTask callback to be a generator, and I like to pass the message we are going to log as a parameter instead of repeating the string (but no big deal)


```js
  const text = "foobarz";
  const onFooBarzMessage = waitForMessage(hud, text)
  ContentTask.spawn(gBrowser.selectedBrowser, text, function (msg) {
    content.console.log(msg);
  });
  const message = await onFooBarzMessage;
  ok(true, `"${text}" log is displayed as expected`);

```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:39
(Diff revision 1)
> -    is(text.indexOf("do-not-show-me"), -1,
> +  is(text.indexOf("do-not-show-me"), -1,
> -       "nsIConsoleMessages are not displayed");
> +      "nsIConsoleMessages are not displayed");
> -    is(text.indexOf("test1 for bug859756"), -1,
> +  is(text.indexOf("test1"), -1,
> -       "nsIConsoleMessages are not displayed (confirmed)");
> +      "nsIConsoleMessages are not displayed (confirmed)");

nit: could we switch to includes instead of indexOf ?
```js
ok(!text.includes("do-not-show-me"), "nsIConsoleMessages are not displayed");
```

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:55
(Diff revision 1)
> -        text: "do-not-show-me",
> -        category: CATEGORY_JS,
> -      }],
> -    });
>  
> -    let msg = [...results[2].matched][0];
> +  let msg = await waitFor(() => findMessage(hud, "do-not-show-me"));

nit: could we change the text to be more descriptive ? here we are waiting for a message that says do not show me to appear, and we expect it to be visible, it's confusing.
Maybe turn it into "don't show me on webconsole" ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:57
(Diff revision 1)
> -    isnot(msg.textContent.indexOf("do-not-show"), -1,
> +  isnot(msg.textContent.indexOf("do-not-show"), -1,
> -          "element content is correct");
> +        "element content is correct");

ditto about using includes + ok (double negation is really hard to parse)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:59
(Diff revision 1)
>  
> -    let msg = [...results[2].matched][0];
> +  let msg = await waitFor(() => findMessage(hud, "do-not-show-me"));
> -    ok(msg, "message element for do-not-show-me (nsIConsoleMessage)");
> +  ok(msg, "message element for do-not-show-me (nsIConsoleMessage)");
> -    isnot(msg.textContent.indexOf("do-not-show"), -1,
> +  isnot(msg.textContent.indexOf("do-not-show"), -1,
> -          "element content is correct");
> +        "element content is correct");
> -    ok(!msg.classList.contains("filtered-by-type"), "element is not filtered");
> +  ok(!msg.classList.contains("filtered-by-type"), "element is not filtered");

this is not how message are hidden anymore, we can remove this line I think

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:61
(Diff revision 1)
> -    hud.setFilterState("jslog", false);
> +  hud.setFilterState("jslog", false);
>  
> -    ok(msg.classList.contains("filtered-by-type"), "element is filtered");
> +  ok(msg.classList.contains("filtered-by-type"), "element is filtered");

so, I expect this to be testing the filtering on the browser console, but this is not done like that anymore (and I'm surprised it did not failed your test, do you have the new frontend enabled in the browser console ?).

We should: 
 - add comments to be explicit about what we are doing
 - toggle the filter off by clicking on the UI
 - check that the test is not visible by calling findMessage and expecting it to return null.
Attachment #8950964 - Flags: review?(nchevobbe) → review-
Comment on attachment 8950964 [details]
Bug 1437850 - Enable browser_console_nsiconsolemessage.js in new frontend

https://reviewboard.mozilla.org/r/220226/#review228548

Thanks a lot Mike, the test looks much better :)

I have a handful of (mostly) nit comments, but nothing serious.
I applied the patch locally, and run the test with --verify and it fails (on the second iteration I think). Could you check if it does too on your machine ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:19
(Diff revision 2)
> -
> -function test() {
> -  const FILTER_PREF = "devtools.browserconsole.filter.jslog";
> -  Services.prefs.setBoolPref(FILTER_PREF, true);
> -
> -  registerCleanupFunction(() => {
> +<title>browser_console_nsiconsolemessage.js</title>
> +<p>hello world<p>
> +nsIConsoleMessages ftw!`;
> +
> +add_task(async function () {
> +  await pushPref("devtools.browserconsole.filter.jslog", true);

We don't have the "jslog" filter anymore (see https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/devtools/client/preferences/devtools.js#258-266)

I think we can remove this line, here the filters should be in their default state, showing logs

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:26
(Diff revision 2)
> -    Services.console.logStringMessage("test1 for bug859756");
> +  Services.console.logStringMessage("test1");
>  
> -    info("open web console");
> +  info("open web console");
> -    let hud = yield openConsole(tab);
> +  let hud = await openConsole();
>  
> -    ok(hud, "web console opened");
> +  ok(hud, "web console opened");
> -    Services.console.logStringMessage("do-not-show-me");
>  
> -    ContentTask.spawn(gBrowser.selectedBrowser, null, function* () {
> -      content.console.log("foobarz");
> +  // This "do-not-show-me-in-web-console" message should not be displayed.
> +  Services.console.logStringMessage("do-not-show-me-in-web-console");

So here, I think we log 2 messages, one before the console is open, and one after, I guess in order to test that both cached and live messages are not displayed in the webconsole.

Maybe we could have more consistent naming, like "cachedBrowserConsoleMessage" and "liveBrowserConsoleMessage" so it's more explicit what those messages are about.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:44
(Diff revision 2)
> -        category: CATEGORY_WEBDEV,
> -        severity: SEVERITY_LOG,
> +  ContentTask.spawn(gBrowser.selectedBrowser, text, function (msg) {
> +    content.console.log(msg);
> -      }],
> -    });
> +  });
> +  await onFooBarzMessage;
> +  ok(true, `"${text}" log is displayed as expected`);

nit: say that it is displayed in the webconsole maybe ? so we avoid confusion

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:52
(Diff revision 2)
> -    is(text.indexOf("do-not-show-me"), -1,
> +  // displayed.
> +  text = hud.outputNode.textContent;
> +  ok(!text.includes("do-not-show-me-in-web-console"),
> -       "nsIConsoleMessages are not displayed");
> +    "nsIConsoleMessages are not displayed");
> -    is(text.indexOf("test1 for bug859756"), -1,
> +  ok(!text.includes("test1"),
> -       "nsIConsoleMessages are not displayed (confirmed)");
> +    "nsIConsoleMessages are not displayed (confirmed)");

We should say that cached nsIConsoleMessages are not displayed, and remove the "(confirmed)" part which does not speak for itself

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:66
(Diff revision 2)
> +  ok(msg.textContent.includes("do-not-show-me-in-web-console"),
> +    "do-not-show-me-in-web-console is displayed in browser console");

we could remove this line, since it's basically what findMessage does

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:71
(Diff revision 2)
> +  const toolbar = await waitFor(() => {
> +    return outputNode.querySelector(".webconsole-filterbar-primary");
> +  });

is there a reason we use waitFor here ? Since we already checked messages, I'd expect the toolbar to be rendered already

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:75
(Diff revision 2)
> -    let results = yield waitForMessages({
> -      webconsole: hud,
> +  // Test that filtering is working by hiding log messages... show the filter
> +  // bar.

maybe "show the filter bar" should be in its own line ?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_console_nsiconsolemessage.js:87
(Diff revision 2)
>  
> -    let msg = [...results[2].matched][0];
> -    ok(msg, "message element for do-not-show-me (nsIConsoleMessage)");
> -    isnot(msg.textContent.indexOf("do-not-show"), -1,
> -          "element content is correct");
> -    ok(!msg.classList.contains("filtered-by-type"), "element is not filtered");
> +  // Check that messages are not shown when their filter is turned off.
> +  filterBar.querySelector(".log").click();
> +
> +  // And then checking that log messages are hidden.
> +  await waitFor(() => findMessages(hud, "test1").length === 0);

should we assert that test2 and "do-not-show-me" are hidden too ?
Attachment #8950964 - Flags: review?(nchevobbe) → review-
Comment on attachment 8950964 [details]
Bug 1437850 - Enable browser_console_nsiconsolemessage.js in new frontend

https://reviewboard.mozilla.org/r/220226/#review228968

Looks good, thanks Mike
Attachment #8950964 - Flags: review?(nchevobbe) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddde09c787dc
Enable browser_console_nsiconsolemessage.js in new frontend r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/ddde09c787dc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.