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.

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

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

Merged
merged 1 commit into from
Jun 22, 2022
Merged

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

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

@bnoordhuis
test: use unique file names in fs trace test
996d1eb 
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

CI: https://ci.nodejs.org/job/node-test-pull-request/44760/

LiviaMedeiros
LiviaMedeiros approved these changes Jun 20, 2022
View reviewed changes
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.

aduh95
aduh95 approved these changes Jun 20, 2022
View reviewed changes
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
@bnoordhuis @F3n67u
test: use unique file names in fs trace test
b9e4f0f 
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 <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@F3n67u F3n67u mentioned this pull request Jun 27, 2022
Merged
targos pushed a commit that referenced this pull request Jul 12, 2022
@bnoordhuis @targos
test: use unique file names in fs trace test
17b92f0 
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 <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@bnoordhuis @targos
test: use unique file names in fs trace test
b7e2175 
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 <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@bnoordhuis @targos
test: use unique file names in fs trace test
65c3e90 
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 <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@bnoordhuis @guangwong
test: use unique file names in fs trace test
d9413e2 
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 <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@richardlau richardlau richardlau approved these changes

@F3n67u F3n67u F3n67u approved these changes

@aduh95 aduh95 aduh95 approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-trace-events-fs-sync
9 participants
@bnoordhuis @F3n67u @nodejs-github-bot @Trott @benjamingr @lpinca @richardlau @aduh95 @LiviaMedeiros