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

Status: RESOLVED FIXED

(Testing :: Mochitest, enhancement)

Description

[Andrew Halberstadt [:ahal]]

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 1

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1385352 - Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests,

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

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

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)

Comment 4

[Mark Banner (:standard8)]

(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.

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[(no longer active)]

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...

Comment 7

[Mark Banner (:standard8)]

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.

Comment 8

[Andrew Halberstadt [:ahal]]

(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.

Comment 9

[Andrew Halberstadt [:ahal]]

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

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

Comment 10

[Andrew Halberstadt [:ahal]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26fa08eb84d05f8c781c53d40653cae7c05de893

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6896f93a2327
Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests, r=standard8

Comment 12

[Phil Ringnalda (:philor)]

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 13

[Andrew Halberstadt [:ahal]]

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

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f86c7450946b
Enable 'mozilla/no-arbitrary-setTimeout' eslint rule on browser-chrome tests, r=standard8

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/f86c7450946b

Comment 16

[(no longer active)]

(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!