test: replace fs.accessAsync with common.fileExists
#17222
Conversation
replace calls to `fs.accessAsync` with `common.fileExists` in `test/parallel/test-npm-install.js`
df1
changed the title
fs.accessAsync with common.fileExistsfs.accessAsync with common.fileExists
MylesBorins
added
the
code-and-learn
mscdex
added
the
npm
| @@ -58,6 +58,6 @@ function handleExit(error, stdout, stderr) { | |||
| assert.strictEqual(code, 0, `npm install got error code ${code}`); | |||
| assert.strictEqual(signalCode, null, `unexpected signal: ${signalCode}`); | |||
| assert.doesNotThrow(function() { | |||
| fs.accessSync(`${installDir}/node_modules/package-name`); | |||
| common.fileExists(`${installDir}/node_modules/package-name`); | |||
There was a problem hiding this comment.
common.fileExists() doesn't throw, so assert.doesNotThrow() is redundant. Either leave as is, or assert on the return value from common.fileExists().
Side note: common.fileExists() isn't necessary anymore. It was added when fs.exists() was scheduled for deprecation and removal.
There was a problem hiding this comment.
should is maybe be replaced with fs.exists?
Do you think this PR should be closed? If so should we maybe have @df1 remove some instances of common.fileExists?
There was a problem hiding this comment.
@MylesBorins do you mean fs.stat() or fs.access()?
Since fs.exists() is deprecated.
There was a problem hiding this comment.
Sorry for saying fs.exists(), when I meant fs.existsSync(). I'd be +1 to removing common.fileExists() and replacing it with fs.existsSync().
This particular change should either be backed out, or updated to use existsSync() without assert.doesNotThrow().
|
ping @df1 |
|
Hi @df1 — thank you for trying to improve Node.js. It appears that a similar change has landed recently but I hope you get a chance to contribute again in the future! Thanks again. |
replace calls to
fs.accessAsyncwithcommon.fileExistsintest/parallel/test-npm-install.jsChecklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)