Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix test-require-symlink on Windows #23691

Merged
merged 1 commit into from
Oct 23, 2018
Merged

test: fix test-require-symlink on Windows #23691

merged 1 commit into from
Oct 23, 2018

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Oct 16, 2018

Creating directory symlinks on Windows require 'dir' parameter to be provided.

Fixes: #23596

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@bzoz build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1261/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2018
@bzoz bzoz mentioned this pull request Oct 16, 2018
Closed
@bzoz
Copy link
Contributor Author

bzoz commented Oct 16, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17906/

@refack
Copy link
Contributor

refack commented Oct 16, 2018

Shouldn't we validate the arg and throw, or assume dir?
This exposes an API asymmetry between platforms.
.
.
.
@bzoz could you add a new failing test to known_issues?

@refack refack added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Oct 16, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Oct 16, 2018

@refack the API assumes 'file', using 'dir' makes libuv use SYMBOLIC_LINK_FLAG_DIRECTORY with CreateSymbolicLinkW.

We could add a test to see if link target is a directory and make it automatically use 'dir'.

@refack
Copy link
Contributor

refack commented Oct 16, 2018

We could add a test to see if link target is a directory and make it automatically use 'dir'.

👍 (or throw)

@bzoz bzoz mentioned this pull request Oct 18, 2018
Closed
3 tasks
@bzoz
Copy link
Contributor Author

bzoz commented Oct 23, 2018

Resumed CI, all green: https://ci.nodejs.org/job/node-test-commit/22568/

@refack
Copy link
Contributor

refack commented Oct 23, 2018

So we actually don't get coverage for this in CI because of:
https://github.com/nodejs/node/blob/0cbd76c12530e4296ab1d8543567bcdaca7d37d9/test/parallel/test-require-symlink.js#L5-L6

@refack refack mentioned this pull request Oct 23, 2018
Closed
@bzoz @refack
test: fix test-require-symlink on Windows
d1d5924 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: nodejs#23596

PR-URL: nodejs#23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack refack force-pushed the bartek-fix-test-require-symlink branch from 0cbd76c to d1d5924 Compare October 23, 2018 15:19
@refack refack merged commit d1d5924 into nodejs:master Oct 23, 2018
targos pushed a commit that referenced this pull request Oct 24, 2018
@bzoz @targos
test: fix test-require-symlink on Windows
dfecf85 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@bzoz @MylesBorins
test: fix test-require-symlink on Windows
cdf8933 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
@bzoz @MylesBorins
test: fix test-require-symlink on Windows
d9a08c0 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@bzoz @rvagg
test: fix test-require-symlink on Windows
e7a573a 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@bzoz @rvagg
test: fix test-require-symlink on Windows
b9314eb 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
bzoz added a commit to JaneaSystems/node that referenced this pull request Nov 29, 2018
@bzoz
win, fs: detect if symlink target is a directory
7abe623 
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

Ref: nodejs#23691
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@bzoz @MylesBorins
test: fix test-require-symlink on Windows
9b6b280 
Creating directory symlinks on Windows require 'dir' parameter to be
provided.

Fixes: #23596

PR-URL: #23691
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 29, 2018
@bzoz @Trott
win, fs: detect if symlink target is a directory
cda6b20 
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724
Refs: nodejs#23691
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Merged
This was referenced Dec 18, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@bzoz @refack
win, fs: detect if symlink target is a directory
873f0e7 
On Windows creating a symlink to a directory will not work unless extra
'dir' parameter is passed. This adds a check if link target is a
directory, and if so automatically use 'dir' when creating symlink.

PR-URL: nodejs#23724
Refs: nodejs#23691
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test-require-symlink error on Windows 10
6 participants
@bzoz @nodejs-github-bot @refack @jasnell @cjihrig @MylesBorins