test: do not write fixture in test-require-symlink by Trott · Pull Request #15067 · nodejs/node · GitHub

test-require-symlink modifies the fixture directory by adding a symlink.
Copy the fixture to the test tmpdir instead of modifying the fixture
directory.

Checklist

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

Affected core subsystem(s)

test module

Typo: fixtureSource

This could be dirName or tmpDirTarget.

Wouldn't you rather execSync('cp -Rf Src Trg') / for windows execSync('xcopy /E /Y Src Trg')?

I prefer the more verbose way here because:

there's no difference between Windows and POSIX test code that way
it does not depend on even the most mundane external dependency.

execSync always feels like a code smell to me, although it's almost certainly not going to cause problems here.

I admit my reasons are not super-compelling but I'm also not sure the counter-arguments are super-compelling either. I'll certainly change it if this is a blocking objection. :-)

I'm not too thrilled about this (I know it was like this before). I rather have an emperical test:

const symLinkTest = path.join(common.tmpDir, 'symLinkTest .js');
try {
  fs.symlinkSync(__filename, symLinkTest);
} catch (e) {
  assert.strictEqual(e.code, 'EPERM');
  common.skip('insufficient privileges');
}

Sure, done! (I changed the assert to an if ... throw.)

Does this need to be tested upfront as opposed to wrapping the actual symlink calls in the test in a try-catch block?

Hmmm...maybe even better: It looks like there's a common.canCreateSymlink() used in two files. I can use it here.

This can now be fs.copyFileSync().

Ping @Trott

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

Does this need to be tested upfront as opposed to wrapping the actual symlink calls in the test in a try-catch block?

@refack Is this OK by you as is, or do you object? If you want changes, can you clarify what changes you'd like to see?

@refack Is this OK by you as is, or do you object? If you want changes, can you clarify what changes you'd like to see?

common.canCreateSymlink() ✔️ WinWin
copyDir(source, target) ✔️ I yield to OP

New CI (so this could land as soon as): https://ci.nodejs.org/job/node-test-pull-request/10092/

This failed CI across the board:

not ok 1238 parallel/test-require-symlink
  ---
  duration_ms: 0.532
  severity: fail
  stack: |-
    assert.js:43
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-require-symlink.js:79:12)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at maybeClose (internal/child_process.js:944:16)
        at Socket.stream.socket.on (internal/child_process.js:364:11)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at Pipe._handle.close [as _onclose] (net.js:551:12)

Hmmm...passing for me locally. But obviously not in CI.... will take a look...

Odd. When I log in to a CI machine where it's failing and run the test from the CLI there, it works fine.

su iojs?

@refack Tried both sudo -u iojs a and su - iojs and in both cases, the test still ran fine. 🤔

I added some logging of the error in CI and it seems like linkscript.js can't find ../../common.js but only when it is run from CI.

'module.js:505
    throw err;
    ^

Error: Cannot find module '../../common'
    at Function.Module._resolveFilename (module.js:503:15)
    at Function.Module._load (module.js:452:23)
    at Module.require (module.js:544:17) 
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/data/iojs/node-tmp/tmp.2/module-require-symlink/symlinked.js:2:16)
    at Module._compile (module.js:600:30)
    at Object.Module._extensions..js (module.js:611:10)
    at Module.load (module.js:521:32)
    at tryModuleLoad (module.js:484:12)
    at Function.Module._load (module.js:476:3)

Ah! Looks like symlinking with a full path messes things up because of a symlink higher up in the path. The tmp directories in test are symlinked to /data/iojs/node-tmp and this is no doubt a common pattern in CI (as evidenced by across-the-board failure).

I'll fix the test to use relative paths for symlinking all within the tmp dir.

OK, hopefully that fixes it...

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

Cleaned up the test but missed cleaning up the fixture which still refers to the full path of the tmp directory...

test-require-symlink modifies the fixture directory by adding a symlink. Copy the fixture to the test tmpdir instead of modifying the fixture directory. This also uses a more empirical test for checking for the ability to make symlinks on Windows.

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

🤞

OK, results look much better that time, but a few infra-related issues, some of which are now fixed, so one more time...

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

@Trott the CI seems fine (the linter is failing often... I do not know why but maybe a eslint update might help - or does someone know where the errors might come from?). Is this ready to land?

test-require-symlink modifies the fixture directory by adding a symlink. Copy the fixture to the test tmpdir instead of modifying the fixture directory. This also uses a more empirical test for checking for the ability to make symlinks on Windows. PR-URL: nodejs#15067 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in 6f34076

test-require-symlink modifies the fixture directory by adding a symlink. Copy the fixture to the test tmpdir instead of modifying the fixture directory. This also uses a more empirical test for checking for the ability to make symlinks on Windows. PR-URL: #15067 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

test-require-symlink modifies the fixture directory by adding a symlink. Copy the fixture to the test tmpdir instead of modifying the fixture directory. This also uses a more empirical test for checking for the ability to make symlinks on Windows. PR-URL: nodejs/node#15067 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

Trott added module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests. labels Aug 28, 2017

richardlau reviewed Aug 29, 2017

View reviewed changes

refack suggested changes Aug 29, 2017

View reviewed changes

Trott force-pushed the vacay-1 branch 2 times, most recently from fea8367 to b49c223 Compare August 30, 2017 05:37

richardlau reviewed Sep 8, 2017

View reviewed changes

Trott force-pushed the vacay-1 branch 2 times, most recently from 4383a34 to 3f1c2de Compare September 13, 2017 21:35

richardlau approved these changes Sep 13, 2017

View reviewed changes

Trott force-pushed the vacay-1 branch 2 times, most recently from 6443815 to 59f37e4 Compare September 14, 2017 14:44

jasnell approved these changes Sep 14, 2017

View reviewed changes

refack approved these changes Sep 15, 2017

View reviewed changes

Trott force-pushed the vacay-1 branch from 59f37e4 to f3790ef Compare September 15, 2017 22:04

Trott force-pushed the vacay-1 branch from f3790ef to 8de210f Compare September 15, 2017 23:24

Trott force-pushed the vacay-1 branch from 9000972 to 8de210f Compare September 15, 2017 23:55

Trott force-pushed the vacay-1 branch from 8de210f to ce2cadf Compare September 16, 2017 00:12

Trott closed this Sep 19, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

Trott deleted the vacay-1 branch January 13, 2022 22:46

test: do not write fixture in test-require-symlink by Trott · Pull Request #15067 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: do not write fixture in test-require-symlink - #15067

test: do not write fixture in test-require-symlink#15067Trott wants to merge 1 commit intonodejs:masternodejs/node:masterfrom Trott:vacay-1Trott/io.js:vacay-1Copy head branch name to clipboard

Conversation

Trott commented Aug 28, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 13, 2017

Uh oh!

Trott commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 14, 2017

Uh oh!

refack commented Sep 15, 2017

Uh oh!

refack commented Sep 15, 2017

Uh oh!

jasnell commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

refack commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

Trott commented Sep 15, 2017

Uh oh!

Trott commented Sep 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Sep 16, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

Trott commented Sep 19, 2017

Uh oh!

MylesBorins commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

test: do not write fixture in test-require-symlink#15067
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:vacay-1

Trott commented Sep 15, 2017 •
edited
Loading

Trott commented Sep 16, 2017 •
edited
Loading