We can't change the timeout for each individual call which is unfortunate. I know a lot of tests just use a single long context for the entire test, but some tests do have multiple contexts with different timeouts for different calls.

Also, the behavior of resetting on every new call means we can't set test-wide timeouts anymore (except with the -timeout flag on go test, which is intentionally very large to account for the entire test suite).

Maybe we need a different API to solve these things? Here's something off the top of my head:

func TestBlah(t *testing.T) {
  xctx := testutil.ContextProvider(t, testutil.WaitLong) // full test timeout, lazily creates a parent context when we hit the first call
  // ...
  err := MyFunction(xctx.New(testutil.WaitMedium), param) // makes a new "child" context from the parent
}



func testutil.ContextProvider(t *testing.T, testTimeout time.Duration) *testutil.ParentContext {
  return &testutil.ParentContext{
    testTimeout: testTimeout,
  }
}

type testutil.ParentContext struct {
  testTimeout time.Duration

  // todo: synchronization fields...
  ctx context.Context
}

func (c *testutil.ParentContext) New(timeout time.Duration) context.Context {
  c.ensureInitialized() // ensures c.ctx is set
  return context.WithTimeout(c.ctx, timeout)
}

// todo: Other methods as needed, e.g. NewWithCancel, NewWithDeadline

^ Which would just be a very light wrapper around the functions in the context package.

It's a shame that we have tests that initialize the context early followed by heavy non-context consuming startup cost. Does this mostly just stem from tests using coderdtest?

Thanks for taking a look and the discussion @deansheather, I didn't make it clear in the ticket but this is more of a design/proposal to spark discussion.

Also, the behavior of resetting on every new call means we can't set test-wide timeouts anymore (except with the -timeout flag on go test, which is intentionally very large to account for the entire test suite).

This is a valid concern. The reasoning is that in a massively parallel test suite, it's hard to reason about what length of timeout is actually valid, and rather move the thought process to "maximum time we want this test to sit idle".

Do keep in mind that call-site is tracked, so you do get an approximate timeout for the test, even though it's not exactly 15 seconds, perhaps more like 25 seconds.

Granted, this implementation is not a great way to shift the thinking from "max test time" to "max idle time".

Maybe we need a different API to solve these things? Here's something off the top of my head:

This is an interesting alternative, and one we could probably enforce/lint. (It's very hard to lint the referenced bug-case without also causing false positives, which sparked this PR.)

Taking some inspiration from you proposal, I could see an more ergonomic alternative that also functions as a drop-in.

package testutil

var _ context.Context = (*TestContext)(nil)

func Context(...) *TestContext { ... } // always lazy start (or option?)

type TestContext struct{}

func (tc *TestContext) Cancel() { ... }                  // we were missing this previously
func (tc *TestContext) New(timeout) *TestContext { ... } // make timeout optional?
func (tc *TestContext) Reset() *TestContext { ... }      // resets the timer

func (tc *TestContext) Join(ctx ...context.Context) *TestContext { ... } // sometimes you have two independent contexts, but can only pass one along
// more?

This would still allow the most basic use to be ctx := testutil.Context(t, testutil.WaitMedium) whilst using it as a normal context.

My opinion is that this is a lot of complexity for a relatively narrow benefit.

in a massively parallel test suite, it's hard to reason about what length of timeout is actually valid, and rather move the thought process to "maximum time we want this test to sit idle".

This trades one thing that is hard to reason about for something that is much harder. Speaking about the original lazy context, when you make a call in the test function and pass the context, it is very difficult to know whether it will trigger a reset. If the call synchronously "uses" the context, then it is reset, but if it spawns a goroutine that "uses" the context, then it is not.

Even if the resets are explicit, like in your second proposal, the question of what duration to use is still hard to reason about. Like, if your test has steps A, B, C, and D with resets in between, with the current, basic context you have to estimate the sum of the durations. With resets you have to estimate the max. Have we actually made life easier for test authors? Dubious, IMO.

@spikecurtis thanks for taking a look and for the feedback. You raise valid concerns and I honestly don’t disagree with you on any point.

It seems the only concept here that might hold some value is the delayed start, but is it worth the decrease in predictability? Probably not.

I briefly considered flipping the logic, error of context is not used within 1/2 or 2/3 of the timeout. But now we’re just breaking valid use-cases. Maybe something takes a context but doesn’t use it and takes long enough to process you hit the error. Rejected.

One more option comes to mind, swapping out the implementation via build tag allowing you to run a form of ”lint” on the test-suite, where we could enforce the error case. But again, having this code in the code base simply might not be worth catching the few weird flakes we run into.

This was a fun thought experiment, thanks for participating! Unless you guys have any better ideas, I’m going to close this one out.