Skip to content

Fix flaky workloadIdentityCredentialIdentityBinding test on Windows #36745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
minhanh-phan merged 5 commits into from
Dec 9, 2025
Merged

Fix flaky workloadIdentityCredentialIdentityBinding test on Windows #36745

minhanh-phan merged 5 commits into from
Dec 9, 2025

Conversation

Copy link
Contributor

Copilot AI commented Nov 25, 2025 •
edited
Loading

Packages impacted by this PR

  • @azure/identity

Issues associated with this PR

Describe the problem that is addressed by this PR

Test should use custom token endpoint with CA file configuration fails intermittently on Windows with ENOTEMPTY: directory not empty, rmdir. The "Certificate Validation & Processing" tests used fs.unlink() + fs.rmdir() for temp directory cleanup, which fails on Windows due to timing issues with file handle releases. Additionally, the inline cleanup approach meant that cleanup would not occur if test assertions failed.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Two main approaches were implemented:

  1. Replace two-step cleanup pattern: Changed from fs.unlink() + fs.rmdir() to fs.rm(tempDir, { recursive: true, force: true }) for reliable cross-platform directory cleanup. This pattern is already used elsewhere in this test file ("Configuration validation" and "Identity binding feature" sections).

  2. Refactor cleanup to afterEach block: Moved cleanup logic to an afterEach hook to ensure cleanup happens even if test assertions fail. This involved:

    • Declaring tempDir and tempCaFile as describe-block-level variables typed as string | undefined
    • Adding error handling in the cleanup to prevent test suite failures from filesystem issues
    • Resetting variables to undefined after cleanup to prevent stale path reuse

This combined approach ensures both reliable cleanup on Windows and guaranteed cleanup regardless of test outcome.

Are there test cases added in this PR? (If not, why?)

No new tests needed—this fixes existing test cleanup reliability and robustness.

Provide a list of related PRs (if any)

N/A

Command used to generate this PR:**(Applicable only to SDK release request PRs)

N/A

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)
Original prompt

This section details on the original issue you should resolve

<issue_title>[test] [identity] flaky workloadIdentityCredentialIdentityBinding test?</issue_title>
<issue_description>```text
@azure/identity:test:node: [vitest] ⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯
@azure/identity:test:node: [vitest]
@azure/identity:test:node: [vitest] FAIL test/internal/node/workloadIdentityCredentialIdentityBinding.spec.ts > WorkloadIdentityCredential - Identity Binding Configuration > Identity binding feature > should use custom token endpoint with CA file configuration
@azure/identity:test:node: [vitest] Error: ENOTEMPTY: directory not empty, rmdir 'C:\Users\CLOUDT~1\AppData\Local\Temp\tempDir3YnkHc'
@azure/identity:test:node: [vitest] ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
@azure/identity:test:node: [vitest]
@azure/identity:test:node: [vitest]
@azure/identity:test:node: [vitest] Test Files 1 failed | 38 passed | 3 skipped (42)
@azure/identity:test:node: [vitest] Tests 1 failed | 384 passed | 17 skipped (402)
@azure/identity:test:node: [vitest] Start at 19:35:52
@azure/identity:test:node: [vitest] Duration 27.69s (transform 12.00s, setup 0ms, collect 37.61s, tests 21.64s, environment 14ms, prepare 816ms)


## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@jeremymeng</author><body>
https://dev.azure.com/azure-sdk/public/public%20Team/_build/results?buildId=5605868&view=logs&j=3e883751-031e-56f9-9938-d1ed90b686e8&t=209f8deb-5df3-5fcd-d026-4e85ab01a9fe&l=2126</body></comment_new>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@minhanh-phan
Fix flaky workloadIdentityCredentialIdentityBinding test cleanup
bba3360 
Replace fs.unlink + fs.rmdir with fs.rm for reliable directory cleanup on Windows

Co-authored-by: minhanh-phan <[email protected]>
Copilot AI changed the title [WIP] Fix flaky WorkloadIdentityCredentialIdentityBinding test Fix flaky workloadIdentityCredentialIdentityBinding test on Windows Nov 25, 2025
Copilot AI requested a review from minhanh-phan November 25, 2025 18:26
@minhanh-phan minhanh-phan marked this pull request as ready for review November 25, 2025 19:04
Copilot AI review requested due to automatic review settings November 25, 2025 19:04
Copilot AI reviewed Nov 25, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a Windows-specific flaky test failure in the @azure/identity package. The test should use custom token endpoint with CA file configuration was intermittently failing on Windows with ENOTEMPTY: directory not empty, rmdir errors during cleanup. The fix standardizes the directory cleanup approach to use the more robust fs.rm() method instead of the two-step fs.unlink() + fs.rmdir() pattern.

Key changes:

  • Replaces two-step cleanup (fs.unlink() + fs.rmdir()) with single-step fs.rm() using recursive and force options
  • Applied to two test cases in the "Certificate Validation & Processing" section

@minhanh-phan
Copy link
Member

minhanh-phan commented Nov 25, 2025

Pipelines run success here

Copilot AI and others added 3 commits December 8, 2025 18:58
@minhanh-phan
Refactor cleanup to afterEach block for Certificate Validation tests
ef2cb98 
Move tempDir cleanup to afterEach to ensure cleanup happens even if assertions fail

Co-authored-by: minhanh-phan <[email protected]>
@minhanh-phan
Improve afterEach cleanup with error handling and state reset
8056d56 
Add try-catch to handle cleanup errors gracefully and reset variables to prevent stale path reuse

Co-authored-by: minhanh-phan <[email protected]>
@minhanh-phan
Fix type safety in variable declarations
2248e45 
Change tempDir and tempCaFile to string | undefined for proper type safety

Co-authored-by: minhanh-phan <[email protected]>
@minhanh-phan
Copy link
Member

minhanh-phan commented Dec 9, 2025

Pipelines succeed here

@minhanh-phan
Copy link
Member

minhanh-phan commented Dec 9, 2025

/check-enforcer reset

@minhanh-phan minhanh-phan merged commit 854d41e into main Dec 9, 2025
14 checks passed
@minhanh-phan minhanh-phan deleted the copilot/fix-flaky-workload-identity-test branch December 9, 2025 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@maorleger maorleger maorleger approved these changes

@minhanh-phan minhanh-phan Awaiting requested review from minhanh-phan minhanh-phan is a code owner

@KarishmaGhiya KarishmaGhiya Awaiting requested review from KarishmaGhiya KarishmaGhiya is a code owner

@joshfree joshfree Awaiting requested review from joshfree joshfree is a code owner automatically assigned from Azure/azure-sdk-write-identity

@pvaneck pvaneck Awaiting requested review from pvaneck pvaneck is a code owner automatically assigned from Azure/azure-sdk-write-identity

@JonathanCrd JonathanCrd Awaiting requested review from JonathanCrd JonathanCrd is a code owner automatically assigned from Azure/azure-sdk-write-identity

@anannya03 anannya03 Awaiting requested review from anannya03 anannya03 is a code owner automatically assigned from Azure/azure-sdk-write-identity

Assignees

@minhanh-phan minhanh-phan

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[test] [identity] flaky workloadIdentityCredentialIdentityBinding test?

3 participants

@minhanh-phan @maorleger