Enable mozilla/no-arbitrarySetTimeout eslint rule on mochitest-browser-chrome tests

RESOLVED FIXED in Firefox 57

Status

Testing
Mochitest
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

In bug 1383120 an eslint rule to prevent flaky setTimeout's was enabled on xpcshell tests. I didn't enable it on mochitest because of the pre-existing SimpleTest.requestFlakyTimeout mechanism. But apparently this mechanism only works for mochitest-plain, so we should still enable the eslint rule on browser/chrome/a11y/jetpack flavors.
Comment hidden (mozreview-request)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
I'm a little torn by this patch.

On one had I think it is good to notify developers as soon as we can (e.g. via the editor), about bad uses of setTimeout. On the other side, there's quite a bit of boiler plate here, and this overlaps with the not-yet-fully implemented requestFlakyTimeout which also gives a prompt for when tests are being run.

It feels like we should standardise on one or the other. We could land this anyway for now, and then head towards one or the other maybe.

Needinfo-ing Mossop & Ehsan for their thoughts.
Flags: needinfo?(ehsan)
Flags: needinfo?(dtownsend)
My understanding was that requestFlakyTimeout was only available to mochitest-plain (which this patch avoids enabling the rule on). I agree that we should not be using both on the same test suites, and also agree we should standardize on one or the other.

I think we should use the eslint implementation because:
1) Don't need to implement it per test harness
2) People are used to eslint and generally know how to disable rules (or can easily google it)
(In reply to Andrew Halberstadt [:ahal] from comment #3)
> My understanding was that requestFlakyTimeout was only available to
> mochitest-plain (which this patch avoids enabling the rule on). I agree that
> we should not be using both on the same test suites, and also agree we
> should standardize on one or the other.

I think that's because we only got as far as enabling it for mochitest-plain:

http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/testing/mochitest/tests/SimpleTest/SimpleTest.js#660

Hence why I think we should get more thoughts on this.
I lean towards standardising on linting as the way to spot and warn about bad practices in code so I'm in favour of this patch.
Flags: needinfo?(dtownsend)

Comment 6

9 months ago
requestFlakyTimeout() works currently for mochitest-chrome as well (e.g. <https://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul#59>) and I don't see why we can't make it work for browser-chrome if we wanted to (we just never ended up doing the initial triage of figuring out which one of the existing non-zero setTimeout()'s in the tests are there for a good reason and which ones can be fixed, so we didn't end up touching that suite, and it seems like that work isn't done here either, which I guess is fine since after so many years it's clear we shouldn't make the perfect the enemy of the good here.

I have no experience using lint rules so I don't know what that gives us in addition to requestFlakyTimeout(), so if it's better than what we have it's fine to replace it, I'm in no way attached to what we have now.  However, the patch as it is seems like it's making things more complex for developers, since from now on everyone who needs to add a non-zero setTimeout() to a mochitest-plain/chrome will have to add both the lint escape hatch line *and* a requestFlakyTimeout().  I think we should either leave things as is, or replace the current mechanism with linting.  Having two parallel mechanisms to do the same thing doesn't seem like an obvious improvement to me...
Flags: needinfo?(ehsan)

Comment 7

8 months ago
mozreview-review
Comment on attachment 8893928 [details]
Bug 1385352 - Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests,

https://reviewboard.mozilla.org/r/164908/#review172354

Ok, I think we should do this, but get follow-ups in progress for removing the existing mechanism and maybe seeing if we can get bugs filed for ensuring the whitelists are ok.
Attachment #8893928 - Flags: review?(standard8) → review+
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #6)
> I have no experience using lint rules so I don't know what that gives us in
> addition to requestFlakyTimeout(), so if it's better than what we have it's
> fine to replace it, I'm in no way attached to what we have now.  However,
> the patch as it is seems like it's making things more complex for
> developers, since from now on everyone who needs to add a non-zero
> setTimeout() to a mochitest-plain/chrome will have to add both the lint
> escape hatch line *and* a requestFlakyTimeout().  I think we should either
> leave things as is, or replace the current mechanism with linting.  Having
> two parallel mechanisms to do the same thing doesn't seem like an obvious
> improvement to me...

I agree, I'm trying to avoid requiring both requestFlakyTimeout *and* disabling the lint rule. This patch does not enable the lint rule for mochitest-plain and I didn't realize requestFlakyTimeout was on for chrome (and presumably a11y?). I'll re-upload this change so the lint rule only gets enabled on browser-chrome for now.

I'll also file a follow-up to replace requestFlakyTimeout with the lint rule for mochitest-plain and mochitest-chrome/a11y.
Summary: Enable mozilla/no-arbitrarySetTimeout eslint rule on browser/chrome/a11y mochitests → Enable mozilla/no-arbitrarySetTimeout eslint rule on mochitest-browser-chrome tests
Comment hidden (mozreview-request)

Comment 11

8 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6896f93a2327
Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests, r=standard8
See Also: → bug 1389234
Backed out in https://hg.mozilla.org/integration/autoland/rev/520ff4a9eea3 for not making it to m-c before the next violation merged to autoland, resulting in https://treeherder.mozilla.org/logviewer.html#?job_id=122435565&repo=autoland
Comment hidden (mozreview-request)

Comment 14

8 months ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f86c7450946b
Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests, r=standard8
https://hg.mozilla.org/mozilla-central/rev/f86c7450946b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 16

8 months ago
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #6)
> > I have no experience using lint rules so I don't know what that gives us in
> > addition to requestFlakyTimeout(), so if it's better than what we have it's
> > fine to replace it, I'm in no way attached to what we have now.  However,
> > the patch as it is seems like it's making things more complex for
> > developers, since from now on everyone who needs to add a non-zero
> > setTimeout() to a mochitest-plain/chrome will have to add both the lint
> > escape hatch line *and* a requestFlakyTimeout().  I think we should either
> > leave things as is, or replace the current mechanism with linting.  Having
> > two parallel mechanisms to do the same thing doesn't seem like an obvious
> > improvement to me...
> 
> I agree, I'm trying to avoid requiring both requestFlakyTimeout *and*
> disabling the lint rule. This patch does not enable the lint rule for
> mochitest-plain and I didn't realize requestFlakyTimeout was on for chrome
> (and presumably a11y?). I'll re-upload this change so the lint rule only
> gets enabled on browser-chrome for now.

Oh I see, OK that makes more sense.  I guess that shows how unfamiliar I am with eslint.  :-)

> I'll also file a follow-up to replace requestFlakyTimeout with the lint rule
> for mochitest-plain and mochitest-chrome/a11y.

Thanks, sounds good!
You need to log in before you can comment on or make changes to this bug.