Fix tests on Windows#297
Conversation
src/ln.js
Outdated
There was a problem hiding this comment.
This was a bug since the last option doesn't exist in linkSync at all.
|
This commit is mostly "if Windows, skip these tests" but unfortunately Windows is not POSIX compatible so certain things don't even work on Windows ( Tests for |
There was a problem hiding this comment.
Is there no way to support links on Windows? If we can't find a way to make ln behave properly on Windows, then we'll need to document it as not running on Windows. It'd be preferable if we could fix the behavior and get at least some links to work.
There was a problem hiding this comment.
Hey, see my other comments - especially #297 (comment)
This is a bug, long existed in shelljs but it is large enough to deserve a separate bug and PR IMO.
There was a problem hiding this comment.
I agree, if you could open a github issue for the bug, that would be nice to have documented. And if we can get that fixed, even better!
There was a problem hiding this comment.
Done: #301.
I'm willing to take this on. Shouldn't be too hard.
|
I'd advocate for merging #299 first to ensure these fixed tests work in AppVeyor before merging. |
|
@nfischer done |
|
@BYK could you rebase off master one more time? That will make sure AppVeyor runs checks on the code |
|
@nfischer done :) |
|
@BYK could you modify
into:
I think the issue is that Windows can't call the .js file directly, while we expect OSX/Linux to be able to. |
|
@BYK Also, awesome job fixing the other tests! It's great to finally have (almost) all the tests passing on Windows. |
|
I'm going to go ahead and merge this, since I tested it in a separate branch, and I tested it rebased off master and it passes appveyor CI now that #315 is merged 👍 |
|
@BYK Thanks again for all your help! This rocks! |
|
@nfischer - glad I could help. I'm planning to help with other Windows issues too. Just don't have a much time. May be over the weekend :) |
Fixes #294 partially and #296.