Skip to content

Conversation

@sethbrenith
Copy link
Contributor

Attempting to make symlinks as a non-administrator user in Windows
causes a permission error. We need to update test-fs-promises to expect
this error case, as is already done in various other tests.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • fs

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 26, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Can you check that this only happens when common.isWindows === true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, added that check. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have common.canCreateSymLink() for such checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

@Leko
Copy link
Contributor

Leko commented Feb 28, 2018

@Leko
Copy link
Contributor

Leko commented Feb 28, 2018

CI failed but it seems not related to this PR.
I've created an issue #19058

Attempting to make symlinks as a non-administrator user in Windows
causes a permission error. We need to update test-fs-promises to avoid
making symlinks if the platform does not support it.
@sethbrenith sethbrenith changed the title fs: check for EPERM in fs-promises test fs: check for symlink support in fs-promises test Feb 28, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2018

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@Leko
Copy link
Contributor

Leko commented Mar 3, 2018

Landed in 96b2d8d

@Leko Leko closed this Mar 3, 2018
Leko pushed a commit that referenced this pull request Mar 3, 2018
Attempting to make symlinks as a non-administrator user in Windows
causes a permission error. We need to update test-fs-promises to avoid
making symlinks if the platform does not support it.

PR-URL: #19018
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Attempting to make symlinks as a non-administrator user in Windows
causes a permission error. We need to update test-fs-promises to avoid
making symlinks if the platform does not support it.

PR-URL: nodejs#19018
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants