From 029747a819af0313f5c16e4b76c9a5336f907064 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 20 May 2018 21:53:51 -0700 Subject: [PATCH 1/2] test: isolate unusual assert test in its own file test-assert.js contains a test that writes to the source tree, requires an internal module, and depends on modules not needed by the plethora of other test cases in the file. Move it to its own file so that there are not side effects in test-assert.js and so that it can be refactored to not write to the source tree. --- ...ssert-builtins-not-read-from-filesystem.js | 44 +++++++++++++++++++ test/parallel/test-assert.js | 39 ---------------- 2 files changed, 44 insertions(+), 39 deletions(-) create mode 100644 test/parallel/test-assert-builtins-not-read-from-filesystem.js diff --git a/test/parallel/test-assert-builtins-not-read-from-filesystem.js b/test/parallel/test-assert-builtins-not-read-from-filesystem.js new file mode 100644 index 00000000000000..154dc3b33ef87a --- /dev/null +++ b/test/parallel/test-assert-builtins-not-read-from-filesystem.js @@ -0,0 +1,44 @@ +// Flags: --expose-internals + +'use strict'; + +require('../common'); + +const assert = require('assert'); +const EventEmitter = require('events'); +const { errorCache } = require('internal/assert'); +const { writeFileSync, unlinkSync } = require('fs'); + +// Do not read filesystem for error messages in builtin modules. +{ + const e = new EventEmitter(); + + e.on('hello', assert); + + let threw = false; + try { + e.emit('hello', false); + } catch (err) { + const frames = err.stack.split('\n'); + const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/); + // Reset the cache to check again + const size = errorCache.size; + errorCache.delete(`${filename}${line - 1}${column - 1}`); + assert.strictEqual(errorCache.size, size - 1); + const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` + + 'ok(failed(badly));'; + try { + writeFileSync(filename, data); + assert.throws( + () => e.emit('hello', false), + { + message: 'false == true' + } + ); + threw = true; + } finally { + unlinkSync(filename); + } + } + assert(threw); +} diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js index 56489b9e6e8b1c..12ae82475726a0 100644 --- a/test/parallel/test-assert.js +++ b/test/parallel/test-assert.js @@ -19,17 +19,12 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -// Flags: --expose-internals - 'use strict'; /* eslint-disable node-core/prefer-common-expectserror */ const common = require('../common'); const assert = require('assert'); -const EventEmitter = require('events'); -const { errorCache } = require('internal/assert'); -const { writeFileSync, unlinkSync } = require('fs'); const { inspect } = require('util'); const a = assert; @@ -793,40 +788,6 @@ common.expectsError( } ); -// Do not try to check Node.js modules. -{ - const e = new EventEmitter(); - - e.on('hello', assert); - - let threw = false; - try { - e.emit('hello', false); - } catch (err) { - const frames = err.stack.split('\n'); - const [, filename, line, column] = frames[1].match(/\((.+):(\d+):(\d+)\)/); - // Reset the cache to check again - const size = errorCache.size; - errorCache.delete(`${filename}${line - 1}${column - 1}`); - assert.strictEqual(errorCache.size, size - 1); - const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` + - 'ok(failed(badly));'; - try { - writeFileSync(filename, data); - assert.throws( - () => e.emit('hello', false), - { - message: 'false == true' - } - ); - threw = true; - } finally { - unlinkSync(filename); - } - } - assert(threw); -} - common.expectsError( // eslint-disable-next-line no-restricted-syntax () => assert.throws(() => {}, 'Error message', 'message'), From 564ccacd3d4e6bd19937f94110cc772ffc410236 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 20 May 2018 22:19:39 -0700 Subject: [PATCH 2/2] test: improve assert test hygiene Do not pollute the source tree for the test. Instead of writing to the source tree, spawn a process with the temp dir as cwd and write the file there. --- ...ssert-builtins-not-read-from-filesystem.js | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-assert-builtins-not-read-from-filesystem.js b/test/parallel/test-assert-builtins-not-read-from-filesystem.js index 154dc3b33ef87a..000798aca267a3 100644 --- a/test/parallel/test-assert-builtins-not-read-from-filesystem.js +++ b/test/parallel/test-assert-builtins-not-read-from-filesystem.js @@ -1,16 +1,25 @@ -// Flags: --expose-internals - 'use strict'; -require('../common'); +// Do not read filesystem when creating AssertionError messages for code in +// builtin modules. +require('../common'); const assert = require('assert'); -const EventEmitter = require('events'); -const { errorCache } = require('internal/assert'); -const { writeFileSync, unlinkSync } = require('fs'); -// Do not read filesystem for error messages in builtin modules. -{ +if (process.argv[2] !== 'child') { + const tmpdir = require('../common/tmpdir'); + tmpdir.refresh(); + const { spawnSync } = require('child_process'); + const { output, status, error } = + spawnSync(process.execPath, + ['--expose-internals', process.argv[1], 'child'], + { cwd: tmpdir.path, env: process.env }); + assert.ifError(error); + assert.strictEqual(status, 0, `Exit code: ${status}\n${output}`); +} else { + const EventEmitter = require('events'); + const { errorCache } = require('internal/assert'); + const { writeFileSync } = require('fs'); const e = new EventEmitter(); e.on('hello', assert); @@ -27,18 +36,16 @@ const { writeFileSync, unlinkSync } = require('fs'); assert.strictEqual(errorCache.size, size - 1); const data = `${'\n'.repeat(line - 1)}${' '.repeat(column - 1)}` + 'ok(failed(badly));'; - try { - writeFileSync(filename, data); - assert.throws( - () => e.emit('hello', false), - { - message: 'false == true' - } - ); - threw = true; - } finally { - unlinkSync(filename); - } + + writeFileSync(filename, data); + assert.throws( + () => e.emit('hello', false), + { + message: 'false == true' + } + ); + threw = true; + } assert(threw); }