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: add support for NODE_TEST_DIR on a separate mount point #21552

Closed
wants to merge 1 commit into from
Closed

test: add support for NODE_TEST_DIR on a separate mount point #21552

wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 26, 2018

Linux permits a filesystem to be mounted at multiple points, but fs.renameSync does not work across different mount points, even if the same filesystem is mounted on both.
This fixes failing tests when NODE_TEST_DIR mount point is different from the one on which the tests are executed (E.G. on a separate partition).

On my configuration, I have the node source on a NTFS partition (dual-boot Windows), and I want to avoid the tests on the chmod to fail (the chmod is partition-wide and set when mounting). This PR allows to run the tests using a different tmpdir without having to move the node source to the same mount point.

NODE_TEST_DIR=/tmp/ make test -j4

Ref: http://man7.org/linux/man-pages/man2/rename.2.html

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@aduh95
test: add support for NODE_TEST_DIR on a separate mount point
cf8a1ec 
Linux permits a filesystem to be mounted at multiple points, but
`fs.renameSync` does not work across different mount points, even if the
same filesystem is mounted on both.
This fixes failing tests when NODE_TEST_DIR mount point is different
from the one on which the tests are executed (E.G. on a separate
partition).

Ref: http://man7.org/linux/man-pages/man2/rename.2.html
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 26, 2018
@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

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

@Trott
Copy link
Member

Trott commented Jun 29, 2018

@nodejs/testing

@bcoe
Copy link
Contributor

bcoe commented Jun 29, 2018

this seems reasonable to me, @cjihrig did you have similar reasoning around the ENOTSUP and ENOTTY checks -- attempt to determine whether copyFileSync could even function on the system before failing the whole test?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 29, 2018

did you have similar reasoning around the ENOTSUP and ENOTTY checks -- attempt to determine whether copyFileSync could even function on the system before failing the whole test?

Yes, specifically the COPYFILE_FICLONE_FORCE flag, which isn't supported everywhere. Unfortunately, different errors are returned depending on the OS. Side note - ENOTTY shouldn't be a valid option anymore, as of libuv 1.21.0.

@richardlau richardlau mentioned this pull request Jul 5, 2018
Closed
@aduh95
Copy link
Contributor Author

aduh95 commented Jul 17, 2018

Has my PR anything to do with intl or is it just a random CI fail? Is there something I can do to fix this fail?

@Trott
Copy link
Member

Trott commented Jul 17, 2018

@nodejs/fs

@Trott
Copy link
Member

Trott commented Jul 17, 2018

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

@Trott
Copy link
Member

Trott commented Jul 17, 2018

Has my PR anything to do with intl or is it just a random CI fail? Is there something I can do to fix this fail?

Probably unrelated to this. Re-running CI now since it's been a while anyway.

Trott pushed a commit to Trott/io.js that referenced this pull request Jul 18, 2018
@aduh95 @Trott
test: add support for NODE_TEST_DIR on a separate mount point
b75bde3 
Linux permits a filesystem to be mounted at multiple points, but
`fs.renameSync` does not work across different mount points, even if the
same filesystem is mounted on both.
This fixes failing tests when NODE_TEST_DIR mount point is different
from the one on which the tests are executed (E.G. on a separate
partition).

Ref: http://man7.org/linux/man-pages/man2/rename.2.html

PR-URL: nodejs#21552
Refs: http://man7.org/linux/man-pages/man2/rename.2.html
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: Ben Coe <bencoe@gmail.com>
@Trott
Copy link
Member

Trott commented Jul 18, 2018

Landed in b75bde3

@Trott Trott closed this Jul 18, 2018
targos pushed a commit that referenced this pull request Jul 18, 2018
@aduh95 @targos
test: add support for NODE_TEST_DIR on a separate mount point
0108ff6 
Linux permits a filesystem to be mounted at multiple points, but
`fs.renameSync` does not work across different mount points, even if the
same filesystem is mounted on both.
This fixes failing tests when NODE_TEST_DIR mount point is different
from the one on which the tests are executed (E.G. on a separate
partition).

Ref: http://man7.org/linux/man-pages/man2/rename.2.html

PR-URL: #21552
Refs: http://man7.org/linux/man-pages/man2/rename.2.html
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: Ben Coe <bencoe@gmail.com>
@targos targos mentioned this pull request Jul 18, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
@ljharb ljharb mentioned this pull request Jun 14, 2019
Closed
5 tasks
@aduh95 aduh95 deleted the test-cross-device branch November 14, 2020 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe approved these changes

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@aduh95 @jasnell @Trott @bcoe @cjihrig @joyeecheung @richardlau @nodejs-github-bot