FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test, win32: fix up symlink tests by digitalinfinity · Pull Request #10477 · nodejs/node · GitHub

/ node Public

test, win32: fix up symlink tests - #10477

Closed
digitalinfinity wants to merge 1 commit into
nodejs:masterfrom
digitalinfinity:symlink_test_fix
ConversationCommits (1)ChecksFiles changed
Closed

test, win32: fix up symlink tests#10477
digitalinfinity wants to merge 1 commit into
nodejs:masterfrom
digitalinfinity:symlink_test_fix

Conversation

digitalinfinity commented Dec 27, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, win32

Description of change

On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:

  1. It assumed that whoami was the one that came with windows.
    However, whoami also ships with Win32 Unix utility ports
    like the distribution with git, which can cause this to get check
    tripped up.
  2. On failure, the check would just return from the callback instead of
    exiting
  3. whoami was executed asynchronously so the test would run regardless
    of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Dec 27, 2016
jasnell approved these changes Dec 27, 2016
View reviewed changes

jasnell left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM if CI is green

mscdex added the windows Issues and PRs related to the Windows platform. label Dec 28, 2016

Trott commented Dec 28, 2016

Copy link
Copy Markdown
Member

@nodejs/testing

joaocgreis approved these changes Dec 29, 2016
View reviewed changes

joaocgreis left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

CI: https://ci.nodejs.org/view/All/job/node-test-pull-request/5622/ (arm failure unrelated)

gibfahn suggested changes Dec 29, 2016
View reviewed changes

gibfahn left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks good, but could you document the new common.canCreateSymlink function in the test README ?

digitalinfinity commented Dec 29, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @gibfahn - pushed another commit adding the documentation. @joaocgreis @jasnell thank you both also for the reviews! Please let me know if there's anything else I need to do.

gibfahn reviewed Dec 30, 2016
View reviewed changes
Comment thread test/README.md Outdated

gibfahn Dec 30, 2016 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: I don't think we'll get that link below 80 chars, but if you put a newline before [SeCreateSymbolicLinkPrivilege] it should help.

Maybe also worth noting that it returns true for non-windows?

gibfahn commented Dec 30, 2016

Copy link
Copy Markdown
Member

I think we normally use test, win: rather than win32 in commit logs (looking back through the commit history).

gibfahn approved these changes Dec 30, 2016
View reviewed changes

gibfahn left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a couple of nits

test, win: fix up symlink tests
7921443 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477

digitalinfinity commented Dec 30, 2016

Copy link
Copy Markdown
Contributor Author

Thanks! Rebased, squashed and fixed the nits. Thanks for the review @gibfahn

gibfahn approved these changes Dec 30, 2016
View reviewed changes

gibfahn left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, CI: https://ci.nodejs.org/job/node-test-commit/6939/

gibfahn self-assigned this Dec 30, 2016
gibfahn pushed a commit that referenced this pull request Dec 31, 2016
test, win: fix up symlink tests
ee9df35 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: #10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

gibfahn commented Dec 31, 2016

Copy link
Copy Markdown
Member

Landed in ee9df35

gibfahn closed this Dec 31, 2016
digitalinfinity deleted the symlink_test_fix branch January 1, 2017 03:01
mscdex added fs Issues and PRs related to the fs subsystem / file system. and removed dont-land-on-v7.x labels Jan 13, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
test, win: fix up symlink tests
d0deebe 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
test, win: fix up symlink tests
8c914ca 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
test, win: fix up symlink tests
bbf6ed2 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
test, win: fix up symlink tests
3adcfbf 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
test, win: fix up symlink tests
1555ced 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
test, win: fix up symlink tests
11ad41c 
On Windows, creating a symlink requires admin privileges.
There were two tests which created symlinks which were failing when run
as non-admin.

test-fs-symlink.js already had a check for privileges on Windows
but it had a couple issues:
1. It assumed that whoami was the one that came with windows.
   However, whoami also ships with Win32 Unix utility ports
   like the distribution with git, which can cause this to get check
   tripped up.
2. On failure, the check would just return from the callback instead of
   exiting
3. whoami was executed asynchronously so the test would run regardless
   of privilege state.

test-fs-options-immutable had no check.

As part of this change, I refactored the privilege checking to
a function in common, and changed both above tests to use the
refactored function.

Also documented this function in test\README.md

PR-URL: nodejs#10477
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+2 more reviewers
joaocgreis joaocgreis approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

gibfahn

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.

8 participants

Back | FazBrowse Home | New Git URL