Skip to content

lint: Forbid use of context timeouts in tests

Patrick Steinhardt requested to merge pks-linting-disallow-context-timeouts into master

Much of the flakiness we're seeing every day is caused by timeouts which were too tight. This is only natural: our own developer machines are much beefier than the CI runners we use, and furthermore they typically don't suffer from noisy neighbours. So any timeout that works for our machine is likely to not work for CI workers. It is questionable why we'd want to have timeouts in the first place: most tests really shouldn't verify how fast a certain operation is, but instead they should verify that it does what we expect it to do. Otherwise, if one cares about speed, one should write a benchmark instead.

One valid reason is to detect tests which are stuck completely, but the Go runtime handles this for us already with the default 10 minute timeout for tests. Other usecases which want to test for example cancellation should really try hard to avoid using timeouts for this, but instead the system under test should be designed in a way that makes it possible to easily test it, for example by using manual tickers which can be injected from a test.

Forbid usage of a subset of functions which we historically used for creating contexts with timeouts. Most of the tests which used this have been refactored to not do so anymore, with the exception of some tests which explicitly want to exercise deadlines themselves.

This is only a first step: we also want to discourage the use of time.After() and time.Sleep(). These are left for a different commit series though.

Merge request reports