Closed Bug 1383120 Opened 4 years ago Closed 4 years ago

Prevent arbitrary setTimeout's in xpcshell via eslint rule

Categories

(Testing :: XPCShell Harness, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files, 1 obsolete file)

This is focusing on xpcshell for now, as the mochitest harness already prevents flaky setTimeouts at runtime. Enabling an additional eslint check to prevent it there would be annoying to developers, as then they'd need to opt-out of the check in two different ways. Eventually we may want to remove the runtime check from the mochitest harness and switch over to the eslint rule, but that is out of scope for now.

Since xpcshell tests can potentially live alongside mochitests, it'll be difficult to run this *only* on xpcshell tests but not mochitests. The "overrides" key from eslint 4 might be of use here, but until then, the rule could do something like:

if (helpers.getTestType() !== "xpcshell") {
  return;
}

This might mean that it misses a few xpcshell tests, but won't run on any mochitests.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
I'm not sure what the protocol is when enabling new rules, is it right to disable the rule at the config level? Should it be disabled at the file or line level instead?
Comment on attachment 8889574 [details]
Bug 1383120 - [eslint-plugin-mozilla] Fix getTestType() helper so mochitests aren't treated as xpcshell tests,

https://reviewboard.mozilla.org/r/160592/#review166402
Attachment #8889574 - Flags: review?(dtownsend) → review+
Comment on attachment 8889575 [details]
Bug 1383120 - [eslint-plugin-mozilla] Add no-arbitrary-setTimeout eslint rule,

https://reviewboard.mozilla.org/r/160594/#review166404
Attachment #8889575 - Flags: review?(dtownsend) → review+
Comment on attachment 8889576 [details]
Bug 1383120 - Enable no-arbitrary-setTimeout eslint rule on xpcshell tests,

https://reviewboard.mozilla.org/r/160596/#review166432

I think I'd rather we just disable this rule on a per-file basis given that it isn't that many files. This way people can't add new tests that fail the test to those directories.
Attachment #8889576 - Flags: review?(dtownsend) → review-
Attachment #8889576 - Attachment is obsolete: true
Comment on attachment 8890375 [details]
Bug 1383120 - Enable no-arbitrary-setTimeout eslint rule on xpcshell tests,

https://reviewboard.mozilla.org/r/161492/#review166834

Thanks for this!
Attachment #8890375 - Flags: review?(dtownsend) → review+
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d00186874cf
[eslint-plugin-mozilla] Fix getTestType() helper so mochitests aren't treated as xpcshell tests, r=mossop
https://hg.mozilla.org/integration/autoland/rev/4653ae9b9f17
[eslint-plugin-mozilla] Add no-arbitrary-setTimeout eslint rule, r=mossop
https://hg.mozilla.org/integration/autoland/rev/e18f2b95fc8e
Enable no-arbitrary-setTimeout eslint rule on xpcshell tests, r=mossop
https://hg.mozilla.org/mozilla-central/rev/7d00186874cf
https://hg.mozilla.org/mozilla-central/rev/4653ae9b9f17
https://hg.mozilla.org/mozilla-central/rev/e18f2b95fc8e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1385352
You need to log in before you can comment on or make changes to this bug.