1736467 - Make calling SimpleTest.registerCleanupFunction in browser mochitests work, or a linter error

Status: NEW

(Developer Infrastructure :: Lint and Formatting, task)

Description

[:Gijs (he/him)]

Right now it's a no-op, and that's a footgun - the cleanup doesn't get run, but the code runs without errors, which is very confusing.

I ran into this in my patch in bug 1735368 after a lot of confusion - but there are also at least these 6 cases already in-tree: https://searchfox.org/mozilla-central/search?q=simpletest.registercleanup&path=browser_&case=false&regexp=false

(there might be more in head.js files, but those are harder to disambiguate in terms of environment in a quick searchfox query)

I don't know how hard it'd be to make this work; it looks like we can detect the case in SimpleTest.js but registerCleanupFunction is not defined directly in there. Mark, do you know?

Alternatively, and perhaps more simply, we could just make this an eslint error...

Comment 1

[Mark Banner (:standard8)]

Ideally, I think we shouldn't have these multiple different test running systems in the same test suites. I think that is likely to always cause us issues. Unfortunately I suspect this isn't going to be easy to unpick.

One alternative option we could do is to have the add_task in browser-test.js handle the stored SimpleTest._cleanupFunctions. Then it would work regardless.

The cause-a-failure or an ESLint option might be better in the long term if we do want to try and separate these.

Comment 2

[:Gijs (he/him)]

(In reply to Mark Banner (:standard8) from comment #1)

Ideally, I think we shouldn't have these multiple different test running systems in the same test suites. I think that is likely to always cause us issues. Unfortunately I suspect this isn't going to be easy to unpick.

Isn't it? Just looking at https://searchfox.org/mozilla-central/search?q=SimpleTest.&path=browser_&case=false&regexp=false I wonder if we could just export those SimpleTest methods that are useful as global functions in browser_ and then disallow SimpleTest inside browser_ tests altogether? Some of the other uses beyond registerCleanupFunction, like SimpleTest.is (wat) also seem like they might be hiding issues... There's about 370 hits but most of them are the same subsets of functions and rewriting them with eslint doesn't feel like an insurmountably big problem...

Maybe I'm missing something?

Comment 3

[Mark Banner (:standard8)]

Ok, I think that would be reasonable. It would be nice to see if we can stop SimpleTest being exposed to browser_ tests at all, but failing that, using ESLint would work.