lint: Forbid use of context timeouts in tests

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.

1 files changed, 13 insertions, 0 deletions

diff --git a/.golangci.yml b/.golangci.yml
index efb740563..dce1c42d7 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -13,6 +13,7 @@ linters:
     - errcheck
     - exportloopref
     - depguard
+    - forbidigo
     - gci
     # We use both gofmt and gofumpt because gofumpt doesn't seem to be linting
     # for simplifications, while gofmt does.
@@ -44,6 +45,14 @@ linters-settings:
     include-go-root: true
     packages-with-error-message:
       - io/ioutil: "ioutil is deprecated starting with Go 1.16"
+  forbidigo:
+    forbid:
+      # Tests and code which use timing-based setups have repeatedly resulted
+      # in flaky tests and are considered a code smell. Tests should be
+      # rewritten to use deterministic timing sources like tickers. Using the
+      # following functions is thus disallowed. and a code smell.
+      - ^context.WithDeadline$
+      - ^context.WithTimeout$
   stylecheck:
     # ST1000 checks for missing package comments. We don't use these for most
     # packages, so let's disable this check.
@@ -53,6 +62,10 @@ issues:
   exclude-use-default: false
   exclude-rules:
     - linters:
+        - forbidigo
+      # This fine thing excludes all paths which don't end with "_test.go".
+      path: "^([^_]|_([^t]|t([^e]|e([^s]|s([^t]|t([^\\.]|\\.([^g]|g[^o])))))))*$"
+    - linters:
         - revive
       text: "context.Context should be the first parameter of a function"
       path: "_test.go"