Closed Bug 1403454 Opened 2 years ago Closed 2 years ago

Migrate browser_webconsole_script_errordoc_urls.js to the 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.
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [newconsole-mvp]
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
OS: Unspecified → All
Priority: P2 → P1
Hardware: Unspecified → All
Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225572

Thanks! Test will fail on non-e10s (which we still run on some platforms), plus some comments, let's go for another round of review before landing.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:30
(Diff revision 1)
> +    expected: 'TypeError: "score" is read-only',
>    },
>    {
>      jsmsg: "JSMSG_STMT_AFTER_RETURN",
>      script: "function a() { return; 1 + 1; };",
>      isException: false,

The "isException" field was used to "expectUncaughtException" which is still needed when the test runs in non-e10s.

We usually handle this as follows:

  let isE10s = Services.appinfo.browserTabsRemoteAutostart; 
  if (!isE10s) {
    expectUncaughtException();
  }
  
Here with the added isException we would have:

  if (testData.isException && !isE10s) {
    expectUncaughtException();
  }
  
(to be added before loading the new document)

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:35
(Diff revision 1)
>      isException: false,
> +    expected: "unreachable code after return statement",
>    }
>  ];
>  
> -add_task(function* () {
> +add_task(async function testErrorDocs() {

nit: main test functions are usually not named.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:44
(Diff revision 1)
> -  if (testData.isException === true) {
> -    expectUncaughtException();
> +  let browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.loadURI(browser, makeURIData(testData.script));

can use 
  await loadDocument(makeURIData(testData.script))

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:46
(Diff revision 1)
>  
> -function* testScriptError(hud, testData) {
> -  if (testData.isException === true) {
> -    expectUncaughtException();
> +async function testScriptError(hud, testData) {
> +  let browser = gBrowser.selectedBrowser;
> +  BrowserTestUtils.loadURI(browser, makeURIData(testData.script));
> -  }
>  

Can we have an info here before doing the await (in case this part times out it will make log reading easier).

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:60
(Diff revision 1)
>  
> -  let hrefs = {};
> -  for (let link of hud.jsterm.outputNode.querySelectorAll("a")) {
> -    hrefs[link.href] = true;
> +  // Gather all URLs displayed in the console. [Learn More] links have no href
> +  // but have the URL in the title attribute.
> +  let hrefs = new Set();
> +  for (let link of hud.ui.outputNode.querySelectorAll("a")) {
> +    hrefs.add(link.href || link.title);

According to your comment above, link.href is always falsy here, why keep it?
Attachment #8950543 - Flags: review?(jdescottes)
Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225598

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_script_errordoc_urls.js:60
(Diff revision 1)
>  
> -  let hrefs = {};
> -  for (let link of hud.jsterm.outputNode.querySelectorAll("a")) {
> -    hrefs[link.href] = true;
> +  // Gather all URLs displayed in the console. [Learn More] links have no href
> +  // but have the URL in the title attribute.
> +  let hrefs = new Set();
> +  for (let link of hud.ui.outputNode.querySelectorAll("a")) {
> +    hrefs.add(link.href || link.title);

You are right, the links we are interested in here don't have hrefs (although there are other links that do). Removed condition.
Comment on attachment 8950543 [details]
Bug 1403454 - Migrate browser_webconsole_script_errordoc_urls.js to the new frontend

https://reviewboard.mozilla.org/r/219822/#review225660

Looks good Mike, thanks!
Attachment #8950543 - Flags: review?(jdescottes) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17454f8c2ea8
Migrate browser_webconsole_script_errordoc_urls.js to the new frontend r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/17454f8c2ea8
Status: ASSIGNED → RESOLVED
Closed: 2 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.