1795255 - Stop importing useless modules in browser tests

Status: RESOLVED FIXED

(Firefox :: General, task)

Description

[:Gijs (he/him)]

Inspired (if you can use that word) by reviewing this monster phab diff, it seems a bunch of our browser tests import modules that are already lazy imports in browser.js or otherwise end up on the global (cf. https://searchfox.org/mozilla-central/search?q=E10sUtils.(jsm|sys.mjs)&path=browser_&case=false&regexp=true ).

It would be neat if we could lint against this and just remove all the cruft, and prevent people from adding more.

Comment 1

[:Gijs (he/him)]

Lazy as I am, I'm wondering if browser mochitest linting already reuses the "browser window" env, and if so if the "don't redefine things" rule could be used for this in some way. Mark, do you know if this is the case? I know we could detect unused xpcomutils imports so I'm hoping we could detect duplicate imports as well... but maybe only for import statements, not for ChromeUtils.import, ChromeUtils.defineModuleGetter and friends?

Comment 2

[Mark Banner (:standard8)]

I think the ideal way would be to use no-redeclare with the builtinGlobals option turned on.

Unfortunately, I think there's issues with how the content code is set up that would make this hard to do everywhere and the code already reminds me about this.

The problem is that because of the way we define the browser window environment (basically, a set of globals loaded from the files defined to be the browser window), when we come to lint those files directly, no-redeclare will complain since they're already defined in the environment...

The ideal solution here would be to fix our content windows so that use ES modules everywhere and only have one entry point. ESLint then wouldn't have to worry about the browser-window environment (at least for content code) as the globals would be explicit because of the imports.

Unfortunately that's probably going to be a lot of work, so maybe as we're mainly talking about tests here the solution is to turn on builtinGlobals for tests and worry about the content code later.

I know I had bug 1575506 which was going to look at jsm scopes (applies slightly differently ES module scopes), but I think looking at enabling this for tests would probably be a good starting point.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1795255 - add no-redeclare-with-import-autofix eslint rule to deal with duplicate imports in tests, r?Standard8

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1795255 - Enable rule no-redeclare-with-import-autofix for browser tests, r?Standard8

Depends on D166177

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1795255 - autofix duplicate imports in tests under browser/, r?Standard8

Depends on D166178

Comment 6

[:Gijs (he/him)]

Attachment: Remaining eslint issues in browser/base with this patchset for illustration purposes

Comment 7

[:Gijs (he/him)]

As discussed on Matrix, we need to make a decision on what to do with the non-import redeclares. I'm not sure what best to do - we could adjust the eslint rule so it just ignores (some?) of these cases, we could compile a list of existing cases, we could try fixing all of them manually (tedious but perhaps not insurmountable given "just" 19 warnings in browser/base/ which is quite a big set of tests anyway), or add eslint-disable-next-line comments everywhere, or exempt the relevant files in the rule configuration. I'm not sure which is best.

I wonder how hard it would be to "just" rewrite all of these variables, for instance - https://searchfox.org/mozilla-central/search?q=%28let%7Cvar%7Cconst%29+URL+%3D&path=&case=true&regexp=true (includes some xpcshell tests and small smattering of prod code that may want to stay the same - but otherwise all 200-odd of these are shadowing or redeclaring URL, and we could potentially rewrite to kURL. It looks like vs code lets you do this if you open all the files - I don't know if it's straightforward to do the same using a commandline tool or eslint or some other mechanism.

Comment 8

[Mark Banner (:standard8)]

(In reply to :Gijs (he/him) from comment #7)

As discussed on Matrix, we need to make a decision on what to do with the non-import redeclares. I'm not sure what best to do - we could adjust the eslint rule so it just ignores (some?) of these cases, we could compile a list of existing cases, we could try fixing all of them manually (tedious but perhaps not insurmountable given "just" 19 warnings in browser/base/ which is quite a big set of tests anyway), or add eslint-disable-next-line comments everywhere, or exempt the relevant files in the rule configuration. I'm not sure which is best.

Ideally, I think these are cases we should fix. They highlight cases where we're redefining things that in some cases we don't need to redefine (e.g. getLoadContext, and in others where it could potentially confuse future developers (e.g. the URL case you mention below, or highlighting the amount of openContextMenu functions which are not only different to the window global, but repeated across test files...).

I will say that I don't think this is necessarily going to find errors in the code, but I do think this would be good for code cleanliness.

Across mozilla-central, there's about 1340 errors with only the imports fixed (~560 if we exclude layout/ and most of dom/). Though a good proportion of these are due to the rule being enabled on xpcshell-tests due to the non-uniform use of test directory names across the repository. In some cases, the wrong set of globals are applied to the tests, in others xpcshell reports issues as that needs more work on the set-up.

I wonder how hard it would be to "just" rewrite all of these variables, for instance - https://searchfox.org/mozilla-central/search?q=%28let%7Cvar%7Cconst%29+URL+%3D&path=&case=true&regexp=true (includes some xpcshell tests and small smattering of prod code that may want to stay the same - but otherwise all 200-odd of these are shadowing or redeclaring URL, and we could potentially rewrite to kURL. It looks like vs code lets you do this if you open all the files - I don't know if it's straightforward to do the same using a commandline tool or eslint or some other mechanism.

I think it would be good to rewrite URL, if we can do that reasonably easily, that seems like it would fix a lot of cases up front.

Roll-out wise, how about something like this:

• Modify the rule to provide a configuration option that turns off the non-import no-redeclare part (possibly want to call rename the rule as well).
• Land the rule with automated replacements, with the rule enabled everywhere(?) but with the non-import part turned off.
• Do a whole tree replacement for cases of shadowing/redeclaring URL.
• Look at cases where we need to fix ESLint architectural issues, or repository structure issues & resolve those.
• Progressively enable the non-import part of the rule rolling out on a directory basis.

Of course, I don't expect you to do all of this, quite happy to help. I think once we fix out some of the architectural/repository issues we could probably also move some of this to mentored bugs.

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1795255 - autofix duplicate imports in tests under docshell, dom, layout, security, testing, toolkit and widget, r?Standard8

Depends on D166179

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1795255 - autofix duplicate imports in tests under devtools, r?Standard8

Depends on D167715

Comment 11

[:Gijs (he/him)]

https://treeherder.mozilla.org/jobs?repo=try&revision=23ab7254485bd5cb619ba497bad596e969c5c96e

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1795255 - manual fixes for tests that break after applying this lint rule, r?Standard8

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32d1e7a0cbd3
add no-redeclare-with-import-autofix eslint rule to deal with duplicate imports in tests, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/948348f0bf5d
Enable rule no-redeclare-with-import-autofix for browser tests, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/a53cdabc2dce
manual fixes for tests that break after applying this lint rule, r=Standard8,necko-reviewers,extension-reviewers,credential-management-reviewers,sgalich,willdurand
https://hg.mozilla.org/integration/autoland/rev/f1ecda335d9c
autofix duplicate imports in tests under browser/, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/9bb2e1aa7324
autofix duplicate imports in tests under docshell, dom, layout, security, testing, toolkit and widget, r=Standard8,webdriver-reviewers
https://hg.mozilla.org/integration/autoland/rev/12d6795dc153
autofix duplicate imports in tests under devtools, r=Standard8

Comment 14

[:Gijs (he/him)]

Attachment: Bug 1795255 - bustage fix: fix up some more imports that shouldn't have been removed because of confused environment conflicts

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95db9125b940
bustage fix: fix up some more imports that shouldn't have been removed because of confused environment conflicts. CLOSED TREE

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/32d1e7a0cbd3
https://hg.mozilla.org/mozilla-central/rev/948348f0bf5d
https://hg.mozilla.org/mozilla-central/rev/a53cdabc2dce
https://hg.mozilla.org/mozilla-central/rev/f1ecda335d9c
https://hg.mozilla.org/mozilla-central/rev/9bb2e1aa7324
https://hg.mozilla.org/mozilla-central/rev/12d6795dc153
https://hg.mozilla.org/mozilla-central/rev/95db9125b940