[12269] Test Helix Entra token refresh - #17510
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a6ad3a92-2023-4c67-8948-f6d34de1a67c
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to a test addition and the behavior it validates aligns with the stated PR intent, with only a minor maintainability suggestion noted.
Pull request overview
Adds a new Helix SDK test to exercise Entra ID token refresh behavior by using a short-lived TokenCredential and verifying the outgoing Authorization header changes after expiration, helping ensure the generated Helix API client properly reacquires tokens.
Changes:
- Added a new unit test that sends two requests across a token-expiration boundary and validates the credential is invoked again.
- Introduced a
ShortLivedTokenCredentialtest helper that issues expiring tokens and tracks acquisition count.
File summaries
| File | Description |
|---|---|
| src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/HelixApiAuthenticationTests.cs | Adds a token-expiration/refresh test using FakeHttpClient + HttpClientTransport, plus a short-lived credential helper. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to test code, aligns with the PR’s stated validation goals, and introduces no behavioral changes to production components.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Keep the expiration wait tied to the short-lived credential lifetime so the test remains valid when that lifetime changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 521e657d-a005-4f60-bcff-25a05ebcc390
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to tests and the added coverage aligns with the stated validation goals, with only minor maintainability nits noted.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/HelixApiAuthenticationTests.cs:190
- Use HttpHeader.Names.Authorization instead of a string literal to avoid typos and keep the header name consistent with the production auth policy code.
This issue also appears on line 198 of the same file.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/HelixApiAuthenticationTests.cs:198
- Use HttpHeader.Names.Authorization instead of a string literal to avoid typos and keep the header name consistent with the production auth policy code.
Assert.True(secondMessage.Request.Headers.TryGetValue("Authorization", out string secondAuthorization));
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The change is isolated to SDK tests and the new assertions directly validate the intended token refresh behavior without impacting production code.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Validation
https://dev.azure.com/dnceng/internal/_workitems/edit/12269