Conversation
runzhen
requested review from
AbelHu,
Devinwong,
SriHarsha001,
awesomenix,
calvin197,
cameronmeissner,
djsly,
ganeshkumarashok,
junjiezhang1997,
lilypan26,
mxj220,
pdamianov-dev,
phealy,
r2k1,
sulixu,
surajssd,
timmy-wright and
zachary-bailey
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a new E2E scenario runner intended to execute scenarios without t.Parallel(), and switches a set of GPU E2E tests to use it (likely to avoid GPU quota/resource contention).
Changes:
- Introduced
RunScenarioSequentialhelper ine2e/test_helpers.go. - Updated multiple GPU managed-experience tests to call
RunScenarioSequentialinstead ofRunScenario. - Updated the GPU DaemonSet test to call
RunScenarioSequential.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| e2e/test_helpers.go | Adds RunScenarioSequential wrapper around runScenario (intended to avoid t.Parallel()). |
| e2e/scenario_gpu_managed_experience_test.go | Switches several GPU managed-experience scenarios to the sequential runner. |
| e2e/scenario_gpu_daemonset_test.go | Switches the GPU DaemonSet scenario to the sequential runner. |
| func RunScenarioSequential(t *testing.T, s *Scenario) error { | ||
| // Default path | ||
| return runScenario(t, s) | ||
| } |
There was a problem hiding this comment.
RunScenarioSequential diverges from RunScenario semantics: it bypasses the config.Config.TestPreProvision || s.VHDCaching branch, so running GPU scenarios under TEST_PRE_PROVISION or with VHDCaching enabled will no longer execute the two-stage pre-provision/VHD-capture flow. Consider refactoring RunScenario into a shared helper (e.g., runScenarioWithOptions(parallel bool)) so the only behavioral difference is omitting t.Parallel() (and also omitting the inner t.Parallel() calls in the pre-provision subtests when running sequentially).
| func RunScenarioSequential(t *testing.T, s *Scenario) error { | ||
| // Default path | ||
| return runScenario(t, s) | ||
| } |
There was a problem hiding this comment.
The name RunScenarioSequential suggests it will always run scenarios sequentially, but it only avoids calling t.Parallel(); if a caller wraps it in a t.Run(...); t.Parallel() subtest, scenarios will still execute concurrently. Consider renaming to something like RunScenarioNonParallel (or similar) and/or adding a short doc comment clarifying the concurrency guarantee.
| @@ -188,7 +188,7 @@ func Test_DCGM_Exporter_Compatibility(t *testing.T) { | |||
| for _, tc := range testCases { | |||
| tc := tc // capture range variable for parallel execution | |||
There was a problem hiding this comment.
This comment is now misleading: the subtests are not marked t.Parallel() and the scenario runner used is RunScenarioSequential, so this test is no longer set up for parallel execution. Update/remove the comment (and the tc := tc note) to match the intended sequential behavior, or add t.Parallel() if parallel subtests are still desired.
| tc := tc // capture range variable for parallel execution | |
| tc := tc // capture range variable for use in this subtest |
What this PR does / why we need it:
This is a draft, don't review.
Which issue(s) this PR fixes:
Fixes #