fix: Move timeout ctx closer to use in tests, increase timeout #3109
Conversation
Some contexts were moved closer to use so that test setup doesn't affect timeout. And timeout was increased for some others to avoid flakyness due to slow test runners.
| @@ -65,6 +63,9 @@ func TestProvisionerJobLogs_Unit(t *testing.T) { | |||
| {ID: uuid.New(), JobID: jobID, Stage: "Stage3"}, | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |||
| defer cancel() | |||
There was a problem hiding this comment.
Why defer here over t.Cleanup?
There was a problem hiding this comment.
I feel tests should prefer to read like idiomatic Go code, t.Cleanup is useful for helper functions but has no benefit over defer.
It could also lead to logic bugs where there are defers and t.Cleanup mixed, because the order of execution would differ.
defer close1()
t.Cleanup(close2)
defer close3()This would execute in the following order: close3, close1, close2 likely leading to a logic bug.
| @@ -48,7 +48,7 @@ func TestCaching(t *testing.T) { | |||
| defer srv.Close() | |||
|
|
|||
| // Create a context | |||
| ctx, cancelFunc := context.WithTimeout(context.Background(), 1*time.Second) | |||
| ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Second) | |||
There was a problem hiding this comment.
(Idea, out-of-scope): should we have a "standard" set of timeouts we use for all unit tests?
e.g. "short" -> 1 second, "medium" -> 5 seconds, "long", 10 seconds?
There was a problem hiding this comment.
I really like that idea! It's all over the place now and doesn't allow us to easily adjust for slow/fast platforms.
Some contexts were moved closer to use so that test setup doesn't affect
timeout. And timeout was increased for some others to avoid flakyness
due to slow test runners.
This CI run ran into context deadline exceeded:
https://github.com/coder/coder/runs/7456533733?check_suite_focus=true