test: fix failure in test/sequential/test-heapdump.js#41772
Uh oh!
There was an error while loading. Please reload this page.
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 <[email protected]>
nodejs-github-bot added needs-ci 
This comment has been minimized.
This comment has been minimized.
Trott commented Jan 30, 2022
Looks like maybe this change makes it fail on Windows? |
aduh95 left a comment
aduh95 left a comment There was a problem hiding this comment.
Shouldn't the directory be deleted at the end of the test?
Trott commented Jan 30, 2022
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 <[email protected]>
This comment has been minimized.
This comment has been minimized.
RaisinTen commented Jan 31, 2022
@Trott yes that happens because the writeHeapSnapshot() implementation uses things like fopen() instead of uv_fs_open() underneath. Added a TODO. |
| constreadonlyFile='ro'; | ||
| fs.writeFileSync(readonlyFile,Buffer.alloc(0),{mode: 0o444}); | ||
| constdirectory='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
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.
There was a problem hiding this comment.
Thank you, now I understand. That's a clever solution
This comment was marked as outdated.
This comment was marked as outdated.
Trott commented Feb 1, 2022
@nodejs/testing |
RaisinTen commented Feb 20, 2022
Can this get a review please? |
aduh95 added author ready 
github-actionsbot removed the request-ci 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.
nodejs-github-bot commented Mar 14, 2022
aduh95 added the commit-queue nodejs-github-bot removed the commit-queue nodejs-github-bot commented Mar 14, 2022
Landed in 82181bb |
RaisinTen deleted the test/fix-failure-in-test/sequential/test-heapdump.js-when-run-with-sudo branch 
bengl commented Mar 21, 2022
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 <[email protected]> PR-URL: nodejs#41772 Reviewed-By: Antoine du Hamel <[email protected]>
juanarbol commented May 28, 2022
This depends on #41373, which can not be included in v16.x |
7 participants
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 [email protected]