From 53141951abf08c78896da785aa265079c72272f3 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Fri, 1 Dec 2017 00:39:13 +0100 Subject: [PATCH 1/4] fix #121 --- lib/tmp.js | 19 ++++++++- test/child-process.js | 83 +++++++++++++++++++------------------- test/issue121-test.js | 27 +++++++++++++ test/outband/issue121.js | 20 +++++++++ test/outband/issue121.json | 3 ++ test/spawn-custom.js | 8 +++- test/spawn.js | 2 +- 7 files changed, 117 insertions(+), 45 deletions(-) create mode 100644 test/issue121-test.js create mode 100644 test/outband/issue121.js create mode 100644 test/outband/issue121.json diff --git a/lib/tmp.js b/lib/tmp.js index d8826bb..8fe48a0 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -593,6 +593,24 @@ function _safely_install_listener() { } } + // windows does not support signals + // it'd never had won if it wasn't a major PITA + // with node v8.x and win 10 this is no longer an issue + if (process.platform == 'win32') { + var rl = require('readline').createInterface({ + input: process.stdin, + output: process.stdout + }); + + rl.on('SIGINT', function () { + process.emit('SIGINT'); + }); + } + + process.on('SIGINT', function () { + process.exit(0); + }); + process.addListener(EVENT, function _tmp$safe_listener(data) { /* istanbul ignore else */ if (existingListeners.length) { @@ -606,7 +624,6 @@ function _safely_install_listener() { _safely_install_listener(); - /** * Configuration options. * diff --git a/test/child-process.js b/test/child-process.js index 3dd0dde..c7f0e49 100644 --- a/test/child-process.js +++ b/test/child-process.js @@ -1,82 +1,81 @@ // vim: expandtab:ts=2:sw=2 -var +const fs = require('fs'), path = require('path'), - exists = fs.exists || path.exists, + existsSync = fs.existsSync || path.existsSync, spawn = require('child_process').spawn; -const ISTANBUL_PATH = path.join(__dirname, '..', 'node_modules', 'istanbul', 'lib', 'cli.js'); -module.exports.genericChildProcess = _spawnProcess('spawn-generic.js'); -module.exports.childProcess = _spawnProcess('spawn-custom.js'); +module.exports.genericChildProcess = function spawnGenericChildProcess(configFile, cb) { + const + configFilePath = path.join(__dirname, 'outband', configFile), + command_args = [path.join(__dirname, 'spawn-generic.js'), configFilePath]; -function _spawnProcess(spawnFile) { - return function (testCase, configFile, cb) { - testCase.timeout(5000); + // make sure that the config file exists + if (!existsSync(configFilePath)) + return cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); - var - configFilePath = path.join(__dirname, 'outband', configFile), - commandArgs = [path.join(__dirname, spawnFile), configFilePath]; + _do_spawn(command_args, cb); +}; - exists(configFilePath, function (configExists) { - if (configExists) return _doSpawn(commandArgs, cb); +module.exports.childProcess = function spawnChildProcess(configFile, cb, detach) { + var + configFilePath = path.join(__dirname, 'outband', configFile), + command_args = [path.join(__dirname, 'spawn-custom.js'), configFilePath]; + + // make sure that the config file exists + if (!existsSync(configFilePath)) + return cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); + + if (arguments.length > 2) { + for (var i=2; i < arguments.length; i++) { + command_args.push(arguments[i]); + } + } - cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); - }); - }; + _do_spawn(command_args, cb, detach); } -function _doSpawn(commandArgs, cb) { - var +function _do_spawn(command_args, cb, detach) { + const node_path = process.argv[0], stdoutBufs = [], - stderrBufs = [], + stderrBufs = []; + + var child, done = false, stderrDone = false, stdoutDone = false; - if (process.env.running_under_istanbul) { - commandArgs = [ - ISTANBUL_PATH, 'cover', '--report' , 'none', '--print', 'none', - '--dir', path.join('coverage', 'json'), '--include-pid', - commandArgs[0], '--', commandArgs[1] - ]; - } - // spawn doesn’t have the quoting problems that exec does, // especially when going for Windows portability. - child = spawn(node_path, commandArgs); + child = spawn(node_path, command_args, detach ? { detached: true } : undefined); child.stdin.end(); - - // TODO we no longer support node 0.6 + // TODO:we no longer support node <0.10.0 // Cannot use 'close' event because not on node-0.6. function _close() { - var + const stderr = _bufferConcat(stderrBufs).toString(), stdout = _bufferConcat(stdoutBufs).toString(); - if (stderrDone && stdoutDone && !done) { done = true; cb(null, stderr, stdout); } } - child.on('error', function _spawnError(err) { if (!done) { done = true; cb(err); } }); - child.stdout.on('data', function _stdoutData(data) { stdoutBufs.push(data); }).on('close', function _stdoutEnd() { stdoutDone = true; _close(); }); - child.stderr.on('data', function _stderrData(data) { stderrBufs.push(data); }).on('close', function _stderrEnd() { @@ -88,13 +87,13 @@ function _doSpawn(commandArgs, cb) { function _bufferConcat(buffers) { if (Buffer.concat) { return Buffer.concat.apply(this, arguments); + } else { + return new Buffer(buffers.reduce(function (acc, buf) { + for (var i = 0; i < buf.length; i++) { + acc.push(buf[i]); + } + return acc; + }, [])); } - - return new Buffer(buffers.reduce(function (acc, buf) { - for (var i = 0; i < buf.length; i++) { - acc.push(buf[i]); - } - return acc; - }, [])); } diff --git a/test/issue121-test.js b/test/issue121-test.js new file mode 100644 index 0000000..e723720 --- /dev/null +++ b/test/issue121-test.js @@ -0,0 +1,27 @@ +/* eslint-disable no-octal */ +// vim: expandtab:ts=2:sw=2 + +const + assert = require('assert'), + assertions = require('./assertions'), + childProcess = require('./child-process').childProcess, + signals = ['SIGINT', 'SIGTERM']; + +describe('tmp', function () { + describe('issue121 - clean up on terminating signals', function () { + for (var i=0; i < signals.length; i++) { + it(signals[i], issue121Tests(signals[i])); + } + }); +}); + +function issue121Tests(signal) { + return function (done) { + childProcess('issue121.json', function (err, stderr, stdout) { + if (err) return done(err); + else if (stderr) return done(new Error(stderr)); + else assertions.assertDoesNotExist(stdout); + done(); + }, true); + }; +} diff --git a/test/outband/issue121.js b/test/outband/issue121.js new file mode 100644 index 0000000..0dce377 --- /dev/null +++ b/test/outband/issue121.js @@ -0,0 +1,20 @@ +/* eslint-disable no-octal */ +// vim: expandtab:ts=2:sw=2 + +var + fs = require('fs'), + tmp = require('../../lib/tmp'), + // we reuse the fixtures from issue62 here + fixture = require('./issue62'); + +tmp.setGracefulCleanup(); + +// https://github.com/raszi/node-tmp/issues/121 +module.exports = function (signal) { + fixture.apply(this, [tmp.dirSync({ unsafeCleanup: true }), tmp]); + + // make sure that the process keeps running + setTimeout(function () {}, 1000000); + + this.kill(signal); +}; diff --git a/test/outband/issue121.json b/test/outband/issue121.json new file mode 100644 index 0000000..86429e8 --- /dev/null +++ b/test/outband/issue121.json @@ -0,0 +1,3 @@ +{ + "tc": "issue121" +} diff --git a/test/spawn-custom.js b/test/spawn-custom.js index bb1cc27..d450635 100644 --- a/test/spawn-custom.js +++ b/test/spawn-custom.js @@ -8,7 +8,13 @@ var var config = readJsonConfig(process.argv[2]); spawn.graceful = !!config.graceful; +var args = []; + +for (var i=3; i Date: Thu, 7 Dec 2017 22:02:04 +0100 Subject: [PATCH 2/4] cleaning up --- test/assertions.js | 6 +++--- test/dir-sync-test.js | 2 +- test/dir-test.js | 4 ++-- test/file-sync-test.js | 2 +- test/inband-standard.js | 4 ++-- test/issue129-test.js | 2 +- test/name-sync-test.js | 2 +- test/name-test.js | 4 ++-- test/spawn-custom.js | 14 +++++--------- test/spawn-generic.js | 8 ++++---- test/spawn.js | 2 +- test/util.js | 4 ++-- 12 files changed, 25 insertions(+), 29 deletions(-) diff --git a/test/assertions.js b/test/assertions.js index 8e7a246..e734fd8 100644 --- a/test/assertions.js +++ b/test/assertions.js @@ -1,6 +1,6 @@ /* eslint-disable no-octal */ -var +const assert = require('assert'), fs = require('fs'), path = require('path'), @@ -18,7 +18,7 @@ module.exports.assertName = function assertName(name, expected) { module.exports.assertMode = function assertMode(name, mode) { - var stat = fs.statSync(name); + const stat = fs.statSync(name); // mode values do not work properly on Windows. Ignore “group” and // “other” bits then. Ignore execute bit on that platform because it @@ -57,7 +57,7 @@ module.exports.assertProperResult = function assertProperResult(result, withfd) module.exports.assertExists = function assertExists(name, isfile) { assert.ok(existsSync(name), name + ' should exist'); - var stat = fs.statSync(name); + const stat = fs.statSync(name); if (isfile) assert.ok(stat.isFile(), name + ' should be a file'); else assert.ok(stat.isDirectory(), name + ' should be a directory'); }; diff --git a/test/dir-sync-test.js b/test/dir-sync-test.js index 343e442..4eda836 100644 --- a/test/dir-sync-test.js +++ b/test/dir-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), fs = require('fs'), path = require('path'), diff --git a/test/dir-test.js b/test/dir-test.js index 406175a..cf1599b 100644 --- a/test/dir-test.js +++ b/test/dir-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), fs = require('fs'), path = require('path'), @@ -20,7 +20,7 @@ describe('tmp', function () { describe('#dir()', function () { describe('when running inband standard tests', function () { inbandStandardTests(false, function before(done) { - var that = this; + const that = this; tmp.dir(this.opts, function (err, name, removeCallback) { if (err) return done(err); that.topic = { name: name, removeCallback: removeCallback }; diff --git a/test/file-sync-test.js b/test/file-sync-test.js index 52689c5..21d0781 100644 --- a/test/file-sync-test.js +++ b/test/file-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), fs = require('fs'), inbandStandardTests = require('./inband-standard'), diff --git a/test/inband-standard.js b/test/inband-standard.js index f56b457..7c5c0e8 100644 --- a/test/inband-standard.js +++ b/test/inband-standard.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), fs = require('fs'), path = require('path'), @@ -12,7 +12,7 @@ var module.exports = function inbandStandard(isFile, beforeHook) { - var testMode = isFile ? 0600 : 0700; + const testMode = isFile ? 0600 : 0700; describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook)); describe('with prefix', inbandStandardTests({ mode: testMode }, { prefix: 'something' }, isFile, beforeHook)); describe('with postfix', inbandStandardTests({ mode: testMode }, { postfix: '.txt' }, isFile, beforeHook)); diff --git a/test/issue129-test.js b/test/issue129-test.js index c157cb3..253c326 100644 --- a/test/issue129-test.js +++ b/test/issue129-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), assertions = require('./assertions'), childProcess = require('./child-process').childProcess; diff --git a/test/name-sync-test.js b/test/name-sync-test.js index e784668..7aa312b 100644 --- a/test/name-sync-test.js +++ b/test/name-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); diff --git a/test/name-test.js b/test/name-test.js index 564ad32..4eaf5c4 100644 --- a/test/name-test.js +++ b/test/name-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); @@ -11,7 +11,7 @@ describe('tmp', function () { describe('#tmpName()', function () { describe('when running inband standard tests', function () { inbandStandardTests(function before(done) { - var that = this; + const that = this; tmp.dir(this.opts, function (err, name) { if (err) return done(err); that.topic = name; diff --git a/test/spawn-custom.js b/test/spawn-custom.js index d450635..cc0a1d2 100644 --- a/test/spawn-custom.js +++ b/test/spawn-custom.js @@ -1,20 +1,16 @@ // vim: expandtab:ts=2:sw=2 -var +const path = require('path'), readJsonConfig = require('./util').readJsonConfig, - spawn = require('./spawn'); + spawn = require('./spawn'), + config = readJsonConfig(process.argv[2]); -var config = readJsonConfig(process.argv[2]); spawn.graceful = !!config.graceful; -var args = []; - -for (var i=3; i Date: Wed, 27 Dec 2017 02:29:02 +0100 Subject: [PATCH 3/4] merge rebase to master --- package.json | 2 +- test/assertions.js | 6 +-- test/child-process.js | 83 ++++++++++++++++++++--------------------- test/dir-sync-test.js | 7 ++-- test/dir-test.js | 4 +- test/file-sync-test.js | 2 +- test/inband-standard.js | 4 +- test/issue121-test.js | 2 +- test/issue129-test.js | 2 +- test/name-sync-test.js | 2 +- test/name-test.js | 4 +- test/spawn-custom.js | 12 +++--- test/spawn-generic.js | 8 ++-- test/util.js | 4 +- 14 files changed, 70 insertions(+), 72 deletions(-) diff --git a/package.json b/package.json index fd60e8c..0c178bc 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "scripts": { "lint": "eslint lib --env mocha test", "clean": "rm -Rf ./coverage", - "test": "istanbul cover ./node_modules/mocha/bin/_mocha --report none --print none --dir ./coverage/json -- -u exports test/*-test.js && istanbul report --root ./coverage/json html && istanbul report text-summary", + "test": "npm run clean && istanbul cover ./node_modules/mocha/bin/_mocha --report none --print none --dir ./coverage/json -u exports -R test/*-test.js && istanbul report --root ./coverage/json html && istanbul report text-summary", "doc": "jsdoc -c .jsdoc.json" } } diff --git a/test/assertions.js b/test/assertions.js index e734fd8..8e7a246 100644 --- a/test/assertions.js +++ b/test/assertions.js @@ -1,6 +1,6 @@ /* eslint-disable no-octal */ -const +var assert = require('assert'), fs = require('fs'), path = require('path'), @@ -18,7 +18,7 @@ module.exports.assertName = function assertName(name, expected) { module.exports.assertMode = function assertMode(name, mode) { - const stat = fs.statSync(name); + var stat = fs.statSync(name); // mode values do not work properly on Windows. Ignore “group” and // “other” bits then. Ignore execute bit on that platform because it @@ -57,7 +57,7 @@ module.exports.assertProperResult = function assertProperResult(result, withfd) module.exports.assertExists = function assertExists(name, isfile) { assert.ok(existsSync(name), name + ' should exist'); - const stat = fs.statSync(name); + var stat = fs.statSync(name); if (isfile) assert.ok(stat.isFile(), name + ' should be a file'); else assert.ok(stat.isDirectory(), name + ' should be a directory'); }; diff --git a/test/child-process.js b/test/child-process.js index c7f0e49..ce189e2 100644 --- a/test/child-process.js +++ b/test/child-process.js @@ -1,81 +1,80 @@ // vim: expandtab:ts=2:sw=2 -const +var fs = require('fs'), path = require('path'), - existsSync = fs.existsSync || path.existsSync, + exists = fs.exists || path.exists, spawn = require('child_process').spawn; +const ISTANBUL_PATH = path.join(__dirname, '..', 'node_modules', 'istanbul', 'lib', 'cli.js'); -module.exports.genericChildProcess = function spawnGenericChildProcess(configFile, cb) { - const - configFilePath = path.join(__dirname, 'outband', configFile), - command_args = [path.join(__dirname, 'spawn-generic.js'), configFilePath]; +module.exports.genericChildProcess = _spawnProcess('spawn-generic.js'); +module.exports.childProcess = _spawnProcess('spawn-custom.js'); - // make sure that the config file exists - if (!existsSync(configFilePath)) - return cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); +function _spawnProcess(spawnFile) { + return function (testCase, configFile, cb) { + var + configFilePath = path.join(__dirname, 'outband', configFile), + commandArgs = [path.join(__dirname, spawnFile), configFilePath]; - _do_spawn(command_args, cb); -}; + exists(configFilePath, function (configExists) { + if (configExists) return _doSpawn(commandArgs, cb); -module.exports.childProcess = function spawnChildProcess(configFile, cb, detach) { - var - configFilePath = path.join(__dirname, 'outband', configFile), - command_args = [path.join(__dirname, 'spawn-custom.js'), configFilePath]; - - // make sure that the config file exists - if (!existsSync(configFilePath)) - return cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); - - if (arguments.length > 2) { - for (var i=2; i < arguments.length; i++) { - command_args.push(arguments[i]); - } - } - - _do_spawn(command_args, cb, detach); + cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); + }); + }; } -function _do_spawn(command_args, cb, detach) { - const +function _doSpawn(commandArgs, cb) { + var node_path = process.argv[0], stdoutBufs = [], - stderrBufs = []; - - var + stderrBufs = [], child, done = false, stderrDone = false, stdoutDone = false; + if (process.env.running_under_istanbul) { + commandArgs = [ + ISTANBUL_PATH, 'cover', '--report' , 'none', '--print', 'none', + '--dir', path.join('coverage', 'json'), '--include-pid', + commandArgs[0], '--', commandArgs[1] + ]; + } + // spawn doesn’t have the quoting problems that exec does, // especially when going for Windows portability. - child = spawn(node_path, command_args, detach ? { detached: true } : undefined); + child = spawn(node_path, commandArgs); child.stdin.end(); - // TODO:we no longer support node <0.10.0 + + // TODO we no longer support node 0.6 // Cannot use 'close' event because not on node-0.6. function _close() { - const + var stderr = _bufferConcat(stderrBufs).toString(), stdout = _bufferConcat(stdoutBufs).toString(); + if (stderrDone && stdoutDone && !done) { done = true; cb(null, stderr, stdout); } } + child.on('error', function _spawnError(err) { if (!done) { done = true; cb(err); } }); + child.stdout.on('data', function _stdoutData(data) { stdoutBufs.push(data); }).on('close', function _stdoutEnd() { stdoutDone = true; _close(); }); + child.stderr.on('data', function _stderrData(data) { stderrBufs.push(data); }).on('close', function _stderrEnd() { @@ -87,13 +86,13 @@ function _do_spawn(command_args, cb, detach) { function _bufferConcat(buffers) { if (Buffer.concat) { return Buffer.concat.apply(this, arguments); - } else { - return new Buffer(buffers.reduce(function (acc, buf) { - for (var i = 0; i < buf.length; i++) { - acc.push(buf[i]); - } - return acc; - }, [])); } + + return new Buffer(buffers.reduce(function (acc, buf) { + for (var i = 0; i < buf.length; i++) { + acc.push(buf[i]); + } + return acc; + }, [])); } diff --git a/test/dir-sync-test.js b/test/dir-sync-test.js index 4eda836..7afe4cc 100644 --- a/test/dir-sync-test.js +++ b/test/dir-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), fs = require('fs'), path = require('path'), @@ -125,10 +125,11 @@ describe('tmp', function () { try { assertions.assertExists(stdout); assertions.assertExists(path.join(stdout, 'should-be-removed.file'), true); - if (process.platform == 'win32') + if (process.platform == 'win32') { assertions.assertExists(path.join(stdout, 'symlinkme-target'), true); - else + } else { assertions.assertExists(path.join(stdout, 'symlinkme-target')); + } } catch (err) { rimraf.sync(stdout); return done(err); diff --git a/test/dir-test.js b/test/dir-test.js index cf1599b..406175a 100644 --- a/test/dir-test.js +++ b/test/dir-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), fs = require('fs'), path = require('path'), @@ -20,7 +20,7 @@ describe('tmp', function () { describe('#dir()', function () { describe('when running inband standard tests', function () { inbandStandardTests(false, function before(done) { - const that = this; + var that = this; tmp.dir(this.opts, function (err, name, removeCallback) { if (err) return done(err); that.topic = { name: name, removeCallback: removeCallback }; diff --git a/test/file-sync-test.js b/test/file-sync-test.js index 21d0781..52689c5 100644 --- a/test/file-sync-test.js +++ b/test/file-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), fs = require('fs'), inbandStandardTests = require('./inband-standard'), diff --git a/test/inband-standard.js b/test/inband-standard.js index 7c5c0e8..f56b457 100644 --- a/test/inband-standard.js +++ b/test/inband-standard.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), fs = require('fs'), path = require('path'), @@ -12,7 +12,7 @@ const module.exports = function inbandStandard(isFile, beforeHook) { - const testMode = isFile ? 0600 : 0700; + var testMode = isFile ? 0600 : 0700; describe('without any parameters', inbandStandardTests({ mode: testMode, prefix: 'tmp-' }, null, isFile, beforeHook)); describe('with prefix', inbandStandardTests({ mode: testMode }, { prefix: 'something' }, isFile, beforeHook)); describe('with postfix', inbandStandardTests({ mode: testMode }, { postfix: '.txt' }, isFile, beforeHook)); diff --git a/test/issue121-test.js b/test/issue121-test.js index e723720..69a24f3 100644 --- a/test/issue121-test.js +++ b/test/issue121-test.js @@ -10,7 +10,7 @@ const describe('tmp', function () { describe('issue121 - clean up on terminating signals', function () { for (var i=0; i < signals.length; i++) { - it(signals[i], issue121Tests(signals[i])); + issue121Tests(signals[i]); } }); }); diff --git a/test/issue129-test.js b/test/issue129-test.js index 253c326..c157cb3 100644 --- a/test/issue129-test.js +++ b/test/issue129-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), assertions = require('./assertions'), childProcess = require('./child-process').childProcess; diff --git a/test/name-sync-test.js b/test/name-sync-test.js index 7aa312b..e784668 100644 --- a/test/name-sync-test.js +++ b/test/name-sync-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); diff --git a/test/name-test.js b/test/name-test.js index 4eaf5c4..564ad32 100644 --- a/test/name-test.js +++ b/test/name-test.js @@ -1,7 +1,7 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -const +var assert = require('assert'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); @@ -11,7 +11,7 @@ describe('tmp', function () { describe('#tmpName()', function () { describe('when running inband standard tests', function () { inbandStandardTests(function before(done) { - const that = this; + var that = this; tmp.dir(this.opts, function (err, name) { if (err) return done(err); that.topic = name; diff --git a/test/spawn-custom.js b/test/spawn-custom.js index cc0a1d2..bb1cc27 100644 --- a/test/spawn-custom.js +++ b/test/spawn-custom.js @@ -1,16 +1,14 @@ // vim: expandtab:ts=2:sw=2 -const +var path = require('path'), readJsonConfig = require('./util').readJsonConfig, - spawn = require('./spawn'), - config = readJsonConfig(process.argv[2]); + spawn = require('./spawn'); +var config = readJsonConfig(process.argv[2]); spawn.graceful = !!config.graceful; -const args = Array.prototype.slice.call(process.argv, 3); - // import the test case function and execute it -const fn = require(path.join(__dirname, 'outband', config.tc)); -fn.apply(spawn, args); +var fn = require(path.join(__dirname, 'outband', config.tc)); +fn.apply(spawn); diff --git a/test/spawn-generic.js b/test/spawn-generic.js index 85001fe..6b4cb5b 100644 --- a/test/spawn-generic.js +++ b/test/spawn-generic.js @@ -1,12 +1,12 @@ // vim: expandtab:ts=2:sw=2 -const +var path = require('path'), readJsonConfig = require('./util').readJsonConfig, spawn = require('./spawn'), - tmp = require('../lib/tmp'), - config = readJsonConfig(process.argv[2]); + tmp = require('../lib/tmp'); +var config = readJsonConfig(process.argv[2]); spawn.graceful = !!config.graceful; var fnUnderTest = null; @@ -18,7 +18,7 @@ else fnUnderTest = (config.file) ? tmp.fileSync : tmp.dirSync; if (config.graceful) tmp.setGracefulCleanup(); // import the test case function and execute it -const fn = require(path.join(__dirname, 'outband', config.tc)); +var fn = require(path.join(__dirname, 'outband', config.tc)); if (config.async) fnUnderTest(config.options, function (err, name, fdOrCallback, cb) { if (err) spawn.err(err); diff --git a/test/util.js b/test/util.js index 4369bcb..478148c 100644 --- a/test/util.js +++ b/test/util.js @@ -1,9 +1,9 @@ // vim: expandtab:ts=2:sw=2 -const +var fs = require('fs'); module.exports.readJsonConfig = function readJsonConfig(path) { - const contents = fs.readFileSync(path); + var contents = fs.readFileSync(path); return JSON.parse(contents); }; From bd853cb9b19585ea8b04ace3ed77ae2a91e53755 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Wed, 12 Sep 2018 18:11:08 +0200 Subject: [PATCH 4/4] fix test for #121 dropping support for node v6.x as it is not working correctly with the installed SIGINT handlers add appveyor build for node 11 --- .eslintrc.js | 3 +- .travis.yml | 1 - appveyor.yml | 2 +- lib/tmp.js | 107 +++++++++++++++++++----------- test/child-process.js | 32 +++++---- test/issue121-test.js | 45 ++++++++++--- test/issue129-sigint-test.js | 34 ++++++++++ test/outband/issue121.js | 23 +++---- test/outband/issue121.json | 1 + test/outband/issue129-sigint.js | 38 +++++++++++ test/outband/issue129-sigint.json | 3 + test/outband/issue129.js | 12 ++-- test/spawn-custom.js | 1 - test/spawn-generic.js | 2 +- test/spawn.js | 3 - 15 files changed, 219 insertions(+), 88 deletions(-) create mode 100644 test/issue129-sigint-test.js create mode 100644 test/outband/issue129-sigint.js create mode 100644 test/outband/issue129-sigint.json diff --git a/.eslintrc.js b/.eslintrc.js index 45d1d61..43ed1a1 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -23,6 +23,7 @@ module.exports = { "semi": [ "error", "always" - ] + ], + "no-extra-boolean-cast": 0 } }; diff --git a/.travis.yml b/.travis.yml index fb0b545..f3dde6f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,7 +4,6 @@ node_js: - "10" - "9" - "8" - - "6" sudo: false cache: directories: diff --git a/appveyor.yml b/appveyor.yml index 15a83a7..743d881 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -2,11 +2,11 @@ environment: matrix: - - nodejs_version: "6" - nodejs_version: "7" - nodejs_version: "8" - nodejs_version: "9" - nodejs_version: "10" + - nodejs_version: "11" install: - ps: Install-Product node $env:nodejs_version diff --git a/lib/tmp.js b/lib/tmp.js index 8fe48a0..33d59bd 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -43,7 +43,9 @@ const DIR_MODE = 448 /* 0o700 */, FILE_MODE = 384 /* 0o600 */, - EVENT = 'exit', + EXIT = 'exit', + + SIGINT = 'SIGINT', // this will hold the objects need to be removed on exit _removeObjects = []; @@ -101,7 +103,7 @@ function _isUndefined(obj) { */ function _parseArguments(options, callback) { /* istanbul ignore else */ - if (typeof options == 'function') { + if (typeof options === 'function') { return [{}, options]; } @@ -132,7 +134,7 @@ function _generateTmpName(opts) { var template = opts.template; // make sure that we prepend the tmp path if none was given /* istanbul ignore else */ - if (path.basename(template) == template) + if (path.basename(template) === template) template = path.join(opts.dir || tmpDir, template); return template.replace(TEMPLATE_PATTERN, _randomChars(6)); } @@ -479,7 +481,7 @@ function _prepareRemoveCallback(removeFunction, arg, cleanupCallbackSync) { called = true; // sync? - if (removeFunction.length == 1) { + if (removeFunction.length === 1) { try { removeFunction(arg); return next(null); @@ -550,7 +552,7 @@ function isENOENT(error) { * error.errno n/a */ function isExpectedError(error, code, errno) { - return error.code == code || error.code == errno; + return error.code === code || error.code === errno; } /** @@ -569,60 +571,87 @@ function setGracefulCleanup() { * @returns {Boolean} true whether listener is a legacy listener */ function _is_legacy_listener(listener) { - return (listener.name == '_exit' || listener.name == '_uncaughtExceptionThrown') + return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown') && listener.toString().indexOf('_garbageCollector();') > -1; } /** - * Safely install process exit listeners. - * + * Safely install SIGINT listener. + * + * NOTE: this will only work on OSX and Linux. + * * @private */ -function _safely_install_listener() { - var listeners = process.listeners(EVENT); +function _safely_install_sigint_listener() { - // collect any existing listeners - var existingListeners = []; - for (var i = 0, length = listeners.length; i < length; i++) { - var lstnr = listeners[i]; + const listeners = process.listeners(SIGINT); + const existingListeners = []; + for (let i = 0, length = listeners.length; i < length; i++) { + const lstnr = listeners[i]; /* istanbul ignore else */ - if (lstnr.name == '_tmp$safe_listener' || _is_legacy_listener(lstnr)) { - // we must forget about the uncaughtException listener - if (lstnr.name != '_uncaughtExceptionThrown') existingListeners.push(lstnr); - process.removeListener(EVENT, lstnr); + if (lstnr.name === '_tmp$sigint_listener') { + existingListeners.push(lstnr); + process.removeListener(SIGINT, lstnr); } } - - // windows does not support signals - // it'd never had won if it wasn't a major PITA - // with node v8.x and win 10 this is no longer an issue - if (process.platform == 'win32') { - var rl = require('readline').createInterface({ - input: process.stdin, - output: process.stdout - }); - - rl.on('SIGINT', function () { - process.emit('SIGINT'); - }); - } - - process.on('SIGINT', function () { - process.exit(0); + process.on(SIGINT, function _tmp$sigint_listener(doExit) { + for (let i = 0, length = existingListeners.length; i < length; i++) { + // let the existing listener do the garbage collection (e.g. jest sandbox) + try { + existingListeners[i](false); + } catch (err) { + // ignore + } + } + try { + // force the garbage collector even it is called again in the exit listener + _garbageCollector(); + } finally { + if (!!doExit) { + process.exit(0); + } + } }); +} + +/** + * Safely install process exit listener. + * + * @private + */ +function _safely_install_exit_listener() { + const listeners = process.listeners(EXIT); - process.addListener(EVENT, function _tmp$safe_listener(data) { + // collect any existing listeners + const existingListeners = []; + for (let i = 0, length = listeners.length; i < length; i++) { + const lstnr = listeners[i]; /* istanbul ignore else */ - if (existingListeners.length) { - for (var i = 0, length = existingListeners.length; i < length; i++) { + // TODO: remove support for legacy listeners once release 1.0.0 is out + if (lstnr.name === '_tmp$safe_listener' || _is_legacy_listener(lstnr)) { + // we must forget about the uncaughtException listener, hopefully it is ours + if (lstnr.name !== '_uncaughtExceptionThrown') { + existingListeners.push(lstnr); + } + process.removeListener(EXIT, lstnr); + } + } + // TODO: what was the data parameter good for? + process.addListener(EXIT, function _tmp$safe_listener(data) { + for (let i = 0, length = existingListeners.length; i < length; i++) { + // let the existing listener do the garbage collection (e.g. jest sandbox) + try { existingListeners[i](data); + } catch (err) { + // ignore } } _garbageCollector(); }); } -_safely_install_listener(); +_safely_install_exit_listener(); +_safely_install_sigint_listener(); /** * Configuration options. diff --git a/test/child-process.js b/test/child-process.js index ce189e2..dd9fd5b 100644 --- a/test/child-process.js +++ b/test/child-process.js @@ -12,24 +12,25 @@ module.exports.genericChildProcess = _spawnProcess('spawn-generic.js'); module.exports.childProcess = _spawnProcess('spawn-custom.js'); function _spawnProcess(spawnFile) { - return function (testCase, configFile, cb) { - var + return function (testCase, configFile, cb, signal) { + const configFilePath = path.join(__dirname, 'outband', configFile), commandArgs = [path.join(__dirname, spawnFile), configFilePath]; exists(configFilePath, function (configExists) { - if (configExists) return _doSpawn(commandArgs, cb); + if (configExists) return _doSpawn(commandArgs, cb, signal); cb(new Error('ENOENT: configFile ' + configFilePath + ' does not exist')); }); }; } -function _doSpawn(commandArgs, cb) { - var +function _doSpawn(commandArgs, cb, signal) { + const node_path = process.argv[0], stdoutBufs = [], - stderrBufs = [], + stderrBufs = []; + let child, done = false, stderrDone = false, @@ -48,14 +49,11 @@ function _doSpawn(commandArgs, cb) { child = spawn(node_path, commandArgs); child.stdin.end(); - // TODO we no longer support node 0.6 - // Cannot use 'close' event because not on node-0.6. function _close() { - var - stderr = _bufferConcat(stderrBufs).toString(), - stdout = _bufferConcat(stdoutBufs).toString(); - if (stderrDone && stdoutDone && !done) { + const + stderr = _bufferConcat(stderrBufs).toString(), + stdout = _bufferConcat(stdoutBufs).toString(); done = true; cb(null, stderr, stdout); } @@ -81,6 +79,13 @@ function _doSpawn(commandArgs, cb) { stderrDone = true; _close(); }); + + if (signal) { + setTimeout(function () { + // does not work on node 6 + child.kill(signal); + }, 2000); + } } function _bufferConcat(buffers) { @@ -89,10 +94,9 @@ function _bufferConcat(buffers) { } return new Buffer(buffers.reduce(function (acc, buf) { - for (var i = 0; i < buf.length; i++) { + for (let i = 0; i < buf.length; i++) { acc.push(buf[i]); } return acc; }, [])); } - diff --git a/test/issue121-test.js b/test/issue121-test.js index 69a24f3..4eaddcb 100644 --- a/test/issue121-test.js +++ b/test/issue121-test.js @@ -2,26 +2,53 @@ // vim: expandtab:ts=2:sw=2 const - assert = require('assert'), assertions = require('./assertions'), childProcess = require('./child-process').childProcess, - signals = ['SIGINT', 'SIGTERM']; + os = require('os'), + rimraf = require('rimraf'), + testCases = [ + { signal: 'SIGINT', expectExists: false }, + { signal: 'SIGTERM', expectExists: true } + ]; + +// skip tests on win32 +const isWindows = os.platform() === 'win32'; +const tfunc = isWindows ? xit : it; describe('tmp', function () { describe('issue121 - clean up on terminating signals', function () { - for (var i=0; i < signals.length; i++) { - issue121Tests(signals[i]); + for (let tc of testCases) { + tfunc('for signal ' + tc.signal, function (done) { + // increase timeout so that the child process may fail + this.timeout(20000); + issue121Tests(tc.signal, tc.expectExists)(done); + }); } }); }); -function issue121Tests(signal) { +function issue121Tests(signal, expectExists) { return function (done) { - childProcess('issue121.json', function (err, stderr, stdout) { + childProcess(this, 'issue121.json', function (err, stderr, stdout) { if (err) return done(err); else if (stderr) return done(new Error(stderr)); - else assertions.assertDoesNotExist(stdout); - done(); - }, true); + + try { + if (expectExists) { + assertions.assertExists(stdout); + } + else { + assertions.assertDoesNotExist(stdout); + } + done(); + } catch (err) { + done(err); + } finally { + // cleanup + if (expectExists) { + rimraf.sync(stdout); + } + } + }, signal); }; } diff --git a/test/issue129-sigint-test.js b/test/issue129-sigint-test.js new file mode 100644 index 0000000..0ba9b82 --- /dev/null +++ b/test/issue129-sigint-test.js @@ -0,0 +1,34 @@ +/* eslint-disable no-octal */ +// vim: expandtab:ts=2:sw=2 + +var + assert = require('assert'), + assertions = require('./assertions'), + childProcess = require('./child-process').childProcess; + +describe('tmp', function () { + describe('issue129: safely install sigint listeners', function () { + it('when simulating sandboxed behavior', function (done) { + childProcess(this, 'issue129-sigint.json', function (err, stderr, stdout) { + if (err) return done(err); + if (!stdout && !stderr) return done(new Error('stderr or stdout expected')); + if (stderr) { + try { + assertions.assertDoesNotStartWith(stderr, 'EEXISTS:MULTIPLE'); + assertions.assertDoesNotStartWith(stderr, 'ENOAVAIL:EXISTING'); + } catch (err) { + return done(err); + } + } + if (stdout) { + try { + assert.equal(stdout, 'EOK', 'existing listeners should have been removed and called'); + } catch (err) { + return done(err); + } + } + done(); + }); + }); + }); +}); diff --git a/test/outband/issue121.js b/test/outband/issue121.js index 0dce377..deba395 100644 --- a/test/outband/issue121.js +++ b/test/outband/issue121.js @@ -1,20 +1,19 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var - fs = require('fs'), - tmp = require('../../lib/tmp'), - // we reuse the fixtures from issue62 here - fixture = require('./issue62'); - -tmp.setGracefulCleanup(); +const + tmp = require('../../lib/tmp'); // https://github.com/raszi/node-tmp/issues/121 -module.exports = function (signal) { - fixture.apply(this, [tmp.dirSync({ unsafeCleanup: true }), tmp]); +module.exports = function () { + + tmp.setGracefulCleanup(); + + const result = tmp.dirSync({ unsafeCleanup: true }); - // make sure that the process keeps running - setTimeout(function () {}, 1000000); + this.out(result.name, function () { }); - this.kill(signal); + setTimeout(function () { + throw new Error('ran into timeout'); + }, 10000); }; diff --git a/test/outband/issue121.json b/test/outband/issue121.json index 86429e8..2210135 100644 --- a/test/outband/issue121.json +++ b/test/outband/issue121.json @@ -1,3 +1,4 @@ { + "graceful": true, "tc": "issue121" } diff --git a/test/outband/issue129-sigint.js b/test/outband/issue129-sigint.js new file mode 100644 index 0000000..0710536 --- /dev/null +++ b/test/outband/issue129-sigint.js @@ -0,0 +1,38 @@ +/* eslint-disable no-octal */ +// vim: expandtab:ts=2:sw=2 + +// addendum to https://github.com/raszi/node-tmp/issues/129 so that with jest sandboxing we do not install our sigint +// listener multiple times +module.exports = function () { + var callState = { + existingListener : false, + }; + + // simulate an existing SIGINT listener + var listener1 = (function (callState) { + return function _tmp$sigint_listener(doExit) { + callState.existingListener = !doExit; + }; + })(callState); + + process.addListener('SIGINT', listener1); + + // now let tmp install its listener safely + require('../../lib/tmp'); + + var sigintListeners = []; + + var listeners = process.listeners('SIGINT'); + for (var i = 0; i < listeners.length; i++) { + var listener = listeners[i]; + if (listener.name === '_tmp$sigint_listener') { + sigintListeners.push(listener); + } + } + + if (listeners.length > 1) this.fail('EEXISTS:MULTIPLE: existing SIGINT listener was not removed', this.exit); + listeners[0](false); // prevent listener from exiting the process + if (!callState.existingListener) this.fail('ENOAVAIL:EXISTING: existing listener was not called', this.exit); + this.out('EOK', this.exit); + process.exit(0); +}; diff --git a/test/outband/issue129-sigint.json b/test/outband/issue129-sigint.json new file mode 100644 index 0000000..967ff91 --- /dev/null +++ b/test/outband/issue129-sigint.json @@ -0,0 +1,3 @@ +{ + "tc": "issue129-sigint" +} diff --git a/test/outband/issue129.js b/test/outband/issue129.js index 0d65547..a36c56b 100644 --- a/test/outband/issue129.js +++ b/test/outband/issue129.js @@ -5,10 +5,10 @@ module.exports = function () { // dup from lib/tmp.js function _is_legacy_listener(listener) { - return (listener.name == '_exit' || listener.name == '_uncaughtExceptionThrown') - && listener.toString().indexOf('_garbageCollector();') != -1; + return (listener.name === '_exit' || listener.name === '_uncaughtExceptionThrown') + && listener.toString().indexOf('_garbageCollector();') !== -1; } - +å function _garbageCollector() {} var callState = { @@ -56,14 +56,14 @@ module.exports = function () { for (var i = 0; i < listeners.length; i++) { var listener = listeners[i]; // the order here is important - if (listener.name == '_tmp$safe_listener') { + if (listener.name === '_tmp$safe_listener') { newStyleListeners.push(listener); } else if (_is_legacy_listener(listener)) { - if (listener.name == '_uncaughtExceptionThrown') legacyUncaughtListener = listener; + if (listener.name === '_uncaughtExceptionThrown') legacyUncaughtListener = listener; else legacyExitListener = listener; } - } + }å if (legacyExitListener) this.fail('EEXISTS:LEGACY:EXIT existing legacy exit listener was not removed', this.exit); if (legacyUncaughtListener) this.fail('EEXISTS:LEGACY:UNCAUGHT existing legacy uncaught exception thrown listener was not removed', this.exit); diff --git a/test/spawn-custom.js b/test/spawn-custom.js index bb1cc27..f833f07 100644 --- a/test/spawn-custom.js +++ b/test/spawn-custom.js @@ -11,4 +11,3 @@ spawn.graceful = !!config.graceful; // import the test case function and execute it var fn = require(path.join(__dirname, 'outband', config.tc)); fn.apply(spawn); - diff --git a/test/spawn-generic.js b/test/spawn-generic.js index 6b4cb5b..d557db0 100644 --- a/test/spawn-generic.js +++ b/test/spawn-generic.js @@ -23,7 +23,7 @@ if (config.async) fnUnderTest(config.options, function (err, name, fdOrCallback, cb) { if (err) spawn.err(err); else { - var result = null; + var result = null; if (config.file) result = { name: name, fd: fdOrCallback, removeCallback: cb }; else result = { name: name, removeCallback: fdOrCallback }; fn.apply(spawn, [result, tmp]); diff --git a/test/spawn.js b/test/spawn.js index 5dfd87e..3f89510 100644 --- a/test/spawn.js +++ b/test/spawn.js @@ -28,9 +28,6 @@ module.exports = { }, exit: function (code) { process.exit(code || 0); - }, - kill: function (signal) { - process.kill(signal || 'SIGINT'); } };