replace wait.WithTimeout(timeout) with wait.WithContext(ctx))
#463
Conversation
| @@ -50,6 +50,9 @@ func TestMain(m *testing.M) { | |||
| log.Fatalf("failed to initialize test environment: %v", err) | |||
| } | |||
| testenv = env.NewWithConfig(cfg) | |||
| ctx, cancel := context.WithTimeout(context.Background(), 60*time.Minute) | |||
There was a problem hiding this comment.
this is a timeout for all suites? should we have a timeout per top-level suite like single, multi, etc so that we don't block all the tests if one hangs?
There was a problem hiding this comment.
Having a timeout per top-level suite is very similar to the the previous approach,.
// WithContext provides a way to configure a context that can be used to cancel the wait condition checks. This will enable
// end users to write test in cases where the max timeout is not really predictable or is a factor of a different
// configuration or event.
I think WithContext makes more sense, since some test takes longer than others, e.g. nccl test vs unit test
There was a problem hiding this comment.
should use WithContext regardless, but i meant using a different context per suite
Issue #, if available:
Description of changes:
replace
wait.WithTimeout(timeout)withwait.WithContext(ctx))By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.