Skip to content
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.

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

test: use unique file names in fs trace test #43504

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

bnoordhuis
Copy link
Member

Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: #43502

Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: nodejs#43502
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 20, 2022
@F3n67u
Copy link
Member

F3n67u commented Jun 20, 2022

@bnoordhuis Great work! Thank you for the fix.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 20, 2022
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

While we're at it, can const tests = new Array(); be replaced with const tests = {}; (sic!)?

@F3n67u
Copy link
Member

F3n67u commented Jun 20, 2022

While we're at it, can const tests = new Array(); be replaced with const tests = {}; (sic!)?

Make sense to me. It is wired use Array as a object/Map.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Would it make sense to use an incrementing variable in case we want to add/delete some test cases in the future?

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 20, 2022
@github-actions
Copy link
Contributor

Fast-track has been requested by @Trott. Please 👍 to approve.

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 22, 2022
@nodejs-github-bot nodejs-github-bot merged commit a7c9130 into nodejs:main Jun 22, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in a7c9130

F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 24, 2022
Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: nodejs#43502

PR-URL: nodejs#43504
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: #43502

PR-URL: #43504
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jul 20, 2022
Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: #43502

PR-URL: #43504
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: #43502

PR-URL: #43504
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Should fix test flakiness that is presumably caused by the asynchronous
nature of the unlink operation on Windows.

It's been observed that sub-tests randomly fail with "permission denied"
errors when trying to create a new file in a directory with appropriate
permissions.

The DeleteFile() NT API call makes a file inaccessible and marks it for
deletion but doesn't actually delete it until the last open handle has
been closed. Accessing such a file fails with ERROR_ACCESS_DENIED.

Processes can close handles manually or wait for the operating system to
close them asynchronously after process termination. I speculate it's
the latter that's causing the test to turn flaky.

Fixes: nodejs/node#43502

PR-URL: nodejs/node#43504
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-trace-events-fs-sync
9 participants