refactor(e2e): use error-returning assertions - #9238
Conversation
Implements github.com/Azure/agentbaker/e2e/check with a generic, error-returning assertion API (no testing import, no panics, no ANSI, no failure callbacks): Equal[T]/NotEqual[T], Contains/NotContains (string), ContainsElement/ NotContainsElement ([]E), ContainsKey/NotContainsKey (map[K]V), NoError/Error/ErrorContains, NotNil[T]/NotEmpty[T]/Len[T], True/False/That. Failure is a structured error (Message/Note/Want/Got/Diff/Cause) with deterministic Error() output and Unwrap() for errors.Is/As. Uses reflect.DeepEqual for equality and cmp.Diff(want, got) for diffs, guarded against go-cmp panics on unexported fields. Adds github.com/google/go-cmp v0.7.0 as a direct dependency in e2e/go.mod (previously only transitive). Includes check/checkconsumer, a separate consumer package that guards against a go vet false positive: formatMessage's fallback previously forwarded straight to fmt.Sprint, which caused go vet's printf analyzer to misclassify every check assertion as a print-style wrapper and flag call sites whose message happened to contain a percent verb. That diagnostic only ever surfaces in importing packages, so vetting check alone can't catch a regression - checkconsumer exists specifically to be caught by the package tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep unit tests, logging, cleanup, signatures, and control flow unchanged while routing runtime assertions through error-returning checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Convert Config.Validator closures in the 7 scenario_*_test.go files to
return error instead of relying on failCheck/reportCheck (t.Fatal/
t.Error) side effects. Runtime helper functions they call are updated
to match, and the handful of fallible VMConfigMutator/
BootstrapConfigMutator closures (VM extension creation, Windows 2025
kubelet version lookup, RCV1P branch CSE zip build) now use the
error-capable VMConfigMutatorWithError/BootstrapConfigMutatorWithError
fields instead of failing via testing.T.
- Former require-style (failCheck) chains short-circuit with
"if err := ...; err != nil { return err }", preserving execution
order.
- Former sequential checks that are genuinely independent (unrelated
file/service assertions on an already-provisioned node) are combined
with errors.Join so a single run surfaces every failure instead of
stopping at the first.
- s.T.Logf, s.T.Cleanup, and Scenario.T are unchanged; no addCleanup or
toolkit.Logf introduced.
- Testify is untouched in the three pure unit tests that do not call
RunScenario (Test_Version_Consistency_GPU_Managed_Components,
Test_extractPackageRevision, Test_CreateVMExtensionLinuxAKSNode_Timing).
gofmt, go vet ./..., go build ./..., and go test -run '^$' ./... all
pass against the current state of the sibling core/validator/special
changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove runtime Fatal/Error assertion reporting, return errors through validators and helpers, and retain the existing logging and cleanup model. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep Testify only in local unit-test assertions and report integration assertion failures through the test boundary. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adopt labeled assertion errors, retain structured fields for future rendering, and reduce mechanical helpers to Equal and NotEqual comparisons. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move the assertion package and update E2E imports and call sites to use the assert name consistently. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Use Scenario.T to emit named ADO/JUnit timing checks while retaining aggregate error propagation from the validator. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Windows Unit Test Results 3 files 12 suites 51s ⏱️ Results for commit fb147b9. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the E2E test harness to replace testify/require/assert-style test-bound assertions with a new e2e/assert package that returns structured errors, enabling validators/helpers to propagate failures to the test boundary while still running independent checks and preserving existing diagnostics/cleanup behavior.
Changes:
- Introduces
e2e/assert(and unit tests) providing error-returning assertions in(got, want)order. - Converts multiple E2E validators/helpers to return
error(often aggregating viaerrors.Join) and updates scenario/test plumbing to handle propagated errors. - Updates VM/VMSS provisioning, execution helpers, and timing validation to return errors instead of calling
Fatal/Fatalf, while keeping existing logging/diagnostic collection patterns.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| e2e/vmss.go | Refactors VMSS model creation and custom data mutation to return errors; updates VMSS creation flow to use error-returning helpers. |
| e2e/validators_kata.go | Converts Kata validators to return aggregated errors and use e2e/assert. |
| e2e/validation.go | Converts common validation orchestration and several helpers to return error and aggregate independent failures. |
| e2e/validate_localdns_exporter_metrics.go | Converts exporter metrics validation to error-returning flow and e2e/assert. |
| e2e/types.go | Extends scenario config with *WithError mutators and changes Validator signature to func(...) error. |
| e2e/test_helpers.go | Updates scenario runner/pre-provision flow to propagate errors instead of require.*/Fatalf. |
| e2e/scenario_win_test.go | Updates Windows scenario validators/mutators to return errors and aggregate checks. |
| e2e/scenario_rcv1p_win_test.go | Updates RCV1P Windows tests to handle error-returning mutator construction and validators. |
| e2e/scenario_rcv1p_test.go | Refactors feature-flag/CSE zip helper flows to return errors (skip vs fail) and reduce hard test-bound failures. |
| e2e/scenario_gpu_managed_experience_test.go | Adds helper validators for version pin checks and refactors GPU scenarios to error-returning/ordered validation. |
| e2e/scenario_gpu_daemonset_test.go | Converts daemonset deployment/wait and validations to return errors and use e2e/assert. |
| e2e/scenario_cse_perf_test.go | Updates perf tests to consume ValidateCSETimings’ new (report, error) signature. |
| e2e/node_config.go | Refactors base NBC/template creation to return errors and adds runtime completeness checks. |
| e2e/kube.go | Converts WaitUntilNodeReady to return (string, error) instead of failing the test directly. |
| e2e/exec.go | Refactors VM/pod exec helpers to return errors; improves error handling (e.g., rand.Read) and nil safety. |
| e2e/cse_timing.go | Changes timing validation to return (report, error) while still emitting per-threshold subtests. |
| e2e/assert/assert.go | New error-returning assertion utilities with structured field formatting and wrapped causes. |
| e2e/assert/assert_test.go | Unit tests for the new e2e/assert package’s formatting and behavior. |
| e2e/artifact_streaming.go | Converts artifact streaming validation and supporting helpers to error-returning flow with preserved diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func createVMSSModel(ctx context.Context, s *Scenario) (armcompute.VirtualMachineScaleSet, error) { | ||
| if s == nil || s.Runtime == nil || s.Runtime.Cluster == nil || s.Runtime.Cluster.Model == nil || | ||
| s.Runtime.Cluster.Model.Name == nil || s.Runtime.Cluster.Model.Properties == nil || | ||
| s.Runtime.Cluster.Model.Properties.NodeResourceGroup == nil || s.Runtime.Cluster.KubeletIdentity == nil || | ||
| s.Runtime.Cluster.KubeletIdentity.ResourceID == nil || s.VHD == nil { | ||
| return armcompute.VirtualMachineScaleSet{}, fmt.Errorf("scenario runtime is incomplete for VMSS model creation") |
Report propagated scenario errors through the existing Scenario test logger so failures retain elapsed time and the red failure marker. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolve E2E pod retry changes while preserving error-returning validators. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (2)
e2e/vmss.go:335
- When addPodIPConfigsForAzureCNI fails, the error is returned without context. Wrapping it will make it clearer which step failed when VMSS creation errors bubble up.
if err != nil {
return armcompute.VirtualMachineScaleSet{}, err
}
e2e/vmss.go:223
- The nil-guard in createVMSSModel returns a generic error message, which makes it hard to debug which required Scenario field is missing. Including the key required fields in the message would make failures actionable without needing extra logs.
This issue also appears on line 333 of the same file.
return armcompute.VirtualMachineScaleSet{}, fmt.Errorf("scenario runtime is incomplete for VMSS model creation")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (3)
e2e/test_helpers.go:94
- RunScenario calls s.T.Error(err) when runScenario returns an error, but s.T is assigned inside runScenario; if runScenario fails before setting it, this will panic. Report the error on the current test handle (t) instead.
if config.Config.DisableScriptless || scriptlessUnsupported(s) {
if err := runScenario(t, s); err != nil {
s.T.Error(err)
}
return
e2e/test_helpers.go:103
- Same issue as above: s.T may be nil when handling errors from runScenario. Using t.Error(err) avoids a nil dereference and ensures the failure is attributed to the correct test/subtest.
if s.Runtime == nil {
s.Runtime = &ScenarioRuntime{}
}
s.Runtime.EnableScriptlessNBCCSECmd = true
if err := runScenario(t, s); err != nil {
s.T.Error(err)
}
e2e/test_helpers.go:86
- RunScenario reports errors via s.T.Error(err), but s.T is not initialized until inside runScenario (and in the VHDCreation subtest it may remain nil on early failure), which can panic and hide the real error. Use the local *testing.T (the subtest's t) to report the failure instead.
This issue also appears in the following locations of the same file:
- line 90
- line 97
t.Run("VHDCreation", func(t *testing.T) {
t.Parallel()
if err := runScenarioWithPreProvision(t, s); err != nil {
s.T.Error(err)
}
What this PR does / why we need it:
e2e/assertpackage that returns structured errors without depending ontesting.T.Which issue(s) this PR fixes:
N/A