-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: fix failure in test/sequential/test-heapdump.js
#41772
test: fix failure in test/sequential/test-heapdump.js
#41772
Conversation
The test was failing when it was being run with superuser privileges, so this changes the test from attempting to write to a read-only file to attempting to write to a file with the same name as that of an existing directory, as that is a more reliable way of making v8.writeHeapSnapshot() throw even when run with sudo. Fixes: nodejs#41643 Signed-off-by: Darshan Sen <raisinten@gmail.com>
This comment has been minimized.
This comment has been minimized.
Looks like maybe this change makes it fail on Windows? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the directory be deleted at the end of the test?
Because we're in a test tmpdir and those are all cleaned up by test.py, it's not strictly necessary and we don't do that in most tests. We do make an exception for large files and try to clean those up. |
Signed-off-by: Darshan Sen <raisinten@gmail.com>
This comment has been minimized.
This comment has been minimized.
@Trott yes that happens because the writeHeapSnapshot() implementation uses things like fopen() instead of uv_fs_open() underneath. Added a TODO. |
const readonlyFile = 'ro'; | ||
fs.writeFileSync(readonlyFile, Buffer.alloc(0), { mode: 0o444 }); | ||
const directory = 'directory'; | ||
fs.mkdirSync(directory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change will keep the test content as same as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mawaregetsuka Yes but the intention is still the same. If you go through the PR description where this was introduced - #41373, it mentions:
this PR makes v8.writeHeapSnapshot throw if the file could not be written
So the current test still attempts to recreate a scenario where the function won't be able to write to the file, so we expect it to throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, now I understand. That's a clever solution
This comment was marked as outdated.
This comment was marked as outdated.
@nodejs/testing |
Can this get a review please? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 82181bb |
This doesn't land on 17.x, and it looks like it's because it's modifying code added in a semver-major, so I'm adding the |
The test was failing when it was being run with superuser privileges, so this changes the test from attempting to write to a read-only file to attempting to write to a file with the same name as that of an existing directory, as that is a more reliable way of making v8.writeHeapSnapshot() throw even when run with sudo. Fixes: nodejs#41643 Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: nodejs#41772 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This depends on #41373, which can not be included in v16.x |
The test was failing when it was being run with superuser privileges,
so this changes the test from attempting to write to a read-only file to
attempting to write to a file with the same name as that of an existing
directory, as that is a more reliable way of making
v8.writeHeapSnapshot()
throw even when run withsudo
.Fixes: #41643
Signed-off-by: Darshan Sen raisinten@gmail.com