Add failing test for root + absolute option #294
Conversation
|
Interesting that this passes on Windows. That's extra confusing for me. |
|
I think I have a partial solution (separate commit) but found another problem and added a failing test. |
|
I think this actually might a symptom of a deeper problem with |
|
Rebased with possible fixes. I have a TODO comment because I don't know the answer to:
|
|
Hmm, broke something else on Windows. |
|
Yay, passing tests! I'm not sure if everything I did to get these tests passing is kosher or even that the tests are written correctly, but it seems to be doing the right thing in the few narrow cases I came up with. Can you think of a better way to write the |
| '/Users/phated/node-glob/test/fixtures/a/bc/e', | ||
| '/Users/phated/node-glob/test/fixtures/a/bc/e/f' ] | ||
| */ | ||
| // t.like(matches, [ '/b', '/b/c', '/b/c/d', '/bc', '/bc/e', '/bc/e/f' ].map(function (m) { |
There was a problem hiding this comment.
Ha! Yes, this is because t.like does substring matching. So, t.like('hamburger', 'amburg') passes. Calling t.like(array, otherArray) does a t.like on the elements, so t.like(['hamburger'], ['amburg']) would also pass. tap is working as designed, but I'm using it wrong here.
There was a problem hiding this comment.
alright, I'll update the tests in this file to use t.same
| // t.like(matches, [ '/b', '/b/c', '/b/c/d', '/bc', '/bc/e', '/bc/e/f' ].map(function (m) { | ||
| // return path.join(path.resolve('a'), m).replace(/\\/g, '/') | ||
| // })) | ||
| t.same(matches, [ '/b', '/b/c', '/b/c/d', '/bc', '/bc/e', '/bc/e/f' ].map(function (m) { |
There was a problem hiding this comment.
t.same is the right thing to use here.
| self.cwdAbs = makeAbs(self, self.cwd) | ||
| // TODO: is an absolute `cwd` supposed to be resolved against `root`? | ||
| // e.g. { cwd: '/test', root: __dirname } === path.join(__dirname, '/test') | ||
| self.cwdAbs = isAbsolute(self.cwd) ? self.cwd : makeAbs(self, self.cwd) |
There was a problem hiding this comment.
This seems like a reasonable fix. We do need makeAbs to mount onto the root, if one is specified, so that /a/* maps onto {root}/a/b. But using cwd and root together is just... kinda weird.
There was a problem hiding this comment.
Hmm, how should I go about resolving this then? If makeAbs is used, it appends an absolute path to an absolute root resulting in something like /Users/phated/node-glob/test/Users/phated/node-glob/test/fixtures
|
@isaacs sorry to ping again but I'd like to get this resolved in some way so I can land the usage in |
|
Thanks! this'll go out in the next release. I also wanna fix the thing with the stat errors on Windows, in #245. |
Hey @isaacs - with the
absoluteoption feature, I encountered a problem that I've created a failing test for. I am still digging into a solution but I thought I'd get this test in front of you incase you have a better idea of what is happening.Side note: I have a commented out test using
t.likewhich was passing, not sure if I stumbled on anode-tapbug while working on this.