fix: use single e2e and run all flavours by awesomenix · Pull Request #8319 · Azure/AgentBaker

awesomenix · 2026-04-15T03:34:31Z

Use single e2e to run all scenarios, dont know if this will work

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Introduces “scenario run type” fanout so a single e2e invocation can execute multiple execution profiles (default/scriptless/scriptless NBC CSE cmd), and updates e2e logic + pipeline to use per-profile run options instead of global-only flags.

Changes:

Add scenario run types/profiles with support filtering and tag filter composition for fanout runs.
Update VMSS/customData, validators, and scenario skipping/prep to use scenario-specific run options.
Simplify pipeline to run a single e2e stage with SCENARIO_RUN_TYPES=all.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
e2e/vmss.go	Use scenario-aware run options when deciding scriptless compilation / NBC hack behavior.
e2e/validators.go	Validate expected NBC cmd script path based on scenario-aware run options.
e2e/types.go	Add scenario fields to support per-run profiles and disallow unsupported run types.
e2e/test_helpers.go	Add fanout behavior in `RunScenario` and apply scenario-aware run options for skip/prep.
e2e/scenario_test.go	Update one test to cooperate with fanout and declare unsupported run type(s).
e2e/run_types.go	New: run type/profile definitions, option composition, and support checks.
e2e/run_types_test.go	New: unit tests for tag composition and support filtering.
e2e/config/config.go	Add `SCENARIO_RUN_TYPES` env var to control fanout.
.pipelines/.vsts-vhd-builder.yaml	Collapse multiple e2e stages into one using `SCENARIO_RUN_TYPES=all`.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Copilot · 2026-04-15T08:03:20Z

+type ScenarioRunProfile struct {
+	Type                         ScenarioRunType
+	TagsToRun                    string
+	TagsToSkip                   string
+	DisableScriptLessCompilation bool
+	EnableScriptlessNBCCSECmd    bool
+}


The ScenarioRunTypeScriptless profile only adds tag filters + DisableScriptLessCompilation, but it does not enable the scriptless execution mode (EnableScriptlessCSECmd) used in prepareAKSNode. With pipelines now using SCENARIO_RUN_TYPES=all (and no per-stage ENABLE_SCRIPTLESS_CSE_CMD), the “scriptless” fanout run can end up executing without the scriptless CSE cmd behavior, effectively making the profile not apply its intended runtime switch. Add an EnableScriptlessCSECmd boolean to ScenarioRunProfile and compose it in buildScenarioRunOptions, then set it to true for the ScenarioRunTypeScriptless default profile.

Copilot · 2026-04-15T08:03:20Z

+		{
+			Type:                         ScenarioRunTypeScriptless,
+			TagsToRun:                    "scriptless=true",
+			DisableScriptLessCompilation: true,
+		},


The ScenarioRunTypeScriptless profile only adds tag filters + DisableScriptLessCompilation, but it does not enable the scriptless execution mode (EnableScriptlessCSECmd) used in prepareAKSNode. With pipelines now using SCENARIO_RUN_TYPES=all (and no per-stage ENABLE_SCRIPTLESS_CSE_CMD), the “scriptless” fanout run can end up executing without the scriptless CSE cmd behavior, effectively making the profile not apply its intended runtime switch. Add an EnableScriptlessCSECmd boolean to ScenarioRunProfile and compose it in buildScenarioRunOptions, then set it to true for the ScenarioRunTypeScriptless default profile.

Copilot · 2026-04-15T08:03:20Z

+func buildScenarioRunOptions(base scenarioRunOptions, s *Scenario) scenarioRunOptions {
+	if s == nil || s.RunProfile == nil {
+		return base
+	}
+
+	base.TagsToRun = joinTagFilters(base.TagsToRun, s.RunProfile.TagsToRun)
+	base.TagsToSkip = joinTagFilters(base.TagsToSkip, s.RunProfile.TagsToSkip)
+	base.DisableScriptLessCompilation = base.DisableScriptLessCompilation || s.RunProfile.DisableScriptLessCompilation
+	base.EnableScriptlessNBCCSECmd = base.EnableScriptlessNBCCSECmd || s.RunProfile.EnableScriptlessNBCCSECmd
+
+	return base
+}


The ScenarioRunTypeScriptless profile only adds tag filters + DisableScriptLessCompilation, but it does not enable the scriptless execution mode (EnableScriptlessCSECmd) used in prepareAKSNode. With pipelines now using SCENARIO_RUN_TYPES=all (and no per-stage ENABLE_SCRIPTLESS_CSE_CMD), the “scriptless” fanout run can end up executing without the scriptless CSE cmd behavior, effectively making the profile not apply its intended runtime switch. Add an EnableScriptlessCSECmd boolean to ScenarioRunProfile and compose it in buildScenarioRunOptions, then set it to true for the ScenarioRunTypeScriptless default profile.

Copilot · 2026-04-15T08:03:21Z

+	scenarioRunProfilesByType = map[ScenarioRunType]ScenarioRunProfile{
+		ScenarioRunTypeDefault:             defaultScenarioRunProfiles[0],
+		ScenarioRunTypeScriptless:          defaultScenarioRunProfiles[1],
+		ScenarioRunTypeScriptlessNBCCSECmd: defaultScenarioRunProfiles[2],
+	}


scenarioRunProfilesByType depends on hard-coded indices into defaultScenarioRunProfiles. This is brittle (reordering the slice or inserting a new profile will silently change semantics). Prefer defining the map with explicit struct literals (or building the map in a small init loop keyed by profile.Type) to avoid index coupling.

Suggested change

scenarioRunProfilesByType = map[ScenarioRunType]ScenarioRunProfile{

ScenarioRunTypeDefault: defaultScenarioRunProfiles[0],

ScenarioRunTypeScriptless: defaultScenarioRunProfiles[1],

ScenarioRunTypeScriptlessNBCCSECmd: defaultScenarioRunProfiles[2],

}

scenarioRunProfilesByType = func() map[ScenarioRunType]ScenarioRunProfile {

profilesByType := make(map[ScenarioRunType]ScenarioRunProfile, len(defaultScenarioRunProfiles))

for _, profile := range defaultScenarioRunProfiles {

profilesByType[profile.Type] = profile

}

return profilesByType

}()

Copilot · 2026-04-15T08:03:21Z

+func configuredScenarioRunProfiles() ([]ScenarioRunProfile, error) {
+	if !scenarioRunFanoutEnabled() {
+		return nil, nil
+	}
+
+	rawRunTypes := strings.TrimSpace(config.Config.ScenarioRunTypes)
+	if strings.EqualFold(rawRunTypes, "all") {
+		return append([]ScenarioRunProfile(nil), defaultScenarioRunProfiles...), nil
+	}
+
+	requestedProfiles := strings.Split(rawRunTypes, ",")
+	runProfiles := make([]ScenarioRunProfile, 0, len(requestedProfiles))
+	seen := map[ScenarioRunType]struct{}{}
+	for _, requestedProfile := range requestedProfiles {
+		runType := ScenarioRunType(strings.TrimSpace(requestedProfile))
+		if runType == "" {
+			continue
+		}
+		if _, ok := seen[runType]; ok {
+			continue
+		}
+		profile, ok := scenarioRunProfilesByType[runType]
+		if !ok {
+			return nil, fmt.Errorf("unsupported scenario run type %q", runType)
+		}
+		runProfiles = append(runProfiles, profile)
+		seen[runType] = struct{}{}
+	}
+
+	return runProfiles, nil
+}


New parsing/validation behavior is introduced in configuredScenarioRunProfiles, but e2e/run_types_test.go doesn’t cover key cases like SCENARIO_RUN_TYPES=all, deduplication, and the error path for unsupported run types. Adding focused unit tests for these cases will help prevent regressions in pipeline configuration handling.

r2k1 · 2026-04-15T08:15:59Z

This looks way more complicated than it can be.

For reference, the existing VHD caching pattern at test_helpers.go#L76-L89 already does exactly this — copyScenario(original), mutate the copy, run it. The scriptless fanout should follow the same simple approach rather than introducing a profile/options resolution layer.

Something like:

func RunScenario(t *testing.T, s *Scenario) error {
      t.Parallel()
      t.Run("original", func(t *testing.T) {
          t.Parallel()
          runScenario(t, copyScenario(s))
      })
      if config.EnableScriptless && s.AKSNodeConfigMutator != nil { // scriptless scenarios
          t.Run("scriptless", func(t *testing.T) {
              t.Parallel()
              s := copyScenario(s)
              s.DisableScriptlessCompilation = true
              runScenario(t, s)
          })
      }
      if config.EnableScriptlessNBC && s.AKSNodeConfigMutator == nil && !s.IsWindows() {
          t.Run("scriptless_nbc", func(t *testing.T) {
              t.Parallel()
              s := copyScenario(s)
              s.EnableScriptlessNBCCSECmd = true
              s.DisableScriptlessCompilation = true
              runScenario(t, s)
          })
      }
      return nil
    // ... existing code
}

No new types, no new files — flags go directly on Scenario (like VHDCaching already does), and prepareAKSNode/vmss.go read from s.DisableScriptlessCompilation instead of config.Config.DisableScriptLessCompilation.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Copilot · 2026-04-15T18:38:58Z

+	t.Run("default", func(t *testing.T) {
+		t.Parallel()
+		runScenario(t, copyScenario(s))
+	})


runScenario returns an error but the new subtests ignore it. There are code paths in runScenario that return an error without failing the test (e.g., if err != nil { return err }), which can lead to false-positive passes. Mandatory: check the returned error inside each subtest (e.g., require.NoError(...)), while still allowing ExpectedError cases to be handled by the existing assertions.

Copilot · 2026-04-15T18:38:59Z

+	t.Run("default", func(t *testing.T) {
+		t.Parallel()
+		runScenario(t, copyScenario(s))
+	})
+
+	if supportsScriptlessNBCCSECmd(s) {
+		t.Run("scriptless_nbc", func(t *testing.T) {
+			t.Parallel()
+			sCopy := copyScenario(s)
+			sCopy.EnableScriptlessNBCCSECmd = true
+			runScenario(t, sCopy)
+		})
+	}


These subtests run in parallel and rely on copyScenario(s), but the provided copyScenario implementation (per context excerpt) is explicitly a shallow/placeholder copy. With parallel execution, any shared slice/map/pointer fields inside Scenario (or nested config) can race and cause flaky tests. Mandatory: either (a) implement a real deep copy for all mutated/shared nested fields and keep parallelism, or (b) remove t.Parallel() from these subtests to keep single-threaded execution per parent test.

Copilot · 2026-04-15T18:38:59Z

+func supportsScriptlessNBCCSECmd(s *Scenario) bool {
+	return s.AKSNodeConfigMutator == nil && !s.IsWindows() && len(s.Config.CustomDataWriteFiles) <= 0 && !s.VHDCaching && !config.Config.TestPreProvision
 }


supportsScriptlessNBCCSECmd doesn’t account for scenarios that are known to be unsupported in the NBC CSE cmd path. In this PR, Test_Ubuntu2204_ScriptlessCSECmd_Hotfix no longer skips for that condition, so the new scriptless_nbc subtest will run and likely fail despite the test’s intent. Mandatory: gate the scriptless_nbc variant with an explicit per-scenario opt-in/opt-out (e.g., a Scenario flag like EnableScriptlessNBCVariant / DisableScriptlessNBCVariant), or extend supportsScriptlessNBCCSECmd with a concrete signal used by the hotfix test (such as a dedicated tag/field) so unsupported scenarios don’t run the NBC variant.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

e2e/scenario_test.go:1

This will not compile if RunScenario still returns an error (a call returning values can’t be used as a bare statement in Go). Either capture the return value (e.g., assign to _ or assert on it), or change RunScenario’s signature to not return an error and update call sites consistently.

package e2e

e2e/test_helpers.go:76

RunScenario now performs assertions inside subtests and always returns nil, which makes the error return value misleading and (as seen in updated tests) easy to use incorrectly. Consider changing the signature to func RunScenario(t *testing.T, s *Scenario) and removing the unused return paths, or restore meaningful error propagation and keep assertions at the call site.

func RunScenario(t *testing.T, s *Scenario) error {

Copilot · 2026-04-15T18:55:28Z

+	t.Run("default", func(t *testing.T) {
+		t.Parallel()
+		err := runScenario(t, copyScenario(s))
+		require.NoError(t, err)
+	})
+
+	if supportsScriptlessNBCCSECmd(s) {
+		t.Run("scriptless_nbc", func(t *testing.T) {
+			t.Parallel()
+			sCopy := copyScenario(s)
+			sCopy.EnableScriptlessNBCCSECmd = true
+			err := runScenario(t, sCopy)
+			require.NoError(t, err)
+		})
+	}
+	return nil


The two flavour subtests run in parallel and rely on copyScenario(s) to isolate mutations. Given the current copyScenario context indicates it’s a shallow/placeholder copy, this can lead to data races and cross-test contamination (e.g., runScenario/prepareAKSNode mutate fields like s.T, s.Runtime, s.Location, and may mutate nested/slice fields). Make copyScenario a real deep copy of all mutated nested fields (and ensure Runtime starts nil per run), or remove t.Parallel() from these subtests so flavours run sequentially.

Copilot · 2026-04-15T18:55:29Z

+			require.NoError(t, err)
+		})
+	}
+	return nil


RunScenario now performs assertions inside subtests and always returns nil, which makes the error return value misleading and (as seen in updated tests) easy to use incorrectly. Consider changing the signature to func RunScenario(t *testing.T, s *Scenario) and removing the unused return paths, or restore meaningful error propagation and keep assertions at the call site.

r2k1 · 2026-04-15T20:15:13Z

-		nbc.EnableScriptlessCSECmd = true
-	}
-	if config.Config.EnableScriptlessNBCCSECmd {
+	nbc.EnableScriptlessCSECmd = true


So, it's something we set always to true now?
Should it be inside NBC initialiser?

Yep eventually will remove this parameter, will wait until then

r2k1 · 2026-04-15T20:18:23Z

 	Location string

+	// EnableScriptlessNBCCSECmd indicates whether to enable scriptless NBCCSECmd in the scenario.
+	EnableScriptlessNBCCSECmd bool


Maybe move it to ScenarioRuntime to separate configurable and non-configurable options

r2k1 · 2026-04-15T20:23:38Z

 	return ctx
 }

 func RunScenario(t *testing.T, s *Scenario) error {


I guess this now may have no return value?

Agreed. Will change this in follow up PR

Copilot AI review requested due to automatic review settings April 15, 2026 03:34

awesomenix force-pushed the nishp/multiplescenario branch from 48362c1 to ace4273 Compare April 15, 2026 03:36

awesomenix temporarily deployed to test April 15, 2026 03:36 — with GitHub Actions Inactive

awesomenix force-pushed the nishp/multiplescenario branch from ace4273 to 90e1c0f Compare April 15, 2026 03:41

awesomenix temporarily deployed to test April 15, 2026 03:41 — with GitHub Actions Inactive

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread e2e/run_types.go Outdated

Comment thread e2e/test_helpers.go Outdated

Comment thread e2e/test_helpers.go Outdated

Comment thread e2e/run_types.go Outdated

Comment thread e2e/run_types_test.go Outdated

Comment thread .pipelines/.vsts-vhd-builder.yaml Outdated

Copilot started reviewing on behalf of awesomenix April 15, 2026 05:15 View session

awesomenix force-pushed the nishp/multiplescenario branch from 90e1c0f to 1adb440 Compare April 15, 2026 07:47

awesomenix temporarily deployed to test April 15, 2026 07:47 — with GitHub Actions Inactive

awesomenix force-pushed the nishp/multiplescenario branch from 1adb440 to 13660b5 Compare April 15, 2026 07:54

Copilot AI review requested due to automatic review settings April 15, 2026 07:54

awesomenix temporarily deployed to test April 15, 2026 07:54 — with GitHub Actions Inactive

awesomenix marked this pull request as ready for review April 15, 2026 07:54

awesomenix requested review from AbelHu, Devinwong, SriHarsha001, calvin197, cameronmeissner, djsly, ganeshkumarashok, junjiezhang1997, lilypan26, mxj220, pdamianov-dev, phealy, r2k1, sulixu, surajssd, timmy-wright and zachary-bailey as code owners April 15, 2026 07:54

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Copilot started reviewing on behalf of awesomenix April 15, 2026 09:54 View session

awesomenix force-pushed the nishp/multiplescenario branch from 13660b5 to d79e7bb Compare April 15, 2026 17:41

awesomenix temporarily deployed to test April 15, 2026 17:41 — with GitHub Actions Inactive

Copilot AI review requested due to automatic review settings April 15, 2026 18:31

awesomenix force-pushed the nishp/multiplescenario branch from d79e7bb to 25eb8c2 Compare April 15, 2026 18:31

awesomenix temporarily deployed to test April 15, 2026 18:31 — with GitHub Actions Inactive

Devinwong approved these changes Apr 15, 2026

View reviewed changes

Copilot AI reviewed Apr 15, 2026

View reviewed changes

awesomenix force-pushed the nishp/multiplescenario branch from 25eb8c2 to 320e91a Compare April 15, 2026 18:44

awesomenix temporarily deployed to test April 15, 2026 18:44 — with GitHub Actions Inactive

fix: run single e2e to run multiple scenarios

fa7180d

Copilot AI review requested due to automatic review settings April 15, 2026 18:47

awesomenix force-pushed the nishp/multiplescenario branch from 320e91a to fa7180d Compare April 15, 2026 18:47

awesomenix temporarily deployed to test April 15, 2026 18:47 — with GitHub Actions Inactive

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Devinwong approved these changes Apr 15, 2026

View reviewed changes

Copilot started reviewing on behalf of awesomenix April 15, 2026 19:46 View session

Copilot started reviewing on behalf of awesomenix April 15, 2026 19:50 View session

r2k1 approved these changes Apr 15, 2026

View reviewed changes

awesomenix merged commit 2240050 into main Apr 15, 2026
40 of 44 checks passed

awesomenix deleted the nishp/multiplescenario branch April 15, 2026 21:23

-	scenarioRunProfilesByType = map[ScenarioRunType]ScenarioRunProfile{
-		ScenarioRunTypeDefault:             defaultScenarioRunProfiles[0],
-		ScenarioRunTypeScriptless:          defaultScenarioRunProfiles[1],
-		ScenarioRunTypeScriptlessNBCCSECmd: defaultScenarioRunProfiles[2],
-	}
+	scenarioRunProfilesByType = func() map[ScenarioRunType]ScenarioRunProfile {
+		profilesByType := make(map[ScenarioRunType]ScenarioRunProfile, len(defaultScenarioRunProfiles))
+		for _, profile := range defaultScenarioRunProfiles {
+			profilesByType[profile.Type] = profile
+		}
+		return profilesByType
+	}()

Conversation

awesomenix commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

r2k1 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r2k1 Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

awesomenix Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

r2k1 Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

r2k1 Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

awesomenix Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

r2k1 commented Apr 15, 2026 •

edited

Loading

r2k1 Apr 15, 2026 •

edited

Loading