Add playback tests as preflight step 8 and fix test robustness#7506
Open
jongio wants to merge 2 commits intoAzure:mainfrom
Open
Add playback tests as preflight step 8 and fix test robustness#7506jongio wants to merge 2 commits intoAzure:mainfrom
jongio wants to merge 2 commits intoAzure:mainfrom
Conversation
Add new preflight check that discovers and runs functional tests with existing recordings in AZURE_RECORD_MODE=playback. This catches stale test recordings before they cause CI failures. Changes: - Add PlaybackTests() standalone mage target and preflight step 8 - Add discoverPlaybackTests() to scan testdata/recordings for test names - Add setEnvScoped() helper for safe env var save/restore with LookupEnv - Add goModVersion() to pin GOTOOLCHAIN and prevent parallel build races - Add excludedPlaybackTests map for known stale recordings - Use regexp.QuoteMeta for test name regex safety - Filter non-test directories with strings.HasPrefix guard - Modernize os.IsNotExist to errors.Is(err, fs.ErrNotExist) - Delegate runStreaming to runStreamingWithEnv to eliminate duplication - Fix kubectl test to use mock CommandRunner (no real kubectl needed) - Fix rzip tests with conditional symlink handling (no skip) - Increase grpcbroker cancellation timeout from 1s to 5s Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new “playback” functional test run to mage preflight to catch stale/invalid recordings locally (before CI), and makes several unit tests more robust across developer environments and under load.
Changes:
- Add preflight step 8 + standalone mage target to discover recording-backed functional tests and run them in
AZURE_RECORD_MODE=playback. - Refactor env var save/restore into a shared helper and add optional
GOTOOLCHAINpinning fromgo.mod. - Improve test robustness: mock kubectl in kube config tests, make rzip symlink tests OS-capability-aware, and relax a cancellation timing assertion.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cli/azd/magefile.go | Adds playback-test discovery/execution to preflight + new env helpers/toolchain pinning. |
| cli/azd/pkg/tools/kubectl/kube_config_test.go | Avoids requiring a real kubectl binary by mocking the command runner. |
| cli/azd/pkg/rzip/rzip_test.go | Makes symlink-related expectations conditional on platform symlink support. |
| cli/azd/pkg/grpcbroker/message_broker_test.go | Increases cancellation test timeout to reduce flakiness under load. |
Comment on lines
+304
to
+316
| escaped := make([]string, len(names)) | ||
| for i, name := range names { | ||
| escaped[i] = regexp.QuoteMeta(name) | ||
| } | ||
| pattern := "^(" + strings.Join(escaped, "|") + ")$" | ||
| fmt.Printf("Running %d tests in playback mode...\n", len(names)) | ||
|
|
||
| return runStreamingWithEnv( | ||
| azdDir, | ||
| []string{"AZURE_RECORD_MODE=playback"}, | ||
| "go", "test", "-run", pattern, | ||
| "./test/functional", "-timeout", "30m", "-count=1", | ||
| ) |
There was a problem hiding this comment.
The -run regex is anchored to $ (exact match), which prevents subtests (e.g. Test_DeploymentStacks/Subscription_Scope_Up_Down) from running. Many functional tests under ./test/functional use t.Run(...) (and recordings exist for those tests), so this playback step can become a false positive by only running the top-level wrapper test while skipping the real subtests. Consider changing the regex to also match subtests, e.g. ^(TestA|TestB)(/.*)?$ or ^(TestA|TestB)(/|$).
Comment on lines
+239
to
+245
| // 8. Functional tests in playback mode (no Azure credentials needed). | ||
| fmt.Println("══ Playback tests (functional) ══") | ||
| if err := runPlaybackTests(azdDir); err != nil { | ||
| record("playback tests", "fail", err.Error()) | ||
| } else { | ||
| record("playback tests", "pass", "") | ||
| } |
There was a problem hiding this comment.
The Preflight doc comment lists the checks it runs, but the function now also runs playback functional tests as step 8. Please update the comment so it stays accurate with the actual preflight steps.
cli/azd/pkg/rzip/rzip_test.go
Outdated
Comment on lines
+23
to
+30
| // symlinkAvailable reports whether the OS supports creating symlinks in the | ||
| // given directory. On Windows without Developer Mode, os.Symlink fails with | ||
| // a privilege error. | ||
| func symlinkAvailable(t *testing.T) bool { | ||
| t.Helper() | ||
| tmp := t.TempDir() | ||
| err := os.Symlink(".", filepath.Join(tmp, "testlink")) | ||
| return err == nil |
There was a problem hiding this comment.
The comment for symlinkAvailable says it reports symlink support "in the given directory", but the function doesn't accept a directory (it always probes in a fresh t.TempDir()). Consider rewording the comment to match the implementation.
cli/azd/magefile.go
Outdated
Comment on lines
+131
to
+137
| // Pin GOTOOLCHAIN to the version declared in go.mod. When the system | ||
| // Go is older (e.g. 1.25) and go.mod says 1.26, parallel compilations | ||
| // can race the auto-download, producing "compile: version X does not | ||
| // match go tool version Y" errors. Pinning upfront avoids this. | ||
| if ver, err := goModVersion(azdDir); err == nil && ver != "" { | ||
| defer setEnvScoped("GOTOOLCHAIN", "go"+ver)() | ||
| } |
There was a problem hiding this comment.
Preflight pins GOTOOLCHAIN unconditionally (when go.mod has a version), which can force a toolchain download/downgrade even if the user already has a newer Go installed, and it temporarily overrides any user-supplied GOTOOLCHAIN value. Consider only setting GOTOOLCHAIN when it’s not already set and/or when the currently running go version is older than the version in go.mod (the scenario called out in the comment).
- Fix -run regex to match subtests by changing anchor from $ to (/|$) - Update Preflight doc comment to mention step 8 (playback tests) - Fix symlinkAvailable comment to not reference a dir parameter - Only set GOTOOLCHAIN when not already set to respect user overrides
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new preflight check (step 8) that discovers and runs functional tests with existing recordings in
AZURE_RECORD_MODE=playback. This catches stale test recordings before they cause CI failures, addressing the issue described in #7251 where CI breaks due to outdated recordings that developers don't notice locally.Motivation
From this comment: CI failures are often caused by stale test recordings, not flaky tests. Developers currently have no local signal that their changes broke a recording until CI fails. This preflight step provides that signal.
What's Changed
Preflight Step 8: Playback Tests (
cli/azd/magefile.go)PlaybackTests()— Standalone mage target and new preflight step 8discoverPlaybackTests()— Scanstest/functional/testdata/recordings/for.yamlfiles/directories, extracts top-level test namessetEnvScoped()— Helper for safe env var save/restore usingos.LookupEnv(replaces 5 duplicated save/restore blocks)goModVersion()— Reads Go version fromgo.modto pinGOTOOLCHAINand prevent parallel build racesexcludedPlaybackTests— Map of known stale recordings to exclude (currentlyTest_CLI_Deploy_SlotDeployment)regexp.QuoteMetafor test name regex safetystrings.HasPrefix(name, 'Test')guardrunStreamingtorunStreamingWithEnvto eliminate duplicationTest Robustness Fixes
kube_config_test.go—Test_MergeKubeConfignow usesmockContext.CommandRunnerinstead of requiring realkubectlbinary, matching the mock pattern used by all other tests in the packagerzip_test.go— Symlink tests usesymlinkAvailable()probe and conditionally adjust expectations instead of skipping. Tests always pass regardless of Windows developer mode statusmessage_broker_test.go— Increased context cancellation test timeout from 1s to 5s for robustness under loadHow It Works
testdata/recordings/for.yamlcassette files.yamlsuffix, takes prefix before first.for variant recordings)AZURE_RECORD_MODE=playback— zero Azure credentials needed, all HTTP responses replayed from diskTesting
mage preflight— All 8 steps pass, zero skips, zero warnings-shortflagkubectl, no symlink support required)