Add a setTimeout checker for Gaia integration tests to help catch intermittent failures

RESOLVED WONTFIX

Status

Firefox OS
Gaia::UI Tests
RESOLVED WONTFIX
4 years ago
5 months ago

People

(Reporter: Ehsan, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

4 years ago
In Gecko we have gained a lot of experience debugging intermittent test failures, and one our findings is that tests that use setTimeout to wait for arbitrary amounts of times are often relying on timing information that is not accurate and hence prone to failing intermittently (due to reasons such as slower hardware, more system load, relying implicitly on ordering of events, etc.)

Today I enabled a checker for the Gecko mochitest-plain test suite that catches these types of patterns.  The implementation is fairly straightforward, see <https://hg.mozilla.org/integration/mozilla-inbound/file/fa8bd3194aca/testing/mochitest/tests/SimpleTest/SimpleTest.js#l619> up to line 661.

How this works is that we replace window.setTimeout for every test file with a function that creates a test failure if setTimeout is called with a second argument > 0, and invokes the original setTimeout function immediately.  There are few scenarios where the usage of setTimeout is legitimate and impossible to avoid.  For example, if the test is waiting to check to see a given event does _not_ happen in the future.  For those cases we have added an explicit API to our test harness that you are supposed to use like this:

SimpleTest.requestFlakyTimeout("I'm using a timeout in this test but that's OK because ...");

This function suppresses the check inside the setTimeout shim, and is mostly intended as a note to the reviewer to investigate the timeout usages in the test.

We hope that this check helps people discover these bad patterns locally before checking in the test.  It would be nice if the same idea was adopted in the Gaia integration tests as well.
I'm not sure this is needed as we generally try to ban the use of non-zero setTimeout in tests. I think our time these days would be better spent trying to make our integration tests more solid. In those cases we generally suffer from not having sufficient wait statements.
Hrm, there is no "Gaia::MarionetteJS Tests"/"Gaia::integration Tests" component?
Gaia UI tests component is usually used for the python UI tests, afaict.

(In reply to Kevin Grandon :kgrandon from comment #1)
> I'm not sure this is needed as we generally try to ban the use of non-zero
> setTimeout in tests.

But that's what this setTimeout checker tries to ensure, no? It makes sure no new tests can be added that way.
(Reporter)

Comment 3

4 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #2)
> (In reply to Kevin Grandon :kgrandon from comment #1)
> > I'm not sure this is needed as we generally try to ban the use of non-zero
> > setTimeout in tests.
> 
> But that's what this setTimeout checker tries to ensure, no? It makes sure
> no new tests can be added that way.

Yes, that is exactly the point of doing this.
I suppose I just feel like a linter is a better way to solve this problem. Just prevent the addition of setTimeouts in the first place.
(Reporter)

Comment 5

4 years ago
(In reply to Kevin Grandon :kgrandon from comment #4)
> I suppose I just feel like a linter is a better way to solve this problem.
> Just prevent the addition of setTimeouts in the first place.

If we can disallow even setTimeout(..., 0) then a linter is the right solution (as long as it is run on our test machines!)
I believe we should be able to use sinonJS for timeout and any necessary mocks. I think we should be able to write a linter to perform this.

Even if we still need zero-based timeouts, the linter could allow setTimeout(), but only when there is no second argument for the actual time.

Comment 7

5 months ago
Firefox OS is not being worked on
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.