Fix(which): match only executable files (#657)#874
Conversation
2369ccd to
644e632
Compare
Codecov Report
@@ Coverage Diff @@
## master #874 +/- ##
=========================================
+ Coverage 95.8% 95.8% +<.01%
=========================================
Files 34 34
Lines 1263 1264 +1
=========================================
+ Hits 1210 1211 +1
Misses 53 53
Continue to review full report at Codecov.
|
nfischer
left a comment
There was a problem hiding this comment.
Thanks for the great (and quick) PR!
test/which.js
Outdated
|
|
||
| test('None executable files does not appear in the result list', t => { | ||
| const commandName = 'node'; // Should be an existing command | ||
| const pathEnv = process.env.path || process.env.Path || process.env.PATH; |
There was a problem hiding this comment.
This doesn't look right. This should be process.env.PATH.
Only all-caps $PATH works on unix. On Windows, process.env.path is totally the same as process.env.PATH (and all variations of capitalization of "path").
test/which.js
Outdated
| const matchingFile = path.resolve(extraPath, commandName); | ||
|
|
||
| // make sure that file is exists (will throw error otherwise) | ||
| fs.statSync(matchingFile); |
There was a problem hiding this comment.
nit: can you make this t.truthy(fs.existsSync())? We generally prefer assertion failures over exceptions.
| // set on Windows. | ||
| var XP_DEFAULT_PATHEXT = '.com;.exe;.bat;.cmd;.vbs;.vbe;.js;.jse;.wsf;.wsh'; | ||
|
|
||
| // For earlier versions of NodeJS that doesn't have a list of constants (< v6) |
There was a problem hiding this comment.
Could you add a TODO to use the node-provided constant on v6+? We'll eventually drop v4/5.
There was a problem hiding this comment.
Also, I'm trusting that (1) this value is respected on v4, even though the constant does not exist, and (2) the value is treated the same as F_OK on Windows, just as X_OK is documented:
Flag indicating that the file can be executed by the calling process. This has no effect on Windows (will behave like fs.constants.F_OK).
There was a problem hiding this comment.
True, on Windows X_OK and F_OK are equivalent. But on Windows platform file extensions are used to determine if file is executable or not.
I will not do this check for Windows platform.
There was a problem hiding this comment.
I will not do this check for Windows platform.
Sounds good
test/which.js
Outdated
| t.truthy(resultForWhichA); | ||
| t.truthy(resultForWhichA.length); | ||
| t.not(resultForWhich.toString(), matchingFile); | ||
| t.not(resultForWhichA[0], matchingFile); |
There was a problem hiding this comment.
Could you rewrite this as:
t.falsy(resultForWhichA.includes(matchingFile));Also, I think it makes sense to skip this test on Windows, or to assert the array does contain the value on Windows. WDYT?
There was a problem hiding this comment.
includes is not handled by Ava and it is not supported in node@5. I would also save [0] index to make sure that order is proper.
51fd66a to
9157b53
Compare
nfischer
left a comment
There was a problem hiding this comment.
LGTM % comment.
Also, can you check the coverage on your local machine? codecov says isExecutable() is not covered. If this is a codecov bug, can you add /* istanbul ignore next */ and add a comment mentioning codecov is to blame?
test/which.js
Outdated
| t.truthy(resultForWhichA.length); | ||
| t.not(resultForWhich.toString(), matchingFile); | ||
| // TODO(node-support): this can be used starting from node@6: t.falsy(resultForWhichA.includes(matchingFile)) | ||
| t.not(resultForWhichA[0], matchingFile); |
There was a problem hiding this comment.
Sorry, I thought .includes() was added by babel. Could you instead write this as:
t.is(resultForWhichA.indexOf(matchingFile), -1);That way we have the correct semantics for the assertion.
25232fc to
ce70542
Compare
|
@nfischer done |
| t.not(resultForWhich.toString(), matchingFile); | ||
| t.is(resultForWhichA.indexOf(matchingFile), -1); | ||
|
|
||
| process.env.PATH = pathEnv; |
|
Thanks @termosa! |
Fixes #657.