Ava: Add idle timeout support

[MarkTiedemann]

Just switched to AVA -- and I love it! -- but, -- and this is really a minor issue -- I also noticed that I kept reusing code like this:

import test from 'ava';

function failAfter(time, t){
  setTimeout(() => {
    t.fail('timed out after ' + time + ' ms');
    t.end(); /* fail fast! */
  }, time);
}

async function thisShouldntTakeLong(){ /* or does it? >_< */
  return new Promise(resolve => setTimeout(resolve, 9999));
}

test('this shouldnt take long', async t => {
  failAfter(1000, t);
  await thisShouldntTakeLong();
});

And just to be clear, I don't like global test timeouts -- I think they're a terrible idea.

Yet again, I think something like t.failAfter(time) is not too bad.

What do you guys think? :)

[jamestalmage]

I don't like global test timeouts -- I think they're a terrible idea.

Why?

[MarkTiedemann]

@jamestalmage Well, I'm just kinda annoyed of having to change the default timeout in other test frameworks again and again.

I'm actually fine with having a global timeout, as long as it is disabled by default.

[jamestalmage]

Right now AVA hangs forever if you fail to call t.end(). That's no good. Especially if one of those tests make it to your CI server.

One thing I am always writing (in mocha) is:

if (process.env.TRAVIS) {
   this.timeout(fiveTimesWhateverItTakesLocally) 
}

I dislike having to wait to see my mistake during development, just to accommodate the slower CPU on my CI. It would be cool to build that in to AVA (though maybe not possible to do in a reliable way across all CI implementations out there).

Either way, I think AVA needs global and per tests timeouts with sensible defaults.

[sindresorhus]

I think AVA needs global and per tests timeouts with sensible defaults.

What are sensible defaults in your opinion?

[MarkTiedemann]

@jamestalmage You changed my mind. :) I agree, in CI defaults do make sense.

[jamestalmage]

Ooh. That is a hard question. :smile:

Locally, I would prefer something really fast. 500 ms or something. Some users might find that surprising (though it might also help to gently remind people that they should be writing small, fast tests). It may also unfairly inconvenience users with really old/slow CPU's.

[bookercodes]

Hi,

I gathered some data on the default timeouts used by other popular tools. Hopefully it will aid you in reasoning about a "sensible default":

| Tool | Default timeout for async tests | Source |
| --- | :-: | --: |
| Mocha | 2000ms (2s) | source |
| Jasmine | 5000ms (5s) | source |
| Node Tap | 30000ms (30s)* | source |
| Tape | None | N/A |

* 240000ms (240s) if __coverage__ global is truthy.

As an aside, I think there should be a command-line argument and/or environment variable to configure the timeout value. This'll be useful when running tests in a slow environment.

[markthethomas]

@jamestalmage @alexbooker I agree w/ the need for global/local timeouts and for me a faster default would be preferable and, to your point, encourage me to write smaller, faster tests :+1: Maybe it's crazy/overkill, but what about using something like os.cpus() and the speed prop to smart-set the timeout? I know estimating cpu perf is tough, but just a thought I had :)

[MarkTiedemann]

500 ms seems reasonable to me since AVA is supposed to be fast. And it's also supposed to be minimal; that's why I think timeouts should be disabled by default.

@markthethomas - I don't think smart-setting the timeout based on CPU information is helpful. For normal tests, you don't really need timeouts anyway. At least, when I implement timeouts, it's usually because of possible network issues, like "if you can't connect to service X within Y seconds, abort the tests".

[markthethomas]

@MarkTiedemann sounds good to me — just a thought I had :)

[vadimdemedes]

@markthethomas AVA is supposed to be fast, but that has nothing to do with user's tests. 500 ms is too small imho, I think we'd better choose tap's default timeout - 30 seconds.

[markthethomas]

Sorry, the "sounds good" was in reference to using the os module :)

Mark

Sent from my iPhone

@markthethomas everywhere

On Nov 9, 2015, at 3:42 PM, Vadim Demedes [email protected] wrote:

@markthethomas AVA is supposed to be fast, but that has nothing to do with user's tests. 500 ms is too small imho, I think we'd better choose tap's default timeout - 30 seconds.

—
Reply to this email directly or view it on GitHub.

[MarkTiedemann]

500 ms seems reasonable if you want to enforce small, fast local tests. 30 s seems reasonable if you just want to prevent CI from hanging.

So both defaults kinda make sense, in their own narrow context. So actually they don't: Because how do you know the context? You don't know.

But the solution is simple, I think. If the timeout is disabled by default, we don't need to pick a pseudo-reasonable default -- just let the developer decide!

If someone wants to run quick AVA tests locally, let him use a timeout option:

--timeout 500

And if someone just wants to prevent CI from hanging, let him set his own reasonable timeout:

// timeout still disabled
if (process.env.TRAVIS) {
  test.setTimeout(30000); // timeout now enabled!
}

How about this idea? :)

[jamestalmage]

I think both cli and api hooks are a given. We are all just bikeshedding over what the defaults will be.

[sindresorhus]

@jamestalmage Yup. If we were to have a default it would be at least 30sec. The Mocha default at times drove me mad for being too short.

[jamestalmage]

Me too. I found 2 seconds way too long! :laughing:

[MarkTiedemann]

The Mocha default at times drove me mad for being too short.

Yeah, I know that feeling. In Mocha, I usually set the timeout to 10 s just so I didn't have to worry about tests failing due to the timeout. Then again, I don't think that's good practice either. If a single test takes more than 1 s, most likely, there's something wrong with the code or the test itself (unless we are talking about stuff that is supposed to be slow, like connecting to that 20-year-old Oracle instance your underfinanced university is using to scare its undergraduates away :no_mouth:).

If we were to have a default (...)

What do you think about not having a default, about having timeouts disabled by default and allowing devs to set one if they need one?

[jamestalmage]

We need some sort of default, just as a kindness to CI providers.

[MarkTiedemann]

Well, yeah, kindness to CI providers is a good reason indeed. :)

Which reminds me: Most CI providers (including Travis, Circle and Codeship) set the CI environment variable.

So how about setting a default timeout of 30 s if process.env.CI?

[sindresorhus]

Ok, so I think the conclusion is; set a default timeout if process.env.CI (I would go with 60s instead of 30s just to be safe), and add ability to add a timeout, but no default.

[BarryThePenguin]

fyi is-ci

[ariporad]

@sindresorhus: Just my two cents, but that seems rather confusing. I wouldn't want to have to debug tests that magically fail on the CI but don't fail on my dev machine. At the very least, we need to log something about it.

[sindresorhus]

@ariporad Would you rather have the CI stall indefinitely? We will definitely log why it ended. Happy to hear other suggestions/solutions.

[ariporad]

@sindresorhus: Well, one thing to keep in mind is that Travis stops the test after 10 minutes without output, and caps it at 120 minutes period. AppVeyor has a maximum of 60 minutes. And I personally think it's out of scope for AVA, but it's up to you.

[sindresorhus]

I would also prefer not having to deal with this in AVA, but I got the impression from this thread that CI's would stall forever. I think I'm changing my opinion to timeout being opt-in no matter what.

@vdemedes @jamestalmage ?

[ariporad]

@sindresorhus: I agree. (I hope I didn't imply that there should be no timeouts period, I just meant on the CI). CIs defiantly _do not_ just sit there for years waiting for infinite loops to finish. I'm happy to put together a simple little repo to prove that if you want.

[piuccio]

Would be nice to have it. I've tried ava for the last couple days and not having timeouts is really annoying, the feedback loop is too long and you have no clue of what's broken. I hate having to setup babel + node-tap but I prefer that over hanging tests.

[novemberborn]

Since we run many tests concurrently I'm not convinced the timeout should apply to individual tests. Rather it should be an idle timeout, e.g. we're still waiting for at least 1 test and it's been 30 seconds since the last test completed.

Further when AVA is interrupted we could print out which tests are still pending. Even without defining a timeout this would let you debug things.

This may deal with the CI issue as well, assuming it interrupts AVA when killing the CI run.

[vadimdemedes]

Rather it should be an idle timeout, e.g. we're still waiting for at least 1 test and it's been 30 seconds since the last test completed.

It is actually a very good idea, @novemberborn!

[sindresorhus]

It is actually a very good idea, @novemberborn!

:+1:

[novemberborn]

Changed title to "Add idle timeout support". Presumably --idle-timeout, time in seconds after the last test completed that remaining tests should be aborted.

Would be cool if it could print out which tests were aborted.

I've opened #583 to track the interrupt idea.

[jamestalmage]

time in seconds after the last test completed that remaining tests should be aborted

Would we measure that globally, or per test file?

[novemberborn]

Would we measure that globally, or per test file?

Globally. I only care about lack of progress.

I suggested --idle-timeout before but that seems misleading. The tests aren't idle, they're just not progressing. Just --timeout is probably sufficient.

[sindresorhus]

:+1: to globally and --timeout.

[sindresorhus]

@vdemedes You're assigned to this. Still interested or should I unassign you?

[vadimdemedes]

Still interested!

[novemberborn]

Done via #654!

[gajus]

I am lost – what is the current accepted way to set timeout per test? Is it:

test((t) => {
    setTimeout(t.end, 1000);
});

?

[jamestalmage]

There isn't a per test solution. Because we launch all your tests async, and they interleave, a per test timeout wouldn't be very reliable.

Instead you just set a global timeout that measures time between test results. That's best used for preventing overlong builds on your CI server.