test: fix failure in test/sequential/test-heapdump.js - #41772
Conversation
Sorry, something went wrong.
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.
|
Looks like maybe this change makes it fail on Windows? |
Sorry, something went wrong.
aduh95
left a comment
There was a problem hiding this comment.
Shouldn't the directory be deleted at the end of the test?
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Signed-off-by: Darshan Sen <raisinten@gmail.com>
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. |
Sorry, something went wrong.
| const readonlyFile = 'ro'; | ||
| fs.writeFileSync(readonlyFile, Buffer.alloc(0), { mode: 0o444 }); | ||
| const directory = 'directory'; | ||
| fs.mkdirSync(directory); |
There was a problem hiding this comment.
I don't think this change will keep the test content as same as before
Sorry, something went wrong.
There was a problem hiding this comment.
@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.
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you, now I understand. That's a clever solution
Sorry, something went wrong.
This comment was marked as outdated.
|
@nodejs/testing |
Sorry, something went wrong.
|
Can this get a review please? |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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 dont-land-on-17 label. Please comment if you think it should land. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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: #41643
Signed-off-by: Darshan Sen raisinten@gmail.com