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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions test/fixtures/module-require-symlink/symlinked.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
'use strict';
const assert = require('assert');
const path = require('path');

const foo = require('./foo');
const fixtures = require('../../common/fixtures');

const linkScriptTarget = fixtures.path('module-require-symlink', 'symlinked.js');
const linkScriptEnding = path.join('module-require-symlink', 'symlinked.js');

assert.strictEqual(foo.dep1.bar.version, 'CORRECT_VERSION');
assert.strictEqual(foo.dep2.bar.version, 'CORRECT_VERSION');
assert.strictEqual(__filename, linkScriptTarget);
assert(__filename.endsWith(linkScriptEnding));
assert(__filename in require.cache);
72 changes: 43 additions & 29 deletions test/parallel/test-require-symlink.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,66 @@
// Flags: --preserve-symlinks
'use strict';
const common = require('../common');

if (!common.canCreateSymLink())
common.skip('insufficient privileges');

const assert = require('assert');
const path = require('path');
const { spawn } = require('child_process');
const fs = require('fs');
const { exec, spawn } = require('child_process');
const path = require('path');
const process = require('process');

// Setup: Copy fixtures to tmp directory.

const fixtures = require('../common/fixtures');
const dirName = 'module-require-symlink';
const fixtureSource = fixtures.path(dirName);
const tmpDirTarget = path.join(common.tmpDir, dirName);

// Copy fixtureSource to linkTarget recursively.
common.refreshTmpDir();

const linkTarget = fixtures.path('module-require-symlink',
'node_modules',
'dep2');
function copyDir(source, target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

fs.mkdirSync(target);
fs.readdirSync(source).forEach((entry) => {
const fullPathSource = path.join(source, entry);
const fullPathTarget = path.join(target, entry);
const stats = fs.statSync(fullPathSource);
if (stats.isDirectory()) {
copyDir(fullPathSource, fullPathTarget);
} else {
fs.copyFileSync(fullPathSource, fullPathTarget);
}
});
}

copyDir(fixtureSource, tmpDirTarget);

const linkDir = fixtures.path('module-require-symlink',
'node_modules',
'dep1',
'node_modules',
'dep2');
// Move to tmp dir and do everything with relative paths there so that the test
// doesn't incorrectly fail due to a symlink somewhere else in the absolte path.
process.chdir(common.tmpDir);

const linkScriptTarget = fixtures.path('module-require-symlink',
'symlinked.js');
const linkDir = path.join(dirName,
'node_modules',
'dep1',
'node_modules',
'dep2');

const linkScript = path.join(common.tmpDir, 'module-require-symlink.js');
const linkTarget = path.join('..', '..', 'dep2');

if (common.isWindows) {
// On Windows, creating symlinks requires admin privileges.
// We'll only try to run symlink test if we have enough privileges.
exec('whoami /priv', function(err, o) {
if (err || !o.includes('SeCreateSymbolicLinkPrivilege'))
common.skip('insufficient privileges');
const linkScript = 'linkscript.js';

test();
});
} else {
test();
}
const linkScriptTarget = path.join(dirName, 'symlinked.js');

function test() {
process.on('exit', function() {
fs.unlinkSync(linkDir);
});
test();

function test() {
fs.symlinkSync(linkTarget, linkDir);
fs.symlinkSync(linkScriptTarget, linkScript);

// load symlinked-module
const fooModule = require(fixtures.path('/module-require-symlink/foo.js'));
const fooModule = require(path.join(tmpDirTarget, 'foo.js'));
assert.strictEqual(fooModule.dep1.bar.version, 'CORRECT_VERSION');
assert.strictEqual(fooModule.dep2.bar.version, 'CORRECT_VERSION');

Expand Down