-
Notifications
You must be signed in to change notification settings - Fork 722
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
Get pipe tests running on Windows. #550
Conversation
I used the FIND command which ships with Windows as something to pass data through. The test checks that a POSIX-like find(1) exists and skips the tests if this condition is true because Windows-provided FIND is incompatible with find(1).
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.
@binki thanks for helping out our tests on Windows! I'm not much of a Windows guy anymore, so I don't pay as much attention to that side of it.
Most of the changes look good. I left some in-line comments for you to address. Would you be able to link me to the semantics of the Windows Find command? I'm unfamiliar with it.
|
||
// Ensure the user does not have the wrong version of FIND. I.e., | ||
// require the following to return error. | ||
result = shell.exec('CD..&FIND . -name no'); |
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.
Could we change this to shell.which('find')
? That might be more readable.
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’m not sure how we would do that. The point is to detect people (like me) who like to put unix utilities such as MSYS2 in PATH
and attempting to npm test
instead of cleaning up their environment first. Basically, the test failure that you would get when you get find(1)
instead of FIND
would be quite confusing, so I think this check is worth it. FIND
will error because the first argument is not wrapped in ""
whereas unix-like find(1)
will exit successfully. Just finding the path to FIND
would return a path. I guess you could do something like trying to determine if the returned path is in %WINDIR%\System32
or not, but that seems a lot more fragile than actually testing the behavior.
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.
Sorry, I didn't explain myself clearly. I mean, use var find = shell.which('find')
to get a path to find. Now replace CD..&FIND
with the find
variable.
You'll still need to check to make sure it's the right find
, but it should still guarantee that it's not referring to the local find.js
. CD..&FIND
is very confusing to non-Windows people, whereas shell.which()
more intuitively returns a path to find
.
Or if there's so much trouble with the find
command on Windows, what if there's a more straightforward alternative?
result = shell.exec('CD..&FIND . -name no'); | ||
if (!shell.error()) { | ||
console.error('Warning: Cannot verify piped exec: Found POSIX-like find(1) in PATH. This test assumes a Windows environment with its FIND command (fix your PATH, exit cygwin/mingw32/MSYS2).'); | ||
} else { |
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.
Is there a clean way to do this without a nested if-else? It makes things a little confusing to read. One preferable alternative would be an else-if ladder.
If we wait for #512 to be merged, then you might be able to simplify this to:
if (!shell.error()) {
console.error('Cannot verify...');
return; // exit early
}
// normal case....
This does not have to be blocked by #512 though if we can do a similarly readable alternative.
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.
Well, could just wrap it all in an IEFE. I was just imitating how the unix code was conditional on grep(1)
, though I inverted the conditional ;-). Also, not sure how one could use an else-if ladder without repeating expressions in the subsequent conditionals. Functions with returns or the current nesting are the natural ways to do this without repetition, I think. Please let me know how I should fix this!
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.
Hmmm... if you think this is the best format, let's leave it for now. When we switch to ava, I think we'll have more flexibility to refactor this for readability.
@@ -76,8 +107,6 @@ if (process.platform !== 'win32') { | |||
}); | |||
} else { | |||
console.error('Warning: Cannot verify piped exec'); | |||
shell.exit(123); |
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.
Why was this added? It looks like it should've been here already. Did something change here, or did we just forget this 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.
I’m going to guess that no one verified that the tests work on unix when grep is not available because, with grep being part of the POSIX spec, it’s hard to find such a unix system. This whole conditional on shell.which('grep').stdout
should probably just be removed because you can assume grep exists when process.platform !== 'win32'
.
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.
Let's leave it, just to be safe. But I agree, grep
is almost certainly available by default, and I would be extremely surprised if anyone intentionally deleted it. I've actually been meaning to refactor this to check for the existence of grep
(windows or otherwise). Not necessary for this PR though.
// TODO: add windows tests | ||
if (process.platform !== 'win32') { | ||
if (process.platform === 'win32') { | ||
// Windows-specified |
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.
nit: you can delete this comment, it's implied by the conditional. Same thing for the section above.
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.
Ah, I was just imitating the // unix-specific
comment. I’ll remove both and push a new commit to this PR.
@nfischer I find ss64 very useful. See
FIND requires its first argument to be passed between double quotes and refuses to run otherwise. It also appears to only support exact substring matching—it doesn’t appear to have a pattern/glob/regex matching option. But for simply testing that piping in shelljs works, it’s sufficient and provided on all Windows machines. However, an alternative approach would be to do something like |
Requested by @nfischer. I removed both the unix-specific and Windows specific comments because I figure they both fall under being too obvious based on being in the process.platform conditional.
Cool. I think I’ve addressed all of your outstanding review comments which was just removing the |
@binki I'll give it another look a bit later and make sure everything is good, and then merge. Would you be able to add a brief explanation of the semantics of windows
Add in whatever you think is useful in making the test-case more readable. Thanks! |
Oh, I just noticed #525 now. That sounds like a much cleaner way of achieving the goal of this PR in the end. |
|
||
// Get the full path to FIND. If we just exec('find'), Windows will | ||
// try to run ./find.js with Windows Script Host. | ||
findCommand = '"' + shell.which('FIND') + '"'; |
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.
@binki I just released the new |
Discussed in shelljs#550. To test piping and its interaction with real processes, a shellout is necessary. But on Windows, you cannot rely on utilities like grep(1) being around. Using shx, we can write portable code. Otherwise, the tests have to be conditional on the platform and be way more complicated.
Maybe I should rebase to get that ugly |
I wouldn't worry about it. We squash commits upon merge, so that should take care of it. LGTM once Travis passes |
Fixes #525 |
Someone needs to retrigger the travis build because it “errored” rather than “failed” due to badly timed Mac OS X timeouts ;-). |
I used the FIND command which ships with Windows as something to
pass data through.
The test checks that a POSIX-like find(1) exists and skips the tests
if this condition is true because Windows-provided FIND is incompatible
with find(1).