Timers and tests

[Trott]

What should be the general things we want to happen with timers in tests?

• Recommend against using timers if at all possible. (Obviously, if the test is specifically testing timers, then using them is fine.)
• Do we distinguish between setTimeout(), setImmediate(), and process.nextTick()? I think only setTimeout() is generally problematic. The other two are usually fine.
• Tests that rely on timers should go in sequential and not in parallel.

What else?

[Trott]

Idea for allowing timers from CLI but not in CI: Maybe a common.testTimeOut() in common that can detect if it is being run on CI or not. If on the CLI, it sets a timer that calls common.fail() with a message supplied by the test. If run from CI, it is a no-op.

[orangemocha]

If run from CI, it is a no-op.

What about specifying test timeouts based on a "reference system" and applying different multipliers on each platform. CI needs not be an exception.

[Trott]

What about specifying test timeouts based on a "reference system" and applying different multipliers on each platform. CI needs not be an exception.

The problem is that these things are bugs/anti-patterns, and not features. Getting them out of CI is what we're aiming for. It's not a grudging compromise to give up a desirable feature.

• There are already timeouts on CI. Code-based timeouts are unnecessary on CI.
• Running the code-based timeouts in CI as we are currently doing is easily the top cause of flakiness. Making it platform-dependent (via common.platformTimeout()) sometimes helps, but it does not always eliminate the flakiness.
• The arbitrariness of the timeout values is a code smell. (If the test starts failing at 500ms, is that the sign of a problem or does it just mean that we need to bump it up to 1000ms?)
• As far as I am aware, the one compelling reason to include the code-based timeouts in CI is to improve error messages. If the error messages are still there in the code but not used by CI, I think that's a good compromise.

[joaocgreis]

For reference, one test that uses a code-based timeout is test-repl.js: https://github.com/nodejs/node/blob/master@%7B2016-03-22%7D/test/parallel/test-repl.js#L467-L469

Here's a failure on a very slow RPi1 because of this: https://ci.nodejs.org/job/node-test-binary-arm/1468/RUN_SUBSET=3,nodes=pi1-raspbian-wheezy/tapTestReport/test.tap-97/

[orangemocha]

I should have stated that I am in full agreement with this statement:

Recommend against using timers if at all possible. (Obviously, if the test is specifically testing timers, then using them is fine.)

So I was thinking of the 60 second timeout that is now used in CI to determine that a test is hanging. If I understood correctly, devs would like that timeout to be shorter when running tests on their machine. What is the right value of that timeout? Maybe 10 seconds? But for tests that take a bit longer to complete, what's the way to declare an exception to the default?

[Trott]

I think it's more like this: There's a bunch of tests where there is code like this:

setTimeout(() => {common.fail('This test took too long! Event foo must not be firing on object bar!');}, X);

...where X is some arbitrary number. Might be 5000, might be 500.

Sometimes, we go to remove these things, and it sails through. Sometimes we go to remove them and someone states the opinion that if you run the test at the command line (without the test.py wrapper), you should get a decent error message when it fails rather than having the test hang and hang until you end it.

In cases like that, it would be nice to have some pre-baked function loaded from common.js that sets the arbitrary timeout if and only if you are not on CI (or maybe if and only if you are not running via test.py, I'd be fine with that too). It might look like this:

common.testTimeoutForCLIOnly(someFunction, someArbitraryNumberOfMilliseconds);

[santigimeno]

What should be the general things we want to happen with timers in tests?

• Recommend against using timers if at all possible. (Obviously, if the test is specifically testing timers, then using them is fine.)

I totally agree.

The problem is that these things are bugs/anti-patterns, and not features. Getting them out of CI is what we're aiming for. It's not a grudging compromise to give up a desirable feature.

• There are already timeouts on CI. Code-based timeouts are unnecessary on CI.
• Running the code-based timeouts in CI as we are currently doing is easily the top cause of flakiness. Making it platform-dependent (via common.platformTimeout()) sometimes helps, but it does not always eliminate the flakiness.
• The arbitrariness of the timeout values is a code smell. (If the test starts failing at 500ms, is that the sign of a problem or does it just mean that we need to bump it up to 1000ms?)

I wouldn't have said it better.

Sometimes we go to remove them and someone states the opinion that if you run the test at the command line (without the test.py wrapper), you should get a decent error message when it fails rather than having the test hang and hang until you end it.

Has this response been that common? I don't remember anyone complaining about it (probably I have missed those comments). It seems to me that lately the PR's that remove the timers have been accepted with little (no) resistance.

I think that, for the reasons that @Trott enumerated, we should consider that, in addition to recommending against the use of timers (and probably its removal too), not to implement any special mechanism to force a special timeout behavior. If somebody wants to run the tests with a specific timeout there's test.py. If somebody is running a specific test from the command line and the test takes a long time, it should be clear what the problem is (even more if it's a test in parallel).

Just my 2 cents.

[Trott]

Has this response been that common?

Honestly, no. I've perhaps provided too much weight to a single instance.

[Fishrock123]

I think only setTimeout() is generally problematic. The other two are usually fine.

Correct, the others are guaranteed to fire in pretty much any case.

[Trott]

Closing as this WG is defunct and this issue has been dormant for a year. Feel free to comment (or, if GitHub allows, re-open) if you think it should stay open.