Skip to content

Fix tests on Windows#297

Merged
nfischer merged 1 commit intoshelljs:masterfrom
BYK:fix-win-tests
Jan 28, 2016
Merged

Fix tests on Windows#297
nfischer merged 1 commit intoshelljs:masterfrom
BYK:fix-win-tests

Conversation

@BYK
Copy link
Copy Markdown
Contributor

@BYK BYK commented Jan 19, 2016

Fixes #294 partially and #296.

src/ln.js Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug since the last option doesn't exist in linkSync at all.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the docs agree 👍

@BYK
Copy link
Copy Markdown
Contributor Author

BYK commented Jan 19, 2016

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 (chmod for instance). I'll try to make symlinking work properly first and then see if I can get chmod working.

Tests for test -L is also tricky because git on Windows checks symlinks as normal files with symlinked filename as the content making it impossible to use the links in resources directory on Windows. Once/if ln -s is fixed, we may use that to create links and then test them.

@nfischer nfischer added Windows fix Bug/defect, or a fix for such a problem labels Jan 20, 2016
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #301.

I'm willing to take this on. Shouldn't be too hard.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 👍

@nfischer
Copy link
Copy Markdown
Member

@BYK looks like there's some merge conflicts. Could you resolve?

@ariporad Do you think we should merge, or wait until chmod and ln have their Windows-incompatibility documented? This blocks #299, which is a priority.

@mtscout6
Copy link
Copy Markdown

I'd advocate for merging #299 first to ensure these fixed tests work in AppVeyor before merging.

@BYK
Copy link
Copy Markdown
Contributor Author

BYK commented Jan 27, 2016

@nfischer done

@nfischer
Copy link
Copy Markdown
Member

@BYK could you rebase off master one more time? That will make sure AppVeyor runs checks on the code

@BYK
Copy link
Copy Markdown
Contributor Author

BYK commented Jan 27, 2016

@nfischer done :)

@nfischer
Copy link
Copy Markdown
Member

@BYK could you modify test/shjs.js? I think we need to change:

  return shell.exec(path.resolve(__dirname, '../bin/shjs') +
                    ' ' +
                    path.resolve(__dirname, 'resources', 'shjs', name), { silent: true });

into:

  return shell.exec((process.platform === 'win32' ? 'node ' : '') + // call by path for unix, call with 'node' for Windows
                    path.resolve(__dirname, '../bin/shjs') +
                    ' ' +
                    path.resolve(__dirname, 'resources', 'shjs', name), { silent: true });

I think the issue is that Windows can't call the .js file directly, while we expect OSX/Linux to be able to.

@nfischer
Copy link
Copy Markdown
Member

@BYK Also, awesome job fixing the other tests! It's great to finally have (almost) all the tests passing on Windows.

@nfischer
Copy link
Copy Markdown
Member

@BYK or you can just rebase off of the branch in #315 (or off master if it gets merged). This contains the fix I wrote above for the shjs test.

@nfischer
Copy link
Copy Markdown
Member

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 👍

nfischer added a commit that referenced this pull request Jan 28, 2016
@nfischer nfischer merged commit b170d20 into shelljs:master Jan 28, 2016
@nfischer
Copy link
Copy Markdown
Member

@BYK Thanks again for all your help! This rocks!

@nfischer nfischer mentioned this pull request Jan 28, 2016
@BYK BYK deleted the fix-win-tests branch January 28, 2016 17:14
@BYK
Copy link
Copy Markdown
Contributor Author

BYK commented Jan 28, 2016

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug/defect, or a fix for such a problem Windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants