Test edge cases #82

erikkemperman · 2016-09-19T13:59:21Z

(Don't merge, broken on win32).

I thought I might clean up and contribute some of my stumbling around the edges of the node-glob update and corresponding ignore option shuffle.

This is the syntax, at least if I've now understood previous discussion properly, that users would need for several edge cases. Agree?

Unfortunately it is slightly unpractical for me to debug why this breaks on windows, exactly, but I'd guess that somewhere it treats our escape character as path separator. If so, I think that ought to be addressed?

Travis passes,
Appveyor doesn't.

phated · 2016-09-19T22:57:21Z

Edge cases for the edge-case-god. Let's wait on these until some of the other issues are solved. I might be removing is-absolute-glob once isaacs/node-glob#293 is resolved.

erikkemperman · 2016-09-20T05:14:56Z

Fair enough, that sounds good.

phated · 2017-02-03T01:35:41Z

@erikkemperman could you rebase this and update to the new style of tests in master? Thanks!

erikkemperman · 2017-02-03T13:41:34Z

@phated Yup, will do just as soon as I have some time!

erikkemperman · 2017-02-06T13:10:38Z

@phated Rebased and restyled... Still passing on Travis, but still failing on AppVeyor. Can't dig into that further though, for lack of a windows box.

phated · 2017-02-08T01:32:57Z

Thanks! cc'ing @jonschlinkert and @doowb about this, maybe they can help since we are keeping to-absolute-glob as a dependency.

doowb · 2017-02-08T02:31:33Z

I'll be able to take a look and debug on a windows machine over the next few days.

phated · 2017-02-08T03:23:45Z

@doowb perfect, that's the timeframe I'm looking at for the next glob-stream major.

jonschlinkert · 2017-02-08T19:43:29Z

test/main.js

+    }
+
+    pipe([
+      globStream(['*', '!!(case)'], { cwd: cwd }),


a double ! would be recognized as an inclusive/positive matcher by minimatch. From the minimatch readme

Multiple ! characters at the start of a pattern will negate the pattern multiple times.

edit: for that pattern to work as a negation pattern, I believe you would also need to set nonegate to true. But I'm not completely sure how minimatch works for that

edit2: nvm ignore my comments. I thought the test was saying "should ignore file" but it's saying "should ignore pattern" which seems to be exactly correct. so it's doing the right thing I think.

I'm still reviewing... I haven't seen anything else stand out yet, I'll comment back if I do

From earlier discussion I took this syntax to mean "ignore everything matching this negative extglob", which is what it tests

All of these pass on Travis, but not on Appveyor. I'm thinking slashes get parsed as separators, but am not equipped to debug on windows...

I just finished running the tests on Windows 8, Windows 10 and mac with node.js 6 and the current version, and everything is passing 100%. I also logged out the paths created for the failing unit tests, and everything looks correct to me.

I'm waiting for nvm to finish installing on my windows box so I can test other versions. I'll update here when I get those results.

I don't see the added tests in that output (?)

Thanks, I just noticed the same thing -- but accidentally started a review or something. Tired fingers on tiny mobile screen, sorry about that.

Yeah that was my original guess, slashes parsed as separators on windows. Sorry I'm not able to track this down myself, sounds like I'm claiming a lot of your time...

oh no worries, I don't mind. This windows path stuff can be difficult to debug, I love being a part of a community where other devs can help me figure this stuff out. I'm happy to help where I can

quick update. I tried for a couple of hours to get nvm-windows working on my windows box with no luck. I don't think it was a windows-nvm issue, but I'm going to try again tomorrow and let you know once I get these tests running on different versions. Unless someone has a better suggestion for testing node.js versions on windows.

@jonschlinkert I'm not sure this is a node version issue, it seems to be failing on every version.

Side note: this test seems to be passing on Appveyor, if I'm reading the results correctly.

phated · 2017-02-14T20:02:50Z

@doowb any luck debugging? I'm getting super close to cutting the 6.0 release of glob-stream.

phated · 2017-02-14T20:09:32Z

test/main.js

+    ], done);
+  });
+
+  it('should handle pattern starting with exclam and paren', function(done) {


@jonschlinkert @doowb this seems to be one of the failing tests

phated · 2017-02-14T20:09:50Z

test/main.js

+    ], done);
+  });
+
+  it('should ignore pattern starting with exclam and paren', function(done) {


@jonschlinkert @doowb this seems to be the other failing test

yeah they're both essentially the same issue. I believe node-glob or minimatch strips escaping and/or normalize all \\ to /, so no matter how we do the pattern there it won't work. But I can't get nvm working on windows to confirm.

and if it's not that, then it's probably how the tests are designed. If node-glob doesn't normalize slashes then wouldn't we need to do double backslashes on windows? Otherwise the current escaping would be recognized as a path separator.

I was thinking it might be https://www.npmjs.com/package/glob-parent#windows (mentioned below in the thread)

erikkemperman · 2017-02-14T20:16:25Z

@phated Looks like you renamed test/main.js to test/index.js? Should I rebase, or better to await review/debug?

phated · 2017-02-14T20:17:25Z

@erikkemperman yeah, I reorganized the project. Feel free to rebase if you have time now (otherwise I was going to do it in a branch). I believe master is complete on my end.

phated · 2017-02-14T20:19:17Z

@erikkemperman I wonder if the 2 failing tests are related to https://www.npmjs.com/package/glob-parent#windows - maybe @es128 would know

jonschlinkert · 2017-02-14T20:22:45Z

I wonder if the 2 failing tests are related to https://www.npmjs.com/package/glob-parent#windows - maybe @es128 would know

That's not a glob-parent issue, it's a node-glob issue

@jonschlinkert not quite - remember gulpjs/glob-parent#13

Yes, that's why I just corrected you

phated · 2017-02-14T20:23:35Z

@jonschlinkert not quite - remember gulpjs/glob-parent#13

phated · 2017-02-14T20:25:36Z

@erikkemperman @jonschlinkert @doowb I'm trying the glob-parent thing at 6715d6b to see if anything changes on windows (yay appveyor)

jonschlinkert · 2017-02-14T20:26:37Z

also, fwiw is exactly why I've stated a bunch of times that we should try to do anything at all to work around these backslash issues. I was concerned that it would make it harder for users to figure out where the issue was being caused. But it's making it harder for us to figure it out too.

phated · 2017-02-14T20:27:39Z

I believe I agreed and wanted to tell people to use / only (as node-glob does) but that wasn't desirable from @es128's perspective.

phated · 2017-02-14T20:28:05Z

My test didn't work 😛

jonschlinkert · 2017-02-14T20:28:53Z

I believe I agreed and wanted to tell people to use / only (as node-glob does) but that wasn't desirable from @es128's perspective.

true, good point. I'm not sure what we can do to fix this (other than using a different glob lib)

doowb · 2017-02-14T20:31:28Z

I just logged out the paths in different places in windows and the glob pattern that's being passed to node-glob is C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/\\!(case) which returns:

C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/!(case)
C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/!case
C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/(case)

I'm going to see if "doubling up" the escape characters on windows does anything (like @jonschlinkert mentioned above), but this is something that node-glob isn't handling correctly for windows.

doowb · 2017-02-14T20:34:27Z

Same results when the pattern looks like this: C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/\\\\!(case)

es128 · 2017-02-14T20:36:16Z

I'm fine with ripping out the backslash auto-fix thing with a major version bump if it's causing other problems.

But is that what's happening here?

phated · 2017-02-14T20:36:53Z

@es128 I don't think it's a glob-parent problem because I attempted the ./ prefix thing.

phated · 2017-02-14T20:39:36Z

@doowb interesting!!!! What are the results for the other test because that is getting the inverse and might lead us to the solution

doowb · 2017-02-14T20:44:49Z

I updated the comments above with the correct amount of \ in the glob patterns. One second on the other tests...

erikkemperman · 2017-02-14T20:47:16Z

FWIW I've rebased this branch onto tip of master at https://github.com/erikkemperman/glob-stream/tree/test-edge-cases-rb
Same test results, I've just appended the edge case tests to the new test/index.js.

About the slash/separator ambiguity: remember these tests just reflect my understanding of the syntax emerging from earlier discussion -- so maybe let's take a step back and double check if I am actually testing the desired behaviour? Although they pass on Travis, so there's that :-)

phated · 2017-02-14T20:51:56Z

@erikkemperman I don't know that we've ever talked about parens at the end of a path, I've only ever considered them in the directory position.

Edit: And only in relation to glob-parent and to-absolute-glob, not in relation to node-glob being a terrible library to work with 😛

phated · 2017-02-14T20:53:43Z

@erikkemperman please rebase into this branch. I've also split your tests into a separate describe named "edge cases"

phated · 2017-02-14T20:56:52Z

Thinking more about this, we don't even know if node-glob handles globs that contain ( ) correctly and we don't test that at all (nor should we). The only thing we test is that to-absolute-glob and glob-parent don't see ( ) as glob characters in the directory portion of the glob and return us the end of the pattern. I honestly think we can do away with those tests and maybe all of these edge cases because they are only testing node-glob behavior. @erikkemperman @jonschlinkert @doowb @es128 thoughts?

phated · 2017-02-14T20:58:26Z

minimatch does something with ( but I'm guessing it's wrong on Windows or something like that

doowb · 2017-02-14T20:59:23Z

results of one that's passing:

ourGlob: 'C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/\\!case'
matched: 'C:/Users/brian/work/gulpjs/glob-stream/test/fixtures/edge/!case'

es128 · 2017-02-14T20:59:41Z

Testing things that are the responsibility of your dependencies makes sense to a point so you can easily swap them out in the future. But going deep on contrived edge cases, unless these are cases that have come from real users, may not.

phated · 2017-02-14T21:07:21Z

Removing the 6.0 milestone on this because I think it's going to end up being closed without merging. Let's continue to discuss if anyone has more thoughts on it.

erikkemperman · 2017-02-14T21:08:57Z

Rebased but coming back to this thread it might no longer be needed.

I'm not even sure these are valid file names on Windows, and if so if git handles them correctly. Does checking out this branch on Windows even yield all 4 files in /test/fixtures/edge?

Anyway, fair enough if I'm actually testing the wrong edges :-) Feel free to just close this, or I guess we could skip if on Windows.

phated · 2017-02-14T21:10:05Z

An aside: Maybe we should be testing these cases as directory segments of a glob instead of the glob itself?

erikkemperman · 2017-02-14T21:14:28Z

Well, what I intended to make explicit here is my (mis?)understanding of how parens and/or exclams would be handled at the beginning of a pattern, because the exclam means something in glob-stream that it no longer does in node-glob (ignore option) after recent upgrade.

erikkemperman · 2017-02-14T21:37:12Z

So re: your suggestion of edge case in directory segments, I think the significance of beginning in my previous post is that I'd expect these would apply the same way for !(case)/god et. al. if there were four directories with one file each.

phated · 2017-02-14T22:28:01Z

@erikkemperman I understand that ! means we turn a pattern into an ignore but why test parens too? That doesn't seem necessary if you are trying to test cases negative glob patterns for us, right?

I don't know if ending with vs having the !( ) as a directory path have different results since we resolve to an absolute path before passing to node-glob, but that probably doesn't need to be tested either.

erikkemperman · 2017-02-14T22:38:16Z

@phated IIRC the discussion earlier turned to the potential confusion between a pattern to go into the ignore option of node-glob (with exclam stripped) if a leading exclam preceeds a paren, versus expecting it to go as a regular glob argument unchanged, meaning a negative extglob.

And the (slightly idiotic *) possibility of wanting to match literal exclams and parens in actual paths. Hence the combinations.

But I may very well have things backwards, yay GMT+1.

(*) I've had the misfortune of having to develop for folks who thought such things were perfectly reasonable names for their files.

jonschlinkert · 2017-02-14T22:48:25Z

I honestly think we can do away with those test

totally agree, @es128 said it best

erikkemperman · 2017-02-15T08:11:17Z

Sorry for trailing off there, sleeeep.

So just to reiterate, I think that these tests do belong here, if anywhere, because they make explicit an API difference with latest node-glob. Having said that, they are absolutely far-fetched -- and I am personally never going to run into something like this failing on Windows.

I'll leave it up to @phated to close this PR unmerged, or maybe just skip them on Windows, and apologize to everyone for having wasted your time on this!

phated · 2017-02-15T17:58:50Z

@erikkemperman before we test that, you should create the pattern + ignore tests in node-glob to make sure they are working on Windows. If those tests work and ours don't, only then are we transforming ! + ( ) improperly.

erikkemperman · 2017-02-15T19:55:27Z

@phated That'd be interesting, sure! I'll probably find some time tomorrow. Will have to cheat slightly though, because node-glob has an unrelated failing test on Appveyor -- I'll base this on an open PR.

jonschlinkert · 2017-02-15T21:16:39Z

apologize to everyone for having wasted your time on this!

fwiw I don't feel that way at all. It is what it is. Windows paths have always been difficult to work with, but we have to deal with it and just keep looking for ways to minimize the potential for regressions. I can't speak for anyone else, but I don't know everything there is to know about ~~this~~ anything, These discussions help me learn.

jonschlinkert · 2017-02-15T21:24:43Z

I think that these tests do belong here, if anywhere, because they make explicit an API difference with latest node-glob.

Forgot to mention, I tested the failing patterns directly with node-glob and got the same result that the tests are getting. Both on mac and windows. I'm not sure if you're getting something different with node-glob, but if so I'm at a loss.

erikkemperman · 2017-02-16T14:38:22Z

So, I spent some time with node-glob and minimatch and you were right -- calling into those directly, I see the same 2 cases fail on Windows in much the same way as via glob-stream, while they are working on Linux/Mac..

And I've tracked down that it is indeed because it treats escape chars (backslash) as path separators:

https://github.com/isaacs/minimatch/blob/master/minimatch.js#L123-L125

Several issue reports and proposals to resolve this have been submitted:

isaacs/minimatch#64
isaacs/minimatch#78
isaacs/minimatch#103
(probably more)

But these would not appear to be going anywhere, maintainer is probably too busy with other stuff. I don't think there's much we can do here except hoping Windows users won't want to match literal exclams and parens.

phated · 2017-02-16T16:56:19Z

@erikkemperman @jonschlinkert thanks for testing those cases in node-glob. I think I'm going to close this because it's really out of our control and there isn't a good way for us to test that we are grouping things correctly when glob isn't working correctly.

erikkemperman · 2017-02-16T17:02:58Z

Fair enough! Too bad Isaac doesn't get around to addressing issues very often, or delegate. Escaping is currently just not possible in minimatch on windows!

phated · 2017-02-16T17:07:50Z

@erikkemperman That's exceptionally frightening! I agree with you and wish he'd add some contributors to the projects.

phated force-pushed the master branch from 597a368 to ed580de Compare January 18, 2017 21:17

phated added this to the 6.0 milestone Feb 3, 2017

erikkemperman force-pushed the test-edge-cases branch from a33802b to b6e4955 Compare February 6, 2017 13:01

erikkemperman force-pushed the test-edge-cases branch from b6e4955 to 5fd52e3 Compare February 6, 2017 13:12

jonschlinkert reviewed Feb 8, 2017

View reviewed changes

phated reviewed Feb 14, 2017

View reviewed changes

Upgrade to-absolute-glob; add a bunch of edge case tests

45f112e

phated removed this from the 6.0 milestone Feb 14, 2017

erikkemperman force-pushed the test-edge-cases branch from 5fd52e3 to 45f112e Compare February 14, 2017 21:07

phated closed this Feb 16, 2017

Test edge cases #82

Test edge cases #82

Conversation

erikkemperman commented Sep 19, 2016

phated commented Sep 19, 2016

erikkemperman commented Sep 20, 2016

phated commented Feb 3, 2017

erikkemperman commented Feb 3, 2017

erikkemperman commented Feb 6, 2017

phated commented Feb 8, 2017

doowb commented Feb 8, 2017

phated commented Feb 8, 2017

jonschlinkert Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phated commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikkemperman commented Feb 14, 2017

phated commented Feb 14, 2017

phated commented Feb 14, 2017

jonschlinkert commented Feb 14, 2017 • edited Loading

phated commented Feb 14, 2017

phated commented Feb 14, 2017

jonschlinkert commented Feb 14, 2017

phated commented Feb 14, 2017

phated commented Feb 14, 2017

jonschlinkert commented Feb 14, 2017

doowb commented Feb 14, 2017 • edited Loading

doowb commented Feb 14, 2017 • edited Loading

es128 commented Feb 14, 2017

phated commented Feb 14, 2017

phated commented Feb 14, 2017

doowb commented Feb 14, 2017

erikkemperman commented Feb 14, 2017

phated commented Feb 14, 2017 • edited Loading

phated commented Feb 14, 2017

phated commented Feb 14, 2017

phated commented Feb 14, 2017

doowb commented Feb 14, 2017

es128 commented Feb 14, 2017

phated commented Feb 14, 2017 • edited Loading

erikkemperman commented Feb 14, 2017

phated commented Feb 14, 2017

erikkemperman commented Feb 14, 2017 • edited Loading

erikkemperman commented Feb 14, 2017

phated commented Feb 14, 2017

erikkemperman commented Feb 14, 2017

jonschlinkert commented Feb 14, 2017

erikkemperman commented Feb 15, 2017

phated commented Feb 15, 2017

erikkemperman commented Feb 15, 2017

jonschlinkert commented Feb 15, 2017

jonschlinkert commented Feb 15, 2017

erikkemperman commented Feb 16, 2017

phated commented Feb 16, 2017

erikkemperman commented Feb 16, 2017

phated commented Feb 16, 2017

jonschlinkert Feb 8, 2017 •

edited

Loading

jonschlinkert commented Feb 14, 2017 •

edited

Loading

doowb commented Feb 14, 2017 •

edited

Loading

doowb commented Feb 14, 2017 •

edited

Loading

phated commented Feb 14, 2017 •

edited

Loading

phated commented Feb 14, 2017 •

edited

Loading

erikkemperman commented Feb 14, 2017 •

edited

Loading