Get pipe tests running on Windows. by binki · Pull Request #550 · shelljs/shelljs

binki · 2016-11-06T05:32:13Z

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).

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).

nfischer

@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.

nfischer · 2016-11-07T05:07:26Z

test/pipe.js

+
+  // 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');


Could we change this to shell.which('find')? That might be more readable.

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.

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?

nfischer · 2016-11-07T05:11:18Z

test/pipe.js

+  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 {


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.

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!

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.

nfischer · 2016-11-07T05:13:24Z

test/pipe.js

    });
  } else {
    console.error('Warning: Cannot verify piped exec');
+    shell.exit(123);


Why was this added? It looks like it should've been here already. Did something change here, or did we just forget this before?

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'.

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.

nfischer · 2016-11-07T05:15:13Z

test/pipe.js

-// TODO: add windows tests
-if (process.platform !== 'win32') {
+if (process.platform === 'win32') {
+  // Windows-specified


nit: you can delete this comment, it's implied by the conditional. Same thing for the section above.

Ah, I was just imitating the // unix-specific comment. I’ll remove both and push a new commit to this PR.

binki · 2016-11-07T15:12:15Z

@nfischer I find ss64 very useful. See FIND. It’s basically like grep(1) (and even the process description in Task Manager mentions grep on modern Windows). An interesting trait of FIND is that it relies on arguments being quoted a specific way, probably for historical reasons. An article which I didn’t fully read but touches on some of those things is “Everyone quotes command line arguments the wrong way”. Basically, on Windows, programs parse the commandline themselves instead of being passed a vector of arguments. Here’s an example:

C:\Users\ohnob>ECHO hi | FIND "hi"
hi

C:\Users\ohnob>ECHO hi | FIND "hey"

C:\Users\ohnob>ECHO hi | FIND hey
FIND: Parameter format not correct

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 .exec(common.nodeBinPath + ' -e "process.stdin.pipe(process.stdout)"') except with some logic that would actually mutate the stream. By just shelling out to node instead of a unix-specific built-in, we could get rid of the whole if (process.platform). Want me to rewrite this PR using this idea instead? ^^

@nfischer

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.

binki · 2016-11-07T19:10:41Z

Cool. I think I’ve addressed all of your outstanding review comments which was just removing the // Windows-specific comment. Please let me know if I missed anything, otherwise I assume you’re waiting for others to look at this and give feedback (if so, sorry for this bugspam xD).

nfischer · 2016-11-07T20:14:31Z

@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 find in a comment in the code? Something like:

Windows `find` is semantically similar to Unix `grep`. It requires that its first argument be
surrounded by double quotes, and it only matches literal strings (not regex).

Add in whatever you think is useful in making the test-case more readable. Thanks!

binki · 2016-11-08T03:26:16Z

Oh, I just noticed #525 now. That sounds like a much cleaner way of achieving the goal of this PR in the end.

nfischer · 2016-11-08T06:58:07Z

test/pipe.js

+
+  // 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') + '"';


@binki Can you move this section out of the conditional? It'll make it easier when we merge this into #512.

nfischer · 2016-11-08T07:28:01Z

@binki I just released the new shx. Would you be interested in refactoring this to use shx grep instead of find? Then we can get rid of the Windows-specific and unix-specific conditionals. That would be even more readable 😄

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.

binki · 2016-11-08T15:25:20Z

Maybe I should rebase to get that ugly FIND stuff out of the history?

nfischer · 2016-11-08T16:23:24Z

Maybe I should rebase to get that ugly FIND stuff out of the history?

I wouldn't worry about it. We squash commits upon merge, so that should take care of it.

LGTM once Travis passes

nfischer · 2016-11-08T16:24:23Z

Fixes #525

binki · 2016-11-09T02:09:32Z

Someone needs to retrigger the travis build because it “errored” rather than “failed” due to badly timed Mac OS X timeouts ;-).

Get pipe tests running on Windows.

4605ce2

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).

nfischer requested changes Nov 7, 2016

View reviewed changes

Remove obvious comments.

4aa6cfa

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.

nfischer self-assigned this Nov 7, 2016

Make FIND usage cleaner and document it.

5680d7b

nfischer reviewed Nov 8, 2016

View reviewed changes

nfischer approved these changes Nov 8, 2016

View reviewed changes

nfischer added the test label Nov 8, 2016

nfischer merged commit 3bc0852 into shelljs:master Nov 9, 2016

nfischer mentioned this pull request Nov 9, 2016

Add shx as a dependency for testing #525

Closed

snyk-bot mentioned this pull request Sep 5, 2019

[Snyk] Upgrade shelljs from 0.7.0 to 0.8.3 MailOnline/mol-cz-lerna-changelog#1

Open

snyk-bot mentioned this pull request Sep 19, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 ali8889/byteball#2

Open

snyk-bot mentioned this pull request Nov 29, 2019

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 sharksandcupcakes/html-inspector#1

Open

snyk-bot mentioned this pull request Mar 8, 2020

[Snyk] Upgrade shelljs from 0.3.0 to 0.8.3 michelvankessel/colored-coins-copay-wallet#1

Open

Conversation

binki commented Nov 6, 2016

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binki Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binki commented Nov 7, 2016

Uh oh!

binki commented Nov 7, 2016

Uh oh!

nfischer commented Nov 7, 2016

Uh oh!

binki commented Nov 8, 2016

Uh oh!

nfischer Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfischer commented Nov 8, 2016

Uh oh!

binki commented Nov 8, 2016

Uh oh!

nfischer commented Nov 8, 2016

Uh oh!

nfischer commented Nov 8, 2016

Uh oh!

binki commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

binki Nov 7, 2016 •

edited

Loading

nfischer Nov 8, 2016 •

edited

Loading