1448737 - Create ESLint rule to prevent tests from using resources and head scripts from other tests

Status: RESOLVED INCOMPLETE

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Kris Maglione [:kmag]]

I've run into this multiple times. I've made test-only changes to the add-on manager xpcshell tests, run tests for only that directory, since that's clearly all that I could possibly break, landed, and then found out some completely unrelated component was importing private head.js scripts and fixtures.

That's absurdly bad etiquette, even for unit tests. Our tooling should try to prevent it.

Comment 1

[Mark Banner (:standard8)]

This feels like it may be worth a wider discussion, though here's some initial thoughts.

Current state:

- There's various places where we already use head files from other directories via xpcshell's mechanism, e.g. https://searchfox.org/mozilla-central/search?q=head.*%5C.%5C.&case=false&regexp=true&path=xpcshell*.ini
- Any script can also use "loadSubScript" to import a script, e.g. https://searchfox.org/mozilla-central/search?q=head_common.js&case=false&regexp=false&path=
- Some tests use loadSubScript for part of the actual test, e.g. https://searchfox.org/mozilla-central/search?q=loadSubScript&case=false&regexp=false&path=xpcshell

Possible changes:

- Don't allow files/resources to be shared across directories.

I think this would require a test harness change rather than ESLint. We couldn't prevent inclusion of head files in xpcshell.ini with ESLint.

We could potentially replace the cross-directory imported head js files with test-only javascript modules. We couldn't remove head.js completely due to some tests needing consistent set-up functionality.

However, even with test-only javascript modules the modules could still be used elsewhere.

- Ban loadSubScript

I'm not convinced this is easily possible since some tests need to load scripts for various reasons, and detecting the true locations of files may be difficult.

- Improved ESLint detection of globals

I'm currently working on bug 1447034, that would give us the capability for ESLint to know exactly which globals are imported from other head files. However, we'll be lacking the mechanism for incremental lints to know that you've changed the head file & it has impact on other directories.


There's probably more options we can consider. Generally, I think head files are shared for various advantages in some places throughout the tree, though there's clearly an issue when we're not expecting it.

Comment 2

[Dave Townsend [:mossop]]

Bad etiquette maybe but clearly it is necessary in some cases since folks are doing it. I would hate for us to move to a model where it was banned and folks started copy and pasting head code around since that will only end badly.

Are there any downsides to enforcing testing modules for this?

Comment 3

[Andrew Halberstadt [:ahal]]

+1 to the general sentiment, but I agree with Mark that at least 'eslint' is not the place to put this.

Like Mark suggested, we could implement this in the harnesses themselves (or better yet manifestparser) such that we disallow support-files that aren't contained by an ancestor directory of the test. The downsides to this are that everything would need to be fixed up front and there would be no way to share support-files across sibling directories (assuming there are legitimate cases for this).

Another option is to implement a custom linter that parses test manifest files and checks for the same thing. We could enable this bit by bit, and it would allow people to disable the linter on manifests they *really* need to share support-files (for whatever reason).

Comment 4

[Kris Maglione [:kmag]]

(In reply to Dave Townsend [:mossop] from comment #2)
> Bad etiquette maybe but clearly it is necessary in some cases since folks
> are doing it. I would hate for us to move to a model where it was banned and
> folks started copy and pasting head code around since that will only end
> badly.

None of the cases I've seen were actually necessary. Maybe there was more of an argument before it was so easy to create test-only JSMs, but at this point, there really isn't an excuse.

In the case of head_addons.js, the code that people wanted to share should have been moved into a test-only JSM, and a peer who works on the add-on manager should have reviewed it. That's what I wound up doing when I needed those helpers in tests in other part of the tree. It also means that when I change that module, I expect it to affect other parts of the tree, and am prepared for it. And, for that matter, that tests that import it don't get all sorts of weird configuration changes that are in head_addons.js for the sake of ancient tests, but really shouldn't be used outside of the tree.

The situation is even worse for fixture add-ons, though... There's absolutely no reason code in a distant part of the tree should need to load trivial fixture add-ons from the AOM module. But sync code does, and when I make a change to one of those add-ons, it blows up.

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Like Mark suggested, we could implement this in the harnesses themselves (or
> better yet manifestparser) such that we disallow support-files that aren't
> contained by an ancestor directory of the test. The downsides to this are
> that everything would need to be fixed up front and there would be no way to
> share support-files across sibling directories (assuming there are
> legitimate cases for this).

I agree in principal. In practice, though, we currently package all of the resources used by any test in a given suite in a single artifact, which means that that kind of restriction wouldn't actually have an effect. Some of the resources the sync tests were using weren't even listed in their manifest.

I can't really even think of a way of enforcing this without something like an IO interposer. I know there's a goal of limiting the number of resources we pull in for a given test run, but we'd still need all of the resources for tests in the same group (unless we some really fancy setup and teardown between each test directory), so things would just wind up working and then breaking as tests moved between groups.

> Another option is to implement a custom linter that parses test manifest
> files and checks for the same thing. We could enable this bit by bit, and it
> would allow people to disable the linter on manifests they *really* need to
> share support-files (for whatever reason).

I'm inclined to think that people who *really* need to share resources between distant tests should generally be able to put them in testing-common. Maybe there are use cases that wouldn't work for, but in that case, I think the solution solution should probably be that we require the manifest in the directory the resources live in to specify other parts of the tree that can share them. That would at least reduce the element of surprise.

Assuming, anyway, that we actually find a way to restrict tests to using only resources they actually specify...

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 6

[Mark Banner (:standard8)]

I'm closing this out as I don't think this is likely to go anywhere. If anything, it is a test infrastructure bug, but we would need agreement on how we wanted to do this, which I don't think we would have currently.