Skip to content

Commit

Permalink
test: move common.fires() to inspector-helper
Browse files Browse the repository at this point in the history
common.fires() is specific to the inspector tests so move it to
inspector-helper.js. The one REPL test that used common.fires() does not
seem to need it. It provided a 1 second timeout for operations, but that
timeout appears both arbitrary and ineffective as the test passes if it
is reduced to even 1 millisecond.

PR-URL: nodejs#17401
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
Trott committed Dec 4, 2017
1 parent a322b8e commit e1054ab
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 55 deletions.
9 changes: 0 additions & 9 deletions test/common/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,6 @@ Tests whether `name` and `expected` are part of a raised warning.

Checks if `pathname` exists

### fires(promise, [error], [timeoutMs])
* promise [&lt;Promise]
* error [&lt;String] default = 'timeout'
* timeoutMs [&lt;Number] default = 100

Returns a new promise that will propagate `promise` resolution or rejection if
that happens within the `timeoutMs` timespan, or rejects with `error` as
a reason otherwise.

### getArrayBufferViews(buf)
* `buf` [&lt;Buffer>]
* return [&lt;ArrayBufferView&#91;&#93;>]
Expand Down
42 changes: 0 additions & 42 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -850,32 +850,6 @@ function restoreWritable(name) {
delete process[name].writeTimes;
}

function onResolvedOrRejected(promise, callback) {
return promise.then((result) => {
callback();
return result;
}, (error) => {
callback();
throw error;
});
}

function timeoutPromise(error, timeoutMs) {
let clearCallback = null;
let done = false;
const promise = onResolvedOrRejected(new Promise((resolve, reject) => {
const timeout = setTimeout(() => reject(error), timeoutMs);
clearCallback = () => {
if (done)
return;
clearTimeout(timeout);
resolve();
};
}), () => done = true);
promise.clear = clearCallback;
return promise;
}

exports.hijackStdout = hijackStdWritable.bind(null, 'stdout');
exports.hijackStderr = hijackStdWritable.bind(null, 'stderr');
exports.restoreStdout = restoreWritable.bind(null, 'stdout');
Expand All @@ -889,19 +863,3 @@ exports.firstInvalidFD = function firstInvalidFD() {
} catch (e) {}
return fd;
};

exports.fires = function fires(promise, error, timeoutMs) {
if (!timeoutMs && util.isNumber(error)) {
timeoutMs = error;
error = null;
}
if (!error)
error = 'timeout';
if (!timeoutMs)
timeoutMs = 100;
const timeout = timeoutPromise(error, timeoutMs);
return Promise.race([
onResolvedOrRejected(promise, () => timeout.clear()),
timeout
]);
};
41 changes: 39 additions & 2 deletions test/common/inspector-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class InspectorSession {
waitForNotification(methodOrPredicate, description) {
const desc = description || methodOrPredicate;
const message = `Timed out waiting for matching notification (${desc}))`;
return common.fires(
return fires(
this._asyncWaitForNotification(methodOrPredicate), message, TIMEOUT);
}

Expand Down Expand Up @@ -323,7 +323,7 @@ class NodeInstance {
const instance = new NodeInstance(
[], `${scriptContents}\nprocess._rawDebug('started');`, undefined);
const msg = 'Timed out waiting for process to start';
while (await common.fires(instance.nextStderrString(), msg, TIMEOUT) !==
while (await fires(instance.nextStderrString(), msg, TIMEOUT) !==
'started') {}
process._debugProcess(instance._process.pid);
return instance;
Expand Down Expand Up @@ -412,6 +412,43 @@ function readMainScriptSource() {
return fs.readFileSync(_MAINSCRIPT, 'utf8');
}

function onResolvedOrRejected(promise, callback) {
return promise.then((result) => {
callback();
return result;
}, (error) => {
callback();
throw error;
});
}

function timeoutPromise(error, timeoutMs) {
let clearCallback = null;
let done = false;
const promise = onResolvedOrRejected(new Promise((resolve, reject) => {
const timeout = setTimeout(() => reject(error), timeoutMs);
clearCallback = () => {
if (done)
return;
clearTimeout(timeout);
resolve();
};
}), () => done = true);
promise.clear = clearCallback;
return promise;
}

// Returns a new promise that will propagate `promise` resolution or rejection
// if that happens within the `timeoutMs` timespan, or rejects with `error` as
// a reason otherwise.
function fires(promise, error, timeoutMs) {
const timeout = timeoutPromise(error, timeoutMs);
return Promise.race([
onResolvedOrRejected(promise, () => timeout.clear()),
timeout
]);
}

module.exports = {
mainScriptPath: _MAINSCRIPT,
readMainScriptSource,
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-repl-top-level-await.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class REPLStream extends common.ArrayStream {
throw new Error('Currently waiting for response to another command');
}
this.lines = [''];
return common.fires(new Promise((resolve, reject) => {
return new Promise((resolve, reject) => {
const onError = (err) => {
this.removeListener('line', onLine);
reject(err);
Expand All @@ -50,7 +50,7 @@ class REPLStream extends common.ArrayStream {
};
this.once('error', onError);
this.on('line', onLine);
}), new Error(), 1000);
});
}
}

Expand Down

0 comments on commit e1054ab

Please sign in to comment.