-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Limited Process Pools #791
Changes from all commits
94dd695
c646eb0
bc3a9f6
18ab69d
d1615a8
c6bc602
9f58c09
0371cd1
6bfbe19
e4a7c85
cb996bc
24fddb1
631c9bf
777d405
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,6 +116,19 @@ Api.prototype._run = function (files, _options) { | |
self.precompiler = new CachingPrecompiler(cacheDir, self.options.babelConfig); | ||
self.fileCount = files.length; | ||
|
||
var overwatch; | ||
if (this.options.concurrency > 0) { | ||
overwatch = this._runLimitedPool(files, runStatus, self.options.serial ? 1 : this.options.concurrency); | ||
} else { | ||
// _runNoPool exists to preserve legacy behavior, specifically around `.only` | ||
overwatch = this._runNoPool(files, runStatus); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a comment that |
||
} | ||
|
||
return overwatch; | ||
}; | ||
|
||
Api.prototype._runNoPool = function (files, runStatus) { | ||
var self = this; | ||
var tests = new Array(self.fileCount); | ||
|
||
// TODO: thid should be cleared at the end of the run | ||
|
@@ -154,15 +167,7 @@ Api.prototype._run = function (files, _options) { | |
file: path.relative('.', file) | ||
}); | ||
|
||
return { | ||
stats: { | ||
passCount: 0, | ||
skipCount: 0, | ||
todoCount: 0, | ||
failCount: 0 | ||
}, | ||
tests: [] | ||
}; | ||
return getBlankResults(); | ||
}); | ||
})); | ||
} | ||
|
@@ -224,3 +229,83 @@ Api.prototype._run = function (files, _options) { | |
return runStatus; | ||
}); | ||
}; | ||
|
||
function getBlankResults() { | ||
return { | ||
stats: { | ||
testCount: 0, | ||
passCount: 0, | ||
skipCount: 0, | ||
todoCount: 0, | ||
failCount: 0 | ||
}, | ||
tests: [] | ||
}; | ||
} | ||
|
||
Api.prototype._runLimitedPool = function (files, runStatus, concurrency) { | ||
var self = this; | ||
var tests = {}; | ||
|
||
runStatus.on('timeout', function () { | ||
Object.keys(tests).forEach(function (file) { | ||
var fork = tests[file]; | ||
fork.exit(); | ||
}); | ||
}); | ||
|
||
return Promise.map(files, function (file) { | ||
var handleException = function (err) { | ||
runStatus.handleExceptions({ | ||
exception: err, | ||
file: path.relative('.', file) | ||
}); | ||
}; | ||
|
||
try { | ||
var test = tests[file] = self._runFile(file, runStatus); | ||
|
||
return new Promise(function (resolve, reject) { | ||
var runner = function () { | ||
self.emit('ready'); | ||
var options = { | ||
// If we're looking for matches, run every single test process in exclusive-only mode | ||
runOnlyExclusive: self.options.match.length > 0 | ||
}; | ||
test.run(options) | ||
.then(resolve) | ||
.catch(reject); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meh, I prefer it the existing way. Two argument |
||
}; | ||
|
||
test.on('stats', runner); | ||
test.on('exit', function () { | ||
delete tests[file]; | ||
}); | ||
test.catch(runner); | ||
}).catch(handleException); | ||
} catch (err) { | ||
handleException(err); | ||
} | ||
}, {concurrency: concurrency}) | ||
.then(function (results) { | ||
// Filter out undefined results (usually result of caught exceptions) | ||
results = results.filter(Boolean); | ||
|
||
// cancel debounced _onTimeout() from firing | ||
if (self.options.timeout) { | ||
runStatus._restartTimer.cancel(); | ||
} | ||
|
||
if (self.options.match.length > 0 && !runStatus.hasExclusive) { | ||
// Ensure results are empty | ||
results = []; | ||
runStatus.handleExceptions({ | ||
exception: new AvaError('Couldn\'t find any matching tests'), | ||
file: undefined | ||
}); | ||
} | ||
|
||
runStatus.processResults(results); | ||
return runStatus; | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still use
_runNoPool
if concurrency is less than the fileCount.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I would agree but with the current behavior, no matter what if the
--concurrency
flag is used the code behind_runLimitedPool
is used. I vote we keep this as-is until we decide to drop_runNoPool
entirely so that we can deterministically say if you use the concurrency flag you will use the new process pool code.If enough people object to my opinion, however, I will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if I add a
concurrency
option topackage.json
, I want to be able to re-enable.only
behavior. By doingava [files ...]
Where the number of files is less than concurrency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm similarly worried about people using the
--concurrency
flag and seeing different behaviors based on their filename glob patterns. It's much simpler to convey in documentation "If the concurrency flag/option is present, automatic .only behavior is not supported" than a list of rules.Additionally, a user manually running ava with a deliberate subset of tests is likely only doing so to run a few specific tests because they internally do not use
.only
(like my team) which actually cycles back up to my argument of nixing.only
entirely since with the--watch
flag and good file name globbing, there is no real need for.only
.