1392098 - Write a rule to remove cases of run_test that aren't needed

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement)

Description

[Mark Banner (:standard8)]

Bug 982852 made run_test() in xpcshell tests optional. There's still quite a few cases where we do:

function run_test() {
  run_next_test();
}

despite the fact that xpcshell's head.js defines this for us. Hence this is just extra noise in test files.

I keep on coming across these (and cleaning them up), so I've decided it is time just to get a rule setup with fix capability and remove them completely.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions.

Review commit: https://reviewboard.mozilla.org/r/170514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170514/

Comment 2

[Mark Banner (:standard8)]

Attachment: Bug 1392098 - Enable the new ESLint no-useless-run-test rule across the tree.

Review commit: https://reviewboard.mozilla.org/r/170516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170516/

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8899276 [details]
Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170514/diff/1-2/

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8899277 [details]
Bug 1392098 - Enable the new ESLint no-useless-run-test rule across the tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170516/diff/1-2/

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 8899277 [details]
Bug 1392098 - Enable the new ESLint no-useless-run-test rule across the tree.

https://reviewboard.mozilla.org/r/170516/#review176498

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8899276 [details]
Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions.

https://reviewboard.mozilla.org/r/170514/#review176500

Do you happen to know if eslint's test harness verifies that the fixer is producing valid code?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fde66058d330
Add an ESLint rule to avoid unnecessary run_test() functions. r=mossop
https://hg.mozilla.org/integration/autoland/rev/2dd4fb2e079a
Enable the new ESLint no-useless-run-test rule across the tree. r=mossop

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8899276 [details]
Bug 1392098 - Add an ESLint rule to avoid unnecessary run_test() functions.

https://reviewboard.mozilla.org/r/170514/#review176500

I just did an experiment that left the closing brace of the function in the code - it correctly returned that there was a parsing error. I've also just seen in the documentation (https://eslint.org/docs/developer-guide/working-with-rules#applying-fixes):

```
After applying fixes, ESLint will run all of the enabled rules again on the fixed code, potentially applying more fixes. This process will repeat up to 10 times, or until no more fixable problems are found.
```

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fde66058d330
https://hg.mozilla.org/mozilla-central/rev/2dd4fb2e079a