Closed Bug 1795255 Opened 2 years ago Closed 2 years ago

Stop importing useless modules in browser tests

Categories

(Firefox :: General, task)

Desktop
All
task

Tracking

()

RESOLVED FIXED
111 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox107 --- wontfix
firefox109 --- wontfix
firefox110 --- wontfix
firefox111 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

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.

Type: defect → task

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?

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

Flags: needinfo?(standard8)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

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.

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

Flags: needinfo?(standard8)
See Also: → 1575506
Attachment #9311041 - Attachment description: WIP: Bug 1795255 - add no-duplicate-imports eslint rule to deal with duplicate imports in tests, r?Standard8 → Bug 1795255 - add no-redeclare-with-import-autofix eslint rule to deal with duplicate imports in tests, r?Standard8
Attachment #9311042 - Attachment description: WIP: Bug 1795255 - Enable rule no-duplicate-import for browser tests, r?Standard8 → Bug 1795255 - Enable rule no-duplicate-import for browser tests, r?Standard8
Attachment #9311043 - Attachment description: WIP: Bug 1795255 - WIP fix duplicate imports in browser/base → Bug 1795255 - fix duplicate imports in tests under browser/, r?Standard8
Attachment #9311042 - Attachment description: Bug 1795255 - Enable rule no-duplicate-import for browser tests, r?Standard8 → Bug 1795255 - Enable rule no-redeclare-with-import-autofix for browser tests, r?Standard8
Attachment #9311043 - Attachment description: Bug 1795255 - fix duplicate imports in tests under browser/, r?Standard8 → Bug 1795255 - autofix duplicate imports in tests under browser/, r?Standard8
Attachment #9313869 - Attachment description: Bug 1795255 - fix duplicate imports in tests under dom, toolkit and widget, r?Standard8 → Bug 1795255 - autofix duplicate imports in tests under docshell, dom, layout, security, testing, toolkit and widget, r?Standard8
Blocks: 1812563
Pushed by gijskruitbosch@gmail.com: 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
Attachment #9315082 - Attachment description: WIP: Bug 1795255 - bustage fix: fix up some more imports that shouldn't have been removed because of confused environment conflicts → Bug 1795255 - bustage fix: fix up some more imports that shouldn't have been removed because of confused environment conflicts
Pushed by imoraru@mozilla.com: 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
See Also: → 1814275
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: