Test edge cases

[erikkemperman]

(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]

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 https://github.com/isaacs/node-glob/pull/293 is resolved.

[erikkemperman]

Fair enough, that sounds good.

[phated]

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

[erikkemperman]

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

[erikkemperman]

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

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

[doowb]

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

[phated]

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

[phated]

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

[erikkemperman]

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

[phated]

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

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

[jonschlinkert]

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 es128/glob-parent#13

Yes, that's why I just corrected you

[phated]

@jonschlinkert not quite - remember https://github.com/es128/glob-parent/pull/13

[phated]

@erikkemperman @jonschlinkert @doowb I'm trying the glob-parent thing at https://github.com/gulpjs/glob-stream/commit/6715d6b7801c626512f13677a03ccb60377672cb to see if anything changes on windows (yay appveyor)

[jonschlinkert]

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]

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]

My test didn't work 😛

[jonschlinkert]

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]

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]

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

[es128]

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]

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

[jonschlinkert]

But is that what's happening here?

I don't think so. I think it's node-glob.

[es128]

Right, I don't think it applies. But I don't quite understand what the test is trying to test. A directory literally named !(case)? Perhaps \\!\\(case\\)

[phated]

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

[doowb]

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

[erikkemperman]

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]

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

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

[phated]

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]

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

[doowb]

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]

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]

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]

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]

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

[erikkemperman]

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]

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]

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

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

I honestly think we can do away with those test

totally agree, @es128 said it best

[erikkemperman]

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]

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

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

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]

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]

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:

https://github.com/isaacs/minimatch/issues/64 https://github.com/isaacs/minimatch/issues/78 https://github.com/isaacs/minimatch/pull/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]

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

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!