Prevent arbitrary setTimeout's in xpcshell via eslint rule

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

2 years ago
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.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Assignee

Comment 4

2 years ago
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 5

2 years ago
mozreview-review
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 6

2 years ago
mozreview-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 7

2 years ago
mozreview-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-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8889576 - Attachment is obsolete: true

Comment 11

2 years ago
mozreview-review
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+

Comment 12

2 years ago
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

Comment 13

2 years ago
bugherder
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee

Updated

2 years ago
Blocks: 1385352
You need to log in before you can comment on or make changes to this bug.