1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

Review commit: https://reviewboard.mozilla.org/r/219120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219120/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8949812 [details]
Bug 1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

https://reviewboard.mozilla.org/r/219120/#review224886

Thanks for picking this one up Mike! A few comments to address before landing.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser.ini
(Diff revision 1)
>  skip-if = true #	Bug 1404877
>  [browser_webconsole_timestamps.js]
>  [browser_webconsole_trackingprotection_errors.js]
>  tags = trackingprotection
> -skip-if = true #	Bug 1405650
> -# old console skip-if = (os == 'win' && bits == 64) # Bug 1390001

The old console skip-ifs (here windows 64bits) need to be verified before we remove them. 

Can you start a try run for windows 64 to check that the test now passes?

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:12
(Diff revision 1)
>  // 'learn more' link shows up in the webconsole.
>  
>  "use strict";
>  
>  const TEST_URI = "http://tracking.example.org/browser/devtools/client/" +
>                   "webconsole/test/test-trackingprotection-securityerrors.html";

this URI needs to point the support file located in the new web console test folder:

const TEST_URI = "http://tracking.example.org/browser/devtools/client/webconsole/" +
  "new-console-output/test/mochitest/test-trackingprotection-securityerrors.html";

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:29
(Diff revision 1)
> -  yield UrlClassifierTestUtils.addTestTrackers();
> +  info("Clicking on the Learn More link");
> -  Services.prefs.setBoolPref(PREF, true);
>  
> -  let { browser } = yield loadTab(TEST_URI);
> +  const learnMoreLink = match.querySelector(".learn-more-link");
> +  let linkSimulation = await simulateLinkClick(learnMoreLink);
> +  await checkLink({

checkLink should not be async

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:36
(Diff revision 1)
> +    expectedLink: LEARN_MORE_URI,
> +    expectedTab: "tab"
> +  });
> +}
>  
> -  let hud = yield openConsole();
> +async function checkLink({ link, where, expectedLink, expectedTab }) {

this method is not async

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:41
(Diff revision 1)
> -  let hud = yield openConsole();
> +async function checkLink({ link, where, expectedLink, expectedTab }) {
> +  is(link, expectedLink, `Clicking the provided link opens ${link}`);
> +  is(where, expectedTab, `Clicking the provided link opens in expected tab`);
> +}
>  
> -  let results = yield waitForMessages({
> +add_task(async function testMessagesAppear() {

As a convention we usually have the add_task at the "top" and dedicated test functions or helpers below. 

In this case it means moving it below the registerCleanupFunction at L22.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:43
(Diff revision 1)
> +  is(where, expectedTab, `Clicking the provided link opens in expected tab`);
> +}
>  
> -  let results = yield waitForMessages({
> -    webconsole: hud,
> -    messages: [
> +add_task(async function testMessagesAppear() {
> +  await UrlClassifierTestUtils.addTestTrackers();
> +  Services.prefs.setBoolPref(PREF, true);

Could we use the pushPref util from shared-head?

  await pushPref(PREF, true);
  
pushPref takes care of the cleanup automatically, so you can remove "Services.prefs.clearUserPref(PREF);" from the cleanup method.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_trackingprotection_errors.js:47
(Diff revision 1)
> -  });
>  
> -  yield testClickOpenNewTab(hud, results[0]);
> +  let hud = await openNewTabAndConsole(TEST_URI);
> -});
>  
> -function testClickOpenNewTab(hud, match) {
> +  let results = await waitFor(() => findMessage(hud,

findMessage only returns a single message, so rename results to result or message?

::: devtools/client/webconsole/test/browser.ini
(Diff revision 1)
>  [browser_webconsole_scratchpad_panel_link.js]
>  [browser_webconsole_split.js]
>  [browser_webconsole_split_escape_key.js]
>  [browser_webconsole_split_focus.js]
>  [browser_webconsole_split_persist.js]
> -[browser_webconsole_trackingprotection_errors.js]

As mentioned on slack we are keeping the old tests for now.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8949812 [details]
Bug 1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219120/diff/1-2/

Comment 4

[Julian Descottes [:jdescottes]]

try run with mochitest dt for win64:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f43a92b2b7ba9581386a3ef271503aac0e68d22

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8949812 [details]
Bug 1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

https://reviewboard.mozilla.org/r/219120/#review224886

> The old console skip-ifs (here windows 64bits) need to be verified before we remove them. 
> 
> Can you start a try run for windows 64 to check that the test now passes?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f4dce9466f&group_state=expanded

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try on win64 is green and your comments addressed. Can you re-review?

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8949812 [details]
Bug 1405650 - Migrate browser_webconsole_trackingprotection_errors.js to the new frontend

https://reviewboard.mozilla.org/r/219120/#review224980

Looks good, thanks for addressing the comments! 
Glad that it's working on all platforms now, one leff skip-if :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5212d4f7b2b9
Migrate browser_webconsole_trackingprotection_errors.js to the new frontend r=jdescottes

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5212d4f7b2b9