Skip to content

Commit

Permalink
Report parallel spec loading errors as suite errors rather than crashing
Browse files Browse the repository at this point in the history
This avoids logspam and worker process leaks due to bugs in early
termination.
  • Loading branch information
sgravrock committed Jul 4, 2024
1 parent 9a8edf2 commit b71b3be
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 11 deletions.
10 changes: 8 additions & 2 deletions lib/parallel_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,12 @@ class ParallelRunner extends RunnerBase {
runNextSpecFile();
break;

case 'specFileLoadError':
this.addTopLevelError_('load',
`Error loading ${msg.filePath}`, msg.error);
runNextSpecFile();
break;

case 'uncaughtException':
this.addTopLevelError_('lateError',
'Uncaught exception in worker process', msg.error);
Expand Down Expand Up @@ -431,12 +437,12 @@ class ParallelRunner extends RunnerBase {
});
}

addTopLevelError_(type, msgPrefix, serializedError) {
addTopLevelError_(globalErrorType, msgPrefix, serializedError) {
// Match how jasmine-core reports these in non-parallel situations
this.executionState_.failedExpectations.push({
actual: '',
expected: '',
globalErrorType: 'lateError',
globalErrorType,
matcherName: '',
message: `${msgPrefix}: ${serializedError.message}`,
passed: false,
Expand Down
3 changes: 2 additions & 1 deletion lib/parallel_worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ class ParallelWorker {
await this.loader_.load(specFilePath);
} catch (error) {
this.clusterWorker_.send({
type: 'fatalError',
type: 'specFileLoadError',
filePath: specFilePath,
error: serializeError(error)
});
return;
Expand Down
15 changes: 8 additions & 7 deletions spec/integration_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,8 @@ describe('Integration', function () {
['--parallel=2']
);

expect(exitCode).toEqual(1);
expect(output).toContain('Fatal error in worker: nope\n');
expect(exitCode).toEqual(3);
expect(output).toMatch(/Error loading .*\/spec\/fixtures\/parallel_spec_load_exception\/spec.js: nope/);
expect(output).toMatch(/at Object\.<anonymous> .*spec[\\\/]fixtures[\\\/]parallel_spec_load_exception[\\\/]spec\.js/);
});

Expand Down Expand Up @@ -373,8 +373,8 @@ describe('Integration', function () {
['--parallel=2']
);

expect(exitCode).toEqual(1);
expect(output).toContain('Fatal error in worker: In parallel mode, ' +
expect(exitCode).toEqual(3);
expect(output).toContain('In parallel mode, ' +
'beforeEach must be in a describe block or in a helper file');
});

Expand All @@ -385,9 +385,10 @@ describe('Integration', function () {
['--parallel=2']
);

expect(exitCode).toEqual(1);
expect(output).toContain('Fatal error in worker: In parallel mode, ' +
'afterEach must be in a describe block or in a helper file');
expect(exitCode).toEqual(3);
expect(output).toMatch(
/Error loading .*\/spec\/fixtures\/parallel_invalid_afterEach\/spec.js: In parallel mode, afterEach must be in a describe block or in a helper file/
);
});

it('allows beforeEach and afterEach in helpers and in describe in parallel', async function() {
Expand Down
118 changes: 118 additions & 0 deletions spec/parallel_runner_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,124 @@ describe('ParallelRunner', function() {
);
});

describe('When a spec file fails to load', function() {
function captor(valueCb) {
return {
asymmetricMatch(v) {
valueCb(v);
return true;
},
jasmineToString() {
return '<captor>';
}
};
}

it('moves on to the next file', async function() {
this.testJasmine.numWorkers = 2;
this.testJasmine.loadConfig({
spec_dir: 'some/spec/dir'
});
const specFiles = ['spec1.js', 'spec2.js', 'spec3.js'];

for (const f of specFiles) {
this.testJasmine.addSpecFile(f);
}

this.testJasmine.execute();
await this.emitAllBooted();

await new Promise(resolve => setTimeout(resolve));
let worker0SpecFile, worker1SpecFile;
expect(this.cluster.workers[0].send).toHaveBeenCalledWith(
jasmine.objectContaining({
type: 'runSpecFile',
filePath: captor(v => worker0SpecFile = v)
})
);
expect(this.cluster.workers[1].send).toHaveBeenCalledWith(
jasmine.objectContaining({
type: 'runSpecFile',
filePath: captor(v => worker1SpecFile = v)
})
);
this.cluster.workers[0].send.calls.reset();
const remainingSpecFile = specFiles
.filter(f => f !== worker0SpecFile && f !== worker1SpecFile)[0];
this.cluster.workers[0].emit('message',
{
type: 'specFileLoadError',
filePath: worker0SpecFile,
error: {
message: 'not caught',
stack: 'it happened here'
},
}
);

expect(this.cluster.workers[0].send).toHaveBeenCalledWith(
{type: 'runSpecFile', filePath: remainingSpecFile}
);
});

it('reports the error', async function () {
this.testJasmine.numWorkers = 2;
this.testJasmine.loadConfig({
spec_dir: 'some/spec/dir'
});
this.testJasmine.addSpecFile('spec1.js');
this.testJasmine.addSpecFile('spec2.js');
const executePromise = this.testJasmine.execute();
await this.emitAllBooted();

await new Promise(resolve => setTimeout(resolve));
let worker1SpecFile;
expect(this.cluster.workers[1].send).toHaveBeenCalledWith(
jasmine.objectContaining({
type: 'runSpecFile',
filePath: captor(v => worker1SpecFile = v)
})
);
this.emitFileDone(this.cluster.workers[0], {
failedExpectations: ['failed expectation 1'],
deprecationWarnings: [],
});
this.cluster.workers[1].emit('message',
{
type: 'specFileLoadError',
filePath: worker1SpecFile,
error: {
message: 'not caught',
stack: 'it happened here'
},
}
);

await this.disconnect();
await executePromise;

expect(this.consoleReporter.jasmineDone).toHaveBeenCalledWith(
jasmine.objectContaining({
overallStatus: 'failed',
failedExpectations: [
'failed expectation 1',
// We don't just pass this through from jasmine-core,
// so verify the actual output format.
{
actual: '',
expected: '',
globalErrorType: 'load',
matcherName: '',
message: `Error loading ${worker1SpecFile}: not caught`,
passed: false,
stack: 'it happened here',
}
],
})
);
});
});

it('handles errors from reporters', async function() {
const reportDispatcher = new StubParallelReportDispatcher();
spyOn(reportDispatcher, 'installGlobalErrors');
Expand Down
3 changes: 2 additions & 1 deletion spec/parallel_worker_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,8 @@ describe('ParallelWorker', function() {

expect(this.clusterWorker.send).toHaveBeenCalledWith(
{
type: 'fatalError',
type: 'specFileLoadError',
filePath: 'aSpec.js',
error: {
message: error.message,
stack: error.stack
Expand Down

0 comments on commit b71b3be

Please sign in to comment.