fix: functional tests fail in playback when OIDC endpoint times out#8088
fix: functional tests fail in playback when OIDC endpoint times out#8088jongio wants to merge 3 commits intoAzure:mainfrom
Conversation
In CI playback mode, AZD_AUTH_ENDPOINT was set but AZD_AUTH_KEY was not, causing UseExternalAuth() to return false and fall through to real AzurePipelinesCredential. When the OIDC endpoint is slow, tests timeout. Three-part fix: 1. Set AZD_AUTH_KEY=playback-test-key alongside AZD_AUTH_ENDPOINT so RemoteCredential is used instead of AzurePipelinesCredential 2. Add 127.0.0.1 /token to recording proxy passthrough so local credential server requests aren't intercepted 3. When AZD_DEBUG_SYNTHETIC_SUBSCRIPTION is set and cache misses, skip ListSubscriptions() ARM call (cassette lacks /tenants response) and use the synthetic subscription directly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes intermittent CI functional test failures in playback mode by ensuring the test credential server is used instead of falling back to real Azure credentials when OIDC endpoints are slow/unreachable.
Changes:
- Set
AZD_AUTH_KEYalongsideAZD_AUTH_ENDPOINTduring playback to forceUseExternalAuth()to use the local test credential server. - Allow
/tokencalls to127.0.0.1to bypass the recording proxy so playback token requests reach the local credential server. - Avoid ARM subscription listing calls when
AZD_DEBUG_SYNTHETIC_SUBSCRIPTIONis set and cache load fails by directly constructing the synthetic subscription.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/test/recording/recording.go | Lets local credential-server /token traffic bypass the proxy in playback. |
| cli/azd/test/azdcli/cli.go | Sets AZD_AUTH_KEY so external auth is consistently enabled in playback. |
| cli/azd/pkg/account/subscriptions_manager.go | Skips real subscription listing when using a synthetic subscription after cache load failure. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
wbreza
left a comment
There was a problem hiding this comment.
Review Summary
Well-scoped fix for intermittent CI playback test failures. All three parts are correct and consistent with existing patterns. Two non-blocking suggestions below.
Findings
1. 🟡 Consider adding a unit test for the new synthetic subscription path (Important)
*\cli/azd/pkg/account/subscriptions_manager.go* — The new code path (cache miss + \AZD_DEBUG_SYNTHETIC_SUBSCRIPTION\ set → skip \ListSubscriptions()) has no dedicated unit test. A test in \subscriptions_manager_test.go\ that mocks a cache miss, sets the env var, and asserts \ListSubscriptions()\ is NOT called would protect against regressions.
2. 🟡 Missing issue link (Important)
Branch name references #8074\ but the PR body doesn't contain \Fixes #8074. Repo governance checks may flag this.
Verified ✅
- \UseExternalAuth()\ correctly requires both \AZD_AUTH_ENDPOINT\ + \AZD_AUTH_KEY\
- \AZD_AUTH_KEY\ only set in playback mode (guarded by \opt.Session.Playback)
- Hardcoded "playback-test-key"\ is safe (loopback only, matches existing \�uth_test.go\ pattern)
- Synthetic subscription struct fields correct (\TenantId\ + \UserAccessTenantId\ both from \claims.TenantId)
- Cache intentionally not saved for synthetic (prevents test artifacts)
- \AZD_DEBUG_SYNTHETIC_SUBSCRIPTION\ already documented in \docs/environment-variables.md\
- All new lines comply with 125-char limit
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @wbreza!
|
wbreza
left a comment
There was a problem hiding this comment.
✅ Approved — Re-Review
The new commits address the unit test gap from the first review. The test correctly validates that ListSubscriptions() is skipped when AZD_DEBUG_SYNTHETIC_SUBSCRIPTION is set and cache misses. Well done.
Minor suggestions (non-blocking)
-
Test could also verify TenantId fields —
principalInfoProviderMockreturns emptyTenantIdin claims, so the synthetic subscription'sTenantIdandUserAccessTenantIdare empty strings in the test. Setting a TenantId in the mock and addingrequire.Equalassertions on those fields would strengthen coverage. -
Issue link still missing — Branch references
#8074but PR body doesn't containFixes #8074. Governance checks may flag this.
Verified ✅
- Unit test correctly uses
BypassSubscriptionsCachefor cache miss simulation listSubscriptionsCalledflag pattern properly detects unwanted ARM callst.Setenv/t.Context()follow repo conventions.gitignoreaddition is clean housekeeping- All lines within 125-char limit
hemarina
left a comment
There was a problem hiding this comment.
Review — PR #8088
LGTM ✅ — well-scoped fix for the playback-mode credential fallback. The three-part diagnosis (missing AZD_AUTH_KEY, missing /token passthrough, synthetic-subscription path hitting unrecorded /tenants) is precise and each fix targets exactly one symptom.
Verified
UseExternalAuth()requires bothAZD_AUTH_ENDPOINTandAZD_AUTH_KEY; setting both in playback only (guarded byopt.Session.Playback) is correct.- Hardcoded
"playback-test-key"is safe — loopback-only, never leaves the test process. - Synthetic-subscription branch correctly bypasses both
ListSubscriptions()andcache.Save(); the cache skip is intentional to avoid test artifacts persisting. - New unit test
TestSubscriptionsManager_SyntheticSubscription_SkipsListOnCacheMisscorrectly asserts both the skip behavior and the returned subscription shape. - Recording-proxy passthrough rule is consistent with the surrounding
strings.Containspattern, and the/tokenpath guard makes false-positive host matches practically impossible.
Minor (non-blocking, optional)
- The test could strengthen coverage by giving
principalInfoProviderMocka non-emptyTenantIdand assertingresult.subscriptions[0].TenantId == claims.TenantIdandUserAccessTenantId == claims.TenantId. Without that, both fields are silently empty strings in the test — already noted by @wbreza. .gitignore.worktrees/addition is unrelated to the fix — fine to leave, but worth keeping unrelated churn out of fix PRs going forward.
Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low | 2 |
Problem
Functional tests like \Test_CLI_Publish_ContainerApp_RemoteBuild\ intermittently fail in CI with:
\
AzurePipelinesCredential: unable to resolve an endpoint: context deadline exceeded
\\
Root Cause
In playback mode, the test framework set \AZD_AUTH_ENDPOINT\ (pointing to the local test credential server) but never set \AZD_AUTH_KEY. Since \UseExternalAuth()\ requires both to be non-empty, it returned \alse, causing azd to fall through to real \AzurePipelinesCredential. When the OIDC endpoint is slow/unreachable, the test times out.
Fix (three parts)
*\ est/azdcli/cli.go* — Set \AZD_AUTH_KEY=playback-test-key\ alongside \AZD_AUTH_ENDPOINT\ so \UseExternalAuth()\ returns true and \RemoteCredential\ is used.
*\ est/recording/recording.go* — Add \127.0.0.1\ with path /token\ to the recording proxy passthrough list so the local credential server isn't intercepted.
*\pkg/account/subscriptions_manager.go* — When \AZD_DEBUG_SYNTHETIC_SUBSCRIPTION\ is set and cache load fails, skip calling \ListSubscriptions()\ (which calls ARM /tenants\ API not in the cassette) and directly create the synthetic subscription.
Testing