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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P5
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: standard8, Assigned: tobyfrederickward, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 wontfix, firefox65 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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
(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.

Updated

8 months ago
Priority: -- → P5
Assignee

Comment 2

8 months ago
Posted file WIP .eslintignore file (obsolete) —
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
Flags: needinfo?(standard8)
Assignee: nobody → tobyfrederickward
Flags: needinfo?(standard8)
Attachment #9014926 - Attachment mime type: application/octet-stream → text/plain
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.
Assignee

Comment 4

8 months ago
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?
(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).
Assignee

Comment 6

8 months ago
Attachment #9014926 - Attachment is obsolete: true
Assignee

Comment 7

8 months ago
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).
Flags: needinfo?(standard8)
Assignee

Updated

8 months ago
Attachment #9015333 - Attachment mime type: application/octet-stream → text/plain
Answered outside the bug - we'll work through Toby's eslint issues separately.
Flags: needinfo?(standard8)
Assignee

Comment 9

7 months ago
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
Assignee

Comment 10

7 months ago
Implemented the manual linting fixes for docshell/test/navigation, docshell/test/unit and docshell/test/unit_ipc
Depends on D9430
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.
Flags: needinfo?(tobyfrederickward)
Assignee

Comment 12

7 months ago
(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
Flags: needinfo?(tobyfrederickward)
Attachment #9019125 - Attachment description: Bug 1496082: Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only) → Bug 1496082: Enable ESLint for docshell/test/navigation and docshell/test/unit (automatic fixes only). r=Standard8,r=bzbarsky

Comment 13

7 months ago
Pushed by mbanner@mozilla.com:
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
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.
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.
Flags: needinfo?(tobyfrederickward)
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.
Flags: needinfo?(tobyfrederickward)

Comment 17

7 months ago
Pushed by mbanner@mozilla.com:
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

7 months ago
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1942bfe9ee2d
Backed out 2 changesets for mochitest faiures in docshell/test/navigation/test_bug270414.html
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
Flags: needinfo?(standard8)
Assignee

Comment 20

7 months ago
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.
(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.
Flags: needinfo?(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

7 months ago
Pushed by mbanner@mozilla.com:
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
Assignee

Comment 24

7 months ago
(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.
(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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27b21e84ba9c
https://hg.mozilla.org/mozilla-central/rev/db6a356eef59
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
It landed :-)

Thank you for working on this Toby.
Assignee

Comment 28

7 months ago
(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.
You need to log in before you can comment on or make changes to this bug.