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

Trott · 2017-08-28T23:32:54Z

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

richardlau · 2017-08-29T01:43:19Z

test/parallel/test-require-symlink.js

Typo: fixtureSource

richardlau · 2017-08-29T01:52:34Z

test/parallel/test-require-symlink.js

This could be dirName or tmpDirTarget.

richardlau · 2017-08-29T01:53:03Z

test/parallel/test-require-symlink.js

This could be dirName or tmpDirTarget.

richardlau · 2017-08-29T01:53:13Z

test/parallel/test-require-symlink.js

This could be dirName or tmpDirTarget.

refack · 2017-08-29T01:54:05Z

test/parallel/test-require-symlink.js

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. :-)

refack · 2017-08-29T01:59:21Z

test/parallel/test-require-symlink.js

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.

richardlau · 2017-09-08T17:07:54Z

test/parallel/test-require-symlink.js

This can now be fs.copyFileSync().

BridgeAR · 2017-09-13T18:52:40Z

Ping @Trott

Trott · 2017-09-13T22:17:40Z

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

richardlau · 2017-09-13T22:28:15Z

test/parallel/test-require-symlink.js

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

Trott · 2017-09-14T22:14:43Z

@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 · 2017-09-15T13:42:05Z

@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

refack · 2017-09-15T13:45:16Z

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

jasnell · 2017-09-15T15:58:43Z

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)

Trott · 2017-09-15T22:10:09Z

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

Trott · 2017-09-15T22:15:04Z

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

refack · 2017-09-15T22:15:53Z

su iojs?

Trott · 2017-09-15T22:19:11Z

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

Trott · 2017-09-15T22:50:56Z

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)

Trott · 2017-09-15T22:53:10Z

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.

Trott · 2017-09-15T23:25:50Z

OK, hopefully that fixes it...

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

Trott · 2017-09-15T23:37:53Z

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.

Trott · 2017-09-16T00:17:54Z

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

🤞

Trott · 2017-09-16T03:14:24Z

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/

BridgeAR · 2017-09-19T20:44:59Z

@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>

Trott · 2017-09-19T22:53:45Z

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>

MylesBorins · 2017-10-17T04:10:24Z

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

test/parallel/test-require-symlink.js Outdated

Copy link

Member

richardlau Sep 8, 2017

Choose a reason for hiding this comment

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

This can now be fs.copyFileSync().

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

Uh oh!

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

Uh oh!

6 participants

Trott commented Sep 15, 2017 •

edited

Loading

Trott commented Sep 16, 2017 •

edited

Loading