refactor(e2e): log through a Logger interface instead of testing.TB - #9280
Conversation
Windows Unit Test Results 3 files 13 suites 1m 8s ⏱️ Results for commit 06c19a1. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Refactors E2E logging behind a minimal Logger interface while retaining testing.TB only for explicit test control, complementing #9262’s cleanup boundary.
Changes:
- Introduces context-aware test and standard loggers with failure formatting.
- Migrates scenario helpers and validators from
testing.TBlogging. - Adds tests for logger capabilities, fallback behavior, formatting, and artifact naming.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
e2e/vmss.go |
Migrates VMSS logging and artifact naming. |
e2e/validators.go |
Migrates validator logging and removes helper calls. |
e2e/validators_kata.go |
Migrates Kata validator logging. |
e2e/validation.go |
Migrates common validation logging. |
e2e/validate_localdns_exporter_metrics.go |
Migrates LocalDNS exporter logging. |
e2e/types.go |
Adds scenario logger and test name fields. |
e2e/toolkit/strings.go |
Uses context logger for duration logs. |
e2e/toolkit/log.go |
Defines logger abstractions and failure formatting. |
e2e/toolkit/log_test.go |
Tests logger boundaries and formatting. |
e2e/test_helpers.go |
Initializes loggers at the runner boundary. |
e2e/scenario_gpu_managed_experience_test.go |
Migrates GPU scenario logging. |
e2e/scenario_gpu_daemonset_test.go |
Migrates daemon-set scenario logging. |
e2e/log.go |
Decouples artifact helpers from testing.TB. |
e2e/log_test.go |
Tests artifact paths and VMSS names. |
e2e/kube.go |
Migrates node readiness logging. |
e2e/exec.go |
Migrates command diagnostic logging. |
e2e/cse_timing.go |
Migrates timing report logging. |
e2e/config/azure_vmext_test.go |
Updates tests to use context loggers. |
e2e/artifact_streaming.go |
Migrates artifact-streaming diagnostics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| func (l *testLogger) Log(args ...any) { | ||
| l.tb.Helper() |
|
CI triage: Agentbaker GPU E2E (build Only The same scenario fails identically on This PR cannot be the cause. Worth a separate repair item: raise the WestUS3 A100 quota or serialize the A100 scenarios, and fail fast when a VMSS comes back with no ID instead of burning ~200s on doomed bastion handshakes. |
E2E implementation code logged through testing.TB, which gave every validator and helper the power to fail, skip or rename the test. The context also carried a testing.TB, so the capability reached anything holding a context. Introduce toolkit.Logger, an interface with Log and Logf only. testLogger holds the testing.TB in an unexported field instead of embedding it, so no test-control method leaks. ContextWithT is replaced by ContextWithLogger, and the context now carries a Logger. Scenario gains a Logger field, and the ~200 direct s.T.Log/s.T.Logf call sites use it. The 149 s.T.Helper() calls only shaped log attribution, so they are removed; the logger marks its own frames instead. Artifact path helpers take an explicit test name rather than a testing.TB. Test control stays explicit at the runner boundary. Scenario.T remains for Error, Failed, Skip, Name, Run and Cleanup, and the failure decoration moves to toolkit.WithFailureFormatting, a testing.TB decorator. Log text, elapsed timing, step markers, failure diagnostics and log line attribution are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
c9f9b74 to
06c19a1
Compare
What this PR does / why we need it:
Remove explicit calls to *testing.T for logging. Preparation to move away from go testing for E2E.