Skip to content

Commit

Permalink
test: fix require-deps-deprecation for installed deps
Browse files Browse the repository at this point in the history
Test test-require-deps-deprecation.js was failing when user already had
node installed with acorn in require.resolve range.

Modified test to acknowledge the possibility and throw only if acorn is
found in the deps directory.

Also changed the deprecation test for v9.x: common.expectWarning was failing
because the required deps now throw ReferenceErrors when not properly called
internally in the right order.

PR-URL: nodejs#17848
Fixes: nodejs#17148
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Tiriel committed Jan 10, 2018
1 parent 3831d87 commit c341f93
Showing 1 changed file with 52 additions and 0 deletions.
52 changes: 52 additions & 0 deletions test/parallel/test-require-deps-deprecation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';

const common = require('../common');
const assert = require('assert');
// The v8 modules when imported leak globals. Disable global check.
common.globalCheck = false;

const deprecatedModules = [
'node-inspect/lib/_inspect',
'node-inspect/lib/internal/inspect_client',
'node-inspect/lib/internal/inspect_repl',
'v8/tools/SourceMap',
'v8/tools/codemap',
'v8/tools/consarray',
'v8/tools/csvparser',
'v8/tools/logreader',
'v8/tools/profile',
'v8/tools/profile_view',
'v8/tools/splaytree',
'v8/tools/tickprocessor',
'v8/tools/tickprocessor-driver'
];

// Newly added deps that do not have a deprecation wrapper around it would
// throw an error, but no warning would be emitted.
const deps = [
'acorn/dist/acorn',
'acorn/dist/walk'
];

const throwingModules = () => {
for (const m of deprecatedModules) {
require(m);
}
};

assert.throws(throwingModules, ReferenceError);

// Instead of checking require, check that resolve isn't pointing toward a
// built-in module, as user might already have node installed with acorn in
// require.resolve range.
// Ref: https://github.com/nodejs/node/issues/17148
for (const m of deps) {
let path;
try {
path = require.resolve(m);
} catch (err) {
assert.ok(err.toString().startsWith('Error: Cannot find module '));
continue;
}
assert.notStrictEqual(path, m);
}

0 comments on commit c341f93

Please sign in to comment.