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

fix #342 set child process debug port to available #613

Closed
wants to merge 1 commit into from
Closed

fix #342 set child process debug port to available #613

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
61 changes: 50 additions & 11 deletions api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var resolveCwd = require('resolve-cwd');
var uniqueTempDir = require('unique-temp-dir');
var findCacheDir = require('find-cache-dir');
var slash = require('slash');
var getPort = require('get-port');
var AvaError = require('./lib/ava-error');
var fork = require('./lib/fork');
var formatter = require('./lib/enhance-assert').formatter();
Expand Down Expand Up @@ -61,19 +62,44 @@ Api.prototype._reset = function () {
this.base = '';
};

Api.prototype._runFile = function (file) {
Api.prototype._runFile = function (file, onForkStarting) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a weird combination of callbacks and promises. Can we clean it up to just use promises?

var options = objectAssign({}, this.options, {
precompiled: this.precompiler.generateHashForFile(file)
});

return fork(file, options)
var execArgv = process.execArgv.slice();
var debugArgIndex = -1;
for (var i = 0; i < execArgv.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Just use Array#some here.

if (execArgv[i].indexOf('--debug-brk=') === 0 || execArgv[i].indexOf('--debug=') === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't you just use the https://github.com/sindresorhus/has-flag dependency here? (we already depend on it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

has-flag returns boolean, but we need position of debug argument. In the future, es6 includes can be used here to avoid === 0.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you need the position? These are flag arguments, not positional arguments, and can be in any order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we don't append flag, but change existing. By default child_process.fork pass process.execArgv to child process as is. What we do here — we find debug argument and change its value to any free port.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps extract this into a memoizing function? Less logic in this function and no need to recompute for every file.

Copy link
Member

Choose a reason for hiding this comment

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

I think memoization would be a premature optimization for this as execArv usually contains zero or one elements.

Copy link
Member

Choose a reason for hiding this comment

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

This also doesn't handle the case where the user would do --debug-brk 213 instead of --debug-brk=213.

debugArgIndex = i;
break;
}
}

var execArgvPromise;
if (debugArgIndex === -1) {
execArgvPromise = Promise.resolve(execArgv);
} else {
execArgvPromise = getPort()
.then(function (port) {
execArgv[debugArgIndex] = '--debug-brk=' + port;
return execArgv;
});
}

return execArgvPromise
.then(function (execArgv) {
var result = fork(file, options, execArgv)
.on('teardown', this._handleTeardown)
.on('stats', this._handleStats)
.on('test', this._handleTest)
.on('unhandledRejections', this._handleRejections)
.on('uncaughtException', this._handleExceptions)
.on('stdout', this._handleOutput.bind(this, 'stdout'))
.on('stderr', this._handleOutput.bind(this, 'stderr'));
onForkStarting(result);
return result.promise;
}.bind(this));
};

Api.prototype._handleOutput = function (channel, data) {
Expand Down Expand Up @@ -191,13 +217,30 @@ Api.prototype.run = function (files) {
self.fileCount = files.length;
self.base = path.relative('.', commonPathPrefix(files)) + path.sep;

var tests = files.map(self._runFile);

return new Promise(function (resolve) {
// receive test count from all files and then run the tests
var statsCount = 0;

return new Promise(function (resolve) {
tests.forEach(function (test) {
var tests = new Array(files.length);
files.forEach(function (file, index) {
self._runFile(file, function (forkManager) {
tests[index] = forkManager;
forkManager.on('stats', tryRun);
})
.catch(function (error) {
Copy link
Member

Choose a reason for hiding this comment

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

This is asynchronous whereas onForkStarting is not. Probably best to build up tests similarly (and synchronously) to how it was done before and leave the rest of the code as it is. (Note that I just changed this you so you'll need to rebase).

tests[index] = {
run: function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would run just return this? Shouldn't it run something?

return this;
},
promise: Promise.reject(error),
then: function () {
return this.promise.then.apply(this.promise, arguments);
}
};
tryRun();
});
});

function tryRun() {
if (++statsCount === self.fileCount) {
self.emit('ready');
Expand All @@ -208,7 +251,7 @@ Api.prototype.run = function (files) {
};

resolve(Promise[method](files, function (file, index) {
return tests[index].run(options).catch(function (err) {
return tests[index].run(options).promise.catch(function (err) {
// The test failed catastrophically. Flag it up as an
// exception, then return an empty result. Other tests may
// continue to run.
Expand All @@ -225,10 +268,6 @@ Api.prototype.run = function (files) {
}));
}
}

test.on('stats', tryRun);
test.catch(tryRun);
});
});
})
.then(function (results) {
Expand Down
40 changes: 21 additions & 19 deletions lib/fork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ if (env.NODE_PATH) {
.join(path.delimiter);
}

module.exports = function (file, opts) {
module.exports = function (file, opts, execArgv) {
opts = objectAssign({
file: file,
tty: process.stdout.isTTY ? {
Expand All @@ -33,7 +33,8 @@ module.exports = function (file, opts) {
var ps = childProcess.fork(path.join(__dirname, 'test-worker.js'), [JSON.stringify(opts)], {
cwd: path.dirname(file),
silent: true,
env: env
env: env,
execArgv: execArgv
});

var relFile = path.relative('.', file);
Expand Down Expand Up @@ -115,37 +116,38 @@ module.exports = function (file, opts) {
}
});

promise.on = function () {
ps.on.apply(ps, arguments);

return promise;
};

promise.send = function (name, data) {
send(ps, name, data);

return promise;
};

// send 'run' event only when fork is listening for it
var isReady = false;

ps.on('stats', function () {
isReady = true;
});

promise.run = function (options) {
return {
promise: promise,
on: function () {
ps.on.apply(ps, arguments);
return this;
},
send: function (name, data) {
send(ps, name, data);
return this;
},
run: function (options) {
if (isReady) {
send(ps, 'run', options);
return promise;
return this;
}

ps.on('stats', function () {
send(ps, 'run', options);
});

return promise;
return this;
},
then: function () {
// only to simplify test code, in production use .promise
Copy link
Member

Choose a reason for hiding this comment

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

We should just bite the bullet and use .promise in the tests as well.

return promise.then.apply(promise, arguments);
}
};

return promise;
};
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
"figures": "^1.4.0",
"find-cache-dir": "^0.1.1",
"fn-name": "^2.0.0",
"get-port": "^2.1.0",
"globby": "^4.0.0",
"ignore-by-default": "^1.0.0",
"is-ci": "^1.0.7",
Expand Down