1496082 - Enable ESLint for docshell/test/navigation and docshell/test/unit

Status: RESOLVED FIXED

(Core :: DOM: Navigation, enhancement, P5)

Description

[Mark Banner (:standard8)]

As part of rolling out ESLint across the tree, we should enable it for dom/notification/test/unit/

There's background on our eslint setups here:

https://developer.mozilla.org/docs/ESLint

Here's some approximate steps:

1) In .eslintignore, replace the `docshell/**` line with ones for ignoring the test directories other than `docshell/test/navigation` and `docshell/test/unit`
2) Run eslint:

./mach eslint --fix docshell

This should fix most of the issues.

3) Inspect the diff to make sure that the indentation of the lines surrounding the changes look ok.

4) Create a commit of the work so far, e.g. "Bug <bug number> - Enable ESLint for <path> (automatic fixes)."

5) For the remaining issues, you'll need to fix them by hand. You can read details about most of the rules here: https://eslint.org/docs/rules/

If you're not sure, feel free to ask here or on irc.

6) Create a second commit of the manual changes e.g. "Bug <bug number> - Enable ESLint for <path> (manual fixes)."

Push the resulting commits to phabricator:

https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Comment 1

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #0)
> As part of rolling out ESLint across the tree, we should enable it for
> dom/notification/test/unit/

That's meant to be enable it for docshell/test/navigation and docshell/test/unit.

Comment 2

[Toby Ward]

Attachment: WIP .eslintignore file

Hi,

I've started work on this bug and have made some changes to the .eslintignore file (notably lines 156-166) to allow for linting in the docshell/test/navigation and docshell/test/unit folders. I did run eslint after this, but the linter found no errors or warnings - I'd just like to double check that the ignore file has been configured correctly and that I haven't ignored too many files, as I'm concerned I might have had a false result.

Thanks,

Toby

Comment 3

[Mark Banner (:standard8)]

I took the .eslintignore and applied it to my current tree, and there were a surprising amount of differences - I was just expecting the docshell changes - are you sure you're on the latest mozilla-central?

In any case, applying that file to my tree and running ./mach eslint, I see 994 problems.

From the diff:

====

+ # docshell/ exclusions. Currently eslint is only in use for docshell/test/navigation and docshell/test/unit

Lets keep these in the top section just under the current "We ignore all these directories by default, until we get them enabled.". We're going to enable these soon hopefully, so we should keep them there.

+ docshell/base/**
+ docshell/build/**
+ docshell/resources/**
+ docshell/shistory/**

We shouldn't really need to list these as they don't have any js nor html files in.

+ docshell/test/unit_ipc/**

We should probably just do this one in this bug - there isn't much in it.

Comment 4

[Toby Ward]

So I've just done a hg pull in the command and it turns out there's quite a lot of changes that need to be pulled in - that's quite interesting as I had only started the MozillaBuild VM on the 4th, and the boot sequence did indicate that it was pulling the latest version, so I might look into that at a later date.

Either way, I'll make sure I get the changes pulled in and update the .eslintignore asap.

To be clear, do you want me to run eslint on the entire repo, or just the docshell folder, as I have been doing the latter?

Comment 5

[Mark Banner (:standard8)]

(In reply to Toby Ward from comment #4)
> To be clear, do you want me to run eslint on the entire repo, or just the
> docshell folder, as I have been doing the latter?

Just running on the docshell folder is fine, since that's all you're affecting here (it is also quicker).

Comment 6

[Toby Ward]

Attachment: WIP .eslintignore file

Comment 7

[Toby Ward]

Attachment: ESLint Error Message and Repository Version

So after a bit of wrangling I've managed to pull down the latest repository from mozilla-central (harder than it sounds due to a poor internet connection). I did have a quick look at the .eslintignore.orig file after getting the latest repository, and that was definitely the file I was working off originally, which explains why there were so many differences.

Unfortunately I've ran into a problem with ESLint failing and not returning any results. I haven't had too much time to take a look at the error, but it might be a problem with ESLint not knowing which version of ECMAScript to use. Some of the directories it was referring to were odd as they weren't in the directory I was linting, but I don't know if this because the test folders are linking back to other directories or not.

I've attached both the new .eslintignore file as well as a text copy of the error message I received. I've also attached the results of a hg summary at the end of the error message just to make sure I'm on the right version of the repository (although I have tried to pull again and not had any updates).

Comment 8

[Mark Banner (:standard8)]

Answered outside the bug - we'll work through Toby's eslint issues separately.

Comment 9

[Toby Ward]

Attachment: Bug 1496082: Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only). r=Standard8,r=bzbarsky

Enabled ESLint for:

* docshell/test/navigation/**
* docshell/test/unit/**
* docshell/test/unit_ipc/**

Changed .eslintignore to allow for this and ran ./mach eslint --fix on the above directories and checked automatic fixes

Comment 10

[Toby Ward]

Attachment: Bug 1496082: Enable ESLint for docshell/test/navigation and docshell/test/unit (manual fixes)

Implemented the manual linting fixes for docshell/test/navigation, docshell/test/unit and docshell/test/unit_ipc
Depends on D9430

Comment 11

[Mark Banner (:standard8)]

Toby, for some reason, the lander didn't wasn't able to successfully rebase these to be able to land them. Could you try pulling the latest code & rebase, and push updates for both revisions please.

We can then try again.

Comment 12

[Toby Ward]

(In reply to Mark Banner (:standard8) from comment #11)
> Toby, for some reason, the lander didn't wasn't able to successfully rebase
> these to be able to land them. Could you try pulling the latest code &
> rebase, and push updates for both revisions please.
> 
> We can then try again.

Hi Mark,

Just rebased and updated both diffs to the latest changesets (sorry about the spam on one of them, had a bit of an issue accidentally including the manual fixes on the automatic fixes diff).

Hope this helps,
Toby

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f87e383a807
Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only). r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/74780d0a4848
Enable ESLint for docshell/test/navigation and docshell/test/unit (manual fixes) r=bzbarsky

Comment 14

[Mark Banner (:standard8)]

Toby, thanks for the update. As it turns out, there was still some bitrot - I suspect the changes were still on autoland as you updated.

I therefore "commandeered" the revisions and updated the patches for you so that we could make sure to get this landed.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 2 changesets (Bug 1496082) for test_bug270414.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74780d0a4848ac873154e4ff95c3287c375b976b

Backout link: https://hg.mozilla.org/integration/autoland/rev/ea6dfa0f1fcbde785069aed0f4eec2ed5d3c39c4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=210095275&repo=autoland&lineNumber=3450

[task 2018-11-06T15:29:40.258Z] 15:29:40     INFO - TEST-START | docshell/test/navigation/test_bug270414.html
[task 2018-11-06T15:29:42.440Z] 15:29:42     INFO - TEST-INFO | started process screentopng
[task 2018-11-06T15:29:42.927Z] 15:29:42     INFO - TEST-INFO | screentopng: exit 0
[task 2018-11-06T15:29:42.929Z] 15:29:42     INFO - Buffered messages logged at 15:29:42
[task 2018-11-06T15:29:42.930Z] 15:29:42     INFO - TEST-PASS | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by setting location. 
[task 2018-11-06T15:29:42.931Z] 15:29:42     INFO - TEST-PASS | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by calling window.open. 
[task 2018-11-06T15:29:42.932Z] 15:29:42     INFO - TEST-PASS | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by submitting form. 
[task 2018-11-06T15:29:42.934Z] 15:29:42     INFO - Buffered messages finished
[task 2018-11-06T15:29:42.935Z] 15:29:42     INFO - TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by targeted hyperlink. - got TypeError: SpecialPowers.wrap(...).document.body is null; can't access its "innerHTML" property, expected "This frame was navigated."
[task 2018-11-06T15:29:42.937Z] 15:29:42     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:320:5
[task 2018-11-06T15:29:42.938Z] 15:29:42     INFO -     isNavigated@docshell/test/navigation/NavigationUtils.js:65:3
[task 2018-11-06T15:29:42.939Z] 15:29:42     INFO -     @docshell/test/navigation/test_bug270414.html:72:3
[task 2018-11-06T15:29:42.941Z] 15:29:42     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:684:12
[task 2018-11-06T15:29:42.943Z] 15:29:42     INFO -     frameFinished@docshell/test/navigation/NavigationUtils.js:162:7
[task 2018-11-06T15:29:42.944Z] 15:29:42     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:192:9
[task 2018-11-06T15:29:42.946Z] 15:29:42     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
[task 2018-11-06T15:29:42.947Z] 15:29:42     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
[task 2018-11-06T15:29:42.949Z] 15:29:42     INFO -     xpcEnumerateContentWindows@docshell/test/navigation/NavigationUtils.js:129:5
[task 2018-11-06T15:29:42.950Z] 15:29:42     INFO -     poll@docshell/test/navigation/NavigationUtils.js:203:7
[task 2018-11-06T15:29:42.952Z] 15:29:42     INFO -     setInterval handler*xpcWaitForFinishedFrames@docshell/test/navigation/NavigationUtils.js:210:27
[task 2018-11-06T15:29:42.953Z] 15:29:42     INFO -     @docshell/test/navigation/test_bug270414.html:68:1
[task 2018-11-06T15:29:42.955Z] 15:29:42     INFO - GECKO(4442) | MEMORY STAT | vsize 1446MB | residentFast 116MB | heapAllocated 19MB
[task 2018-11-06T15:29:48.604Z] 15:29:48     INFO - TEST-OK | docshell/test/navigation/test_bug270414.html | took 8347ms

Initially failed in Test Verify (Tier2), but appeared on Tier1 in a later push.

Comment 16

[Mark Banner (:standard8)]

Looks like this was a comment that was added in a function that is called in the content process. I'll update the patch and re-land.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0852730f9ecd
Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only). r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/776b54474da9
Enable ESLint for docshell/test/navigation and docshell/test/unit (manual fixes) r=bzbarsky

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1942bfe9ee2d
Backed out 2 changesets for mochitest faiures in docshell/test/navigation/test_bug270414.html

Comment 19

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1496082) for mochitest faiures in docshell/test/navigation/test_bug270414.html

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=210168616&repo=autoland&lineNumber=3227

INFO - TEST-PASS | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by setting location. 
20:33:47     INFO - Buffered messages finished
20:33:47     INFO - TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by calling window.open. - got TypeError: SpecialPowers.wrap(...).document.body is null; can't access its "innerHTML" property, expected "This frame was navigated."
20:33:47     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:320:5
20:33:47     INFO -     isNavigated@docshell/test/navigation/NavigationUtils.js:65:3
20:33:47     INFO -     @docshell/test/navigation/test_bug270414.html:70:3
20:33:47     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:684:12
20:33:47     INFO -     frameFinished@docshell/test/navigation/NavigationUtils.js:162:7
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:192:9
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
20:33:47     INFO -     xpcEnumerateContentWindows@docshell/test/navigation/NavigationUtils.js:129:5
20:33:47     INFO -     poll@docshell/test/navigation/NavigationUtils.js:203:7
20:33:47     INFO -     setInterval handler*xpcWaitForFinishedFrames@docshell/test/navigation/NavigationUtils.js:210:27
20:33:47     INFO -     @docshell/test/navigation/test_bug270414.html:68:1
20:33:47     INFO - TEST-PASS | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by submitting form. 
20:33:47     INFO - Not taking screenshot here: see the one that was previously logged
20:33:47     INFO - TEST-UNEXPECTED-FAIL | docshell/test/navigation/test_bug270414.html | Should be able to navigate on-domain opener's children by targeted hyperlink. - got TypeError: SpecialPowers.wrap(...).document.body is null; can't access its "innerHTML" property, expected "This frame was navigated."
20:33:47     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:320:5
20:33:47     INFO -     isNavigated@docshell/test/navigation/NavigationUtils.js:65:3
20:33:47     INFO -     @docshell/test/navigation/test_bug270414.html:72:3
20:33:47     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:684:12
20:33:47     INFO -     frameFinished@docshell/test/navigation/NavigationUtils.js:162:7
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:192:9
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
20:33:47     INFO -     searchForFinishedFrames@docshell/test/navigation/NavigationUtils.js:196:7
20:33:47     INFO -     xpcEnumerateContentWindows@docshell/test/navigation/NavigationUtils.js:129:5
20:33:47     INFO -     poll@docshell/test/navigation/NavigationUtils.js:203:7
20:33:47     INFO -     setInterval handler*xpcWaitForFinishedFrames@docshell/test/navigation/NavigationUtils.js:210:27
20:33:47     INFO -     @docshell/test/navigation/test_bug270414.html:68:1
20:33:47     INFO - GECKO(12008) | MEMORY STAT | vsize 1495MB | vsizeMaxContiguous 130128901MB | residentFast 93MB | heapAllocated 21MB
20:33:49     INFO - TEST-OK | docshell/test/navigation/test_bug270414.html | took 3852ms

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=776b54474da922d36446aa7cb6e93354767d0913

Backout:
https://hg.mozilla.org/integration/autoland/rev/1942bfe9ee2dfd98714ba94ec7e5aeaff74d9909

Comment 20

[Toby Ward]

Attachment: test_bug270414.html-output.txt

Hi Mark,

I saw that the patch got backed out and had a look at the issue myself, but I can't seem to recreate the issue myself in the testing suite (I've attached the output).

The main thing I can think of causing the issue though is that initializing each window to null might be a mistake (although it doesn't appear to interfere with all the subtests). Alternatively, the error output appears to suggest that the issue is caused in NavigationUtils.js; I know I had issues with ESLint in this file before with services.jsm not working, so I'm wondering if an automatic fix to the file has caused an issue? I'll definitely keep digging to see what I can do anyhow.

Comment 21

[Mark Banner (:standard8)]

(In reply to Toby Ward from comment #20)
> I saw that the patch got backed out and had a look at the issue myself, but
> I can't seem to recreate the issue myself in the testing suite (I've
> attached the output).

I found this a little variable as well, though doing `./mach mochitest --verify /path/to/test` seemed to always reproduce it.

> The main thing I can think of causing the issue though is that initializing
> each window to null might be a mistake (although it doesn't appear to
> interfere with all the subtests). Alternatively, the error output appears to
> suggest that the issue is caused in NavigationUtils.js; I know I had issues
> with ESLint in this file before with services.jsm not working, so I'm
> wondering if an automatic fix to the file has caused an issue? I'll
> definitely keep digging to see what I can do anyhow.

You're thinking along the right lines. Defining `let window0` etc seems to be wrong. For starters the "if !(window.window0)" will be broken since they'll never get set.

What I don't quite understand is the interactions between the two here as to why the exact issue occurs.

I think what is best is to undo the variable additions and just define them globally. That is working for me locally with --verify, so hopefully that'll work on the builders as well.

Comment 22

[Mark Banner (:standard8)]

Oh, I think I see what was happening - the test creates some iframes in a page, opens some windows and gets the windows to load something in the iframe of the original page.

Due to the protection against loading a second time being lost, that second load would cause testChild0 (or whichever one) to be loaded, which would open another window (or maybe-reuse the same one), and try to load into the iframe again - basically ending up in some form of continuous loop, and managing to blow up the test.

So leaving these as globals I think is the easiest thing to do.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27b21e84ba9c
Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only). r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/db6a356eef59
Enable ESLint for docshell/test/navigation and docshell/test/unit (manual fixes) r=bzbarsky

Comment 24

[Toby Ward]

(In reply to Mark Banner (:standard8) from comment #21)
> I found this a little variable as well, though doing `./mach mochitest
> --verify /path/to/test` seemed to always reproduce it.
> 

Does this not seem a little bit odd? Surely the tests should be executing in a similar way regardless of whether it was called with `./mach test /path/to/test` or `./mach mochitest --verify /path/to/test`, or am I missing something?

> You're thinking along the right lines. Defining `let window0` etc seems to
> be wrong. For starters the "if !(window.window0)" will be broken since
> they'll never get set.
> 
> What I don't quite understand is the interactions between the two here as to
> why the exact issue occurs.
> 
> I think what is best is to undo the variable additions and just define them
> globally. That is working for me locally with --verify, so hopefully that'll
> work on the builders as well.

That's fair enough. I think I defined them locally as a precaution in case it somehow interfered with other tests, but if defining globally solves the issue then that's fine.

Comment 25

[Mark Banner (:standard8)]

(In reply to Toby Ward from comment #24)
> (In reply to Mark Banner (:standard8) from comment #21)
> > I found this a little variable as well, though doing `./mach mochitest
> > --verify /path/to/test` seemed to always reproduce it.
> > 
> 
> Does this not seem a little bit odd? Surely the tests should be executing in
> a similar way regardless of whether it was called with `./mach test
> /path/to/test` or `./mach mochitest --verify /path/to/test`, or am I missing
> something?

They do run in a similar way, though --verify also runs the test repeatedly in different ways. It helps find issues with not re-initialising correctly, or sometimes just shows up intermittent timing issues a lot quicker.

Comment 26

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/27b21e84ba9c
https://hg.mozilla.org/mozilla-central/rev/db6a356eef59

Comment 27

[Mark Banner (:standard8)]

It landed :-)

Thank you for working on this Toby.

Comment 28

[Toby Ward]

(In reply to Mark Banner (:standard8) from comment #27)
> It landed :-)
> 
> Thank you for working on this Toby.

Thank you very much for helping me! I know haven't exactly been the best at dealing with the bug so I really do appreciate it. It's definitely been a learning curve for me.