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

watcher: remember failures from previous tests #758

Merged
merged 2 commits into from
Apr 15, 2016
Merged

watcher: remember failures from previous tests #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3decf2b
api: use relative paths for RunStatus exceptions
novemberborn Apr 13, 2016
aab2207
watcher: remember failures from previous tests
novemberborn Apr 13, 2016
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
6 changes: 3 additions & 3 deletions api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Api.prototype._onTimeout = function (runStatus) {

runStatus.handleExceptions({
exception: new AvaError(message),
file: null
file: undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was inconsistent.

});

runStatus.emit('timeout');
Expand Down Expand Up @@ -151,7 +151,7 @@ Api.prototype._run = function (files, _options) {
// continue to run.
runStatus.handleExceptions({
exception: err,
file: file
file: path.relative('.', file)
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 more consistent with existing behavior in fork, etc. But maybe we should be emitting everything as absolute paths and have the reporters handle truncating to relative ones? Seems like it would be best to keep it explicit until we decide to show it to a human.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but that should be done as a separate task. fork emits more of these files so I figured I'd make it consistent with that first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed and understood.

});

return {
Expand Down Expand Up @@ -197,7 +197,7 @@ Api.prototype._run = function (files, _options) {

runStatus.handleExceptions({
exception: err,
file: file
file: path.relative('.', file)
});

resolve([]);
Expand Down
4 changes: 4 additions & 0 deletions lib/reporters/mini.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,10 @@ MiniReporter.prototype.finish = function (runStatus) {
status += '\n ' + colors.error(this.exceptionCount, plur('exception', this.exceptionCount));
}

if (runStatus.previousFailCount > 0) {
status += '\n ' + colors.error(runStatus.previousFailCount, 'previous', plur('failure', runStatus.previousFailCount), 'in test files that were not rerun');
}

var i = 0;

if (this.failCount > 0) {
Expand Down
3 changes: 2 additions & 1 deletion lib/reporters/verbose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ VerboseReporter.prototype.finish = function (runStatus) {
runStatus.skipCount > 0 ? ' ' + colors.skip(runStatus.skipCount, plur('test', runStatus.skipCount), 'skipped') : '',
runStatus.todoCount > 0 ? ' ' + colors.todo(runStatus.todoCount, plur('test', runStatus.todoCount), 'todo') : '',
runStatus.rejectionCount > 0 ? ' ' + colors.error(runStatus.rejectionCount, 'unhandled', plur('rejection', runStatus.rejectionCount)) : '',
runStatus.exceptionCount > 0 ? ' ' + colors.error(runStatus.exceptionCount, 'uncaught', plur('exception', runStatus.exceptionCount)) : ''
runStatus.exceptionCount > 0 ? ' ' + colors.error(runStatus.exceptionCount, 'uncaught', plur('exception', runStatus.exceptionCount)) : '',
runStatus.previousFailCount > 0 ? ' ' + colors.error(runStatus.previousFailCount, 'previous', plur('failure', runStatus.previousFailCount), 'in test files that were not rerun') : ''
].filter(Boolean);

if (lines.length > 0) {
Expand Down
1 change: 1 addition & 0 deletions lib/run-status.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function RunStatus(opts) {
this.failCount = 0;
this.fileCount = 0;
this.testCount = 0;
this.previousFailCount = 0;
this.errors = [];
this.stats = [];
this.tests = [];
Expand Down
74 changes: 67 additions & 7 deletions lib/watcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ function Watcher(logger, api, files, sources) {

this.isTest = makeTestMatcher(files, AvaFiles.defaultExcludePatterns());

var isFirstRun = true;
this.clearLogOnNextRun = true;
this.runVector = 0;
this.run = function (specificFiles) {
if (isFirstRun) {
isFirstRun = false;
} else {
if (this.runVector > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (this.runVector) or if (this.runVector !== 0) imo :).

Copy link
Contributor

Choose a reason for hiding this comment

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

Of those two I'd prefer !== 0, but I'm not sure why > 0 is unacceptable. Because it implies runVector might be less than zero? Or some other reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to avoid implicit conversions. The vector is increasing so > 0 signifies that more than !== 0 would, but not fussed either way.

Copy link
Member

Choose a reason for hiding this comment

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

this.runVector > 0 is the best option here. I would use !== 0 when it can also be negative.

var cleared = this.clearLogOnNextRun && logger.clear();
if (!cleared) {
logger.reset();
Expand All @@ -61,6 +59,8 @@ function Watcher(logger, api, files, sources) {
logger.start();
}

var currentVector = this.runVector += 1;

var runOnlyExclusive = false;

if (specificFiles) {
Expand All @@ -82,6 +82,7 @@ function Watcher(logger, api, files, sources) {
this.busy = api.run(specificFiles || files, {
runOnlyExclusive: runOnlyExclusive
}).then(function (runStatus) {
runStatus.previousFailCount = self.sumPreviousFailures(currentVector);
logger.finish(runStatus);

var badCounts = runStatus.failCount + runStatus.rejectionCount + runStatus.exceptionCount;
Expand All @@ -95,6 +96,9 @@ function Watcher(logger, api, files, sources) {
this.filesWithExclusiveTests = [];
this.trackExclusivity(api);

this.filesWithFailures = [];
this.trackFailures(api);

this.dirtyStates = {};
this.watchFiles(files, sources);
this.rerunAll();
Expand All @@ -121,10 +125,8 @@ Watcher.prototype.watchFiles = function (files, sources) {
Watcher.prototype.trackTestDependencies = function (api, sources) {
var self = this;
var isSource = makeSourceMatcher(sources);
var cwd = process.cwd();

var relative = function (absPath) {
return nodePath.relative(cwd, absPath);
return nodePath.relative('.', absPath);
};

api.on('test-run', function (runStatus) {
Expand Down Expand Up @@ -177,10 +179,68 @@ Watcher.prototype.updateExclusivity = function (file, hasExclusiveTests) {
}
};

Watcher.prototype.trackFailures = function (api) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this tracking behavior belongs in watcher.

Let's enhance RunStatus to track both aggregate and per file failures, and give it the ability to track previous runs.

Here is a possible API:

var testRun = api.run([...listofFiles]);

// someTime later
var previousRun = testRun;
testRun = api.run([...listOfFiles], previousRun);

Alternatively, maybe just create an "AggregateStatus" class:

var combinedStatus = new AggregateStatus({
  previousRun: RunStatus-instance,
  currentRun: RunStatus-instance,
  files: list-of-files-that-are-being-rerun, // or not being rerun, whichever makes more sense
  deleted: list-of-deleted-tests
});

Copy link
Member Author

Choose a reason for hiding this comment

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

The current approach is relatively clean. We could do something like that if we're going to move more of the watcher functionality into the API maybe, or when we have a clearer idea of what programmatic interface we need to present to editors plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current approach is relatively clean

It's clean enough, but watcher.js as a whole is getting pretty complex. I'm just looking for ways to separate what it does into logical chunks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Would be nice to do that later though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure - as stated below - none of my comments need to hold this up.

var self = this;

api.on('test-run', function (runStatus, files) {
files.forEach(function (file) {
self.pruneFailures(nodePath.relative('.', file));
});

var currentVector = self.runVector;
runStatus.on('error', function (err) {
self.countFailure(err.file, currentVector);
});
runStatus.on('test', function (result) {
if (result.error) {
self.countFailure(result.file, currentVector);
}
});
});
};

Watcher.prototype.pruneFailures = function (file) {
this.filesWithFailures = this.filesWithFailures.filter(function (state) {
return state.file !== file;
});
};

Watcher.prototype.countFailure = function (file, vector) {
var isUpdate = this.filesWithFailures.some(function (state) {
if (state.file !== file) {
return false;
}

state.count++;
return true;
});

if (!isUpdate) {
this.filesWithFailures.push({
file: file,
vector: vector,
count: 1
});
}
};

Watcher.prototype.sumPreviousFailures = function (beforeVector) {
var total = 0;

this.filesWithFailures.forEach(function (state) {
if (state.vector < beforeVector) {
total += state.count;
}
});

return total;
};

Watcher.prototype.cleanUnlinkedTests = function (unlinkedTests) {
unlinkedTests.forEach(function (testFile) {
this.updateTestDependencies(testFile, []);
this.updateExclusivity(testFile, false);
this.pruneFailures(testFile);
}, this);
};

Expand Down
21 changes: 19 additions & 2 deletions test/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ test('test file with no tests causes an AvaError to be emitted', function (t) {
return api.run([path.join(__dirname, 'fixture/no-tests.js')]);
});

test('test file that immediately exits with 0 exit code ', function (t) {
test('test file that immediately exits with 0 exit code', function (t) {
t.plan(2);

var api = new Api();
Expand All @@ -479,6 +479,22 @@ test('test file that immediately exits with 0 exit code ', function (t) {
return api.run([path.join(__dirname, 'fixture/immediate-0-exit.js')]);
});

test('test file that immediately exits with 3 exit code', function (t) {
t.plan(3);

var api = new Api();

api.on('test-run', function (runStatus) {
runStatus.on('error', function (err) {
t.is(err.name, 'AvaError');
t.is(err.file, path.join('test', 'fixture', 'immediate-3-exit.js'));
t.match(err.message, /exited with a non-zero exit code: 3/);
});
});

return api.run([path.join(__dirname, 'fixture/immediate-3-exit.js')]);
});

test('testing nonexistent files causes an AvaError to be emitted', function (t) {
t.plan(2);

Expand Down Expand Up @@ -905,13 +921,14 @@ test('using --match with no matching tests causes an AvaError to be emitted', fu
});

test('errors thrown when running files are emitted', function (t) {
t.plan(2);
t.plan(3);

var api = new Api();

api.on('test-run', function (runStatus) {
runStatus.on('error', function (err) {
t.is(err.name, 'SyntaxError');
t.is(err.file, path.join('test', 'fixture', 'syntax-error.js'));
t.match(err.message, /Unexpected token/);
});
});
Expand Down
44 changes: 39 additions & 5 deletions test/reporters/mini.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ test('results with passing tests', function (t) {
reporter.passCount = 1;
reporter.failCount = 0;

var actualOutput = reporter.finish();
var actualOutput = reporter.finish({});
var expectedOutput = [
'\n ' + chalk.green('1 passed') + time,
''
Expand All @@ -173,7 +173,7 @@ test('results with skipped tests', function (t) {
reporter.skipCount = 1;
reporter.failCount = 0;

var actualOutput = reporter.finish();
var actualOutput = reporter.finish({});
var expectedOutput = [
'\n ' + chalk.yellow('1 skipped') + time,
''
Expand All @@ -189,7 +189,7 @@ test('results with todo tests', function (t) {
reporter.todoCount = 1;
reporter.failCount = 0;

var actualOutput = reporter.finish();
var actualOutput = reporter.finish({});
var expectedOutput = [
'\n ' + chalk.blue('1 todo') + time,
''
Expand All @@ -204,7 +204,7 @@ test('results with passing skipped tests', function (t) {
reporter.passCount = 1;
reporter.skipCount = 1;

var output = reporter.finish().split('\n');
var output = reporter.finish({}).split('\n');

t.is(output[0], '');
t.is(output[1], ' ' + chalk.green('1 passed') + time);
Expand Down Expand Up @@ -320,13 +320,47 @@ test('results with errors', function (t) {
t.end();
});

test('results with 1 previous failure', function (t) {
var reporter = miniReporter();
reporter.todoCount = 1;

var runStatus = {
previousFailCount: 1
};

var output = reporter.finish(runStatus);
compareLineOutput(t, output, [
'',
' ' + colors.todo('1 todo') + time,
' ' + colors.error('1 previous failure in test files that were not rerun')
]);
t.end();
});

test('results with 2 previous failures', function (t) {
var reporter = miniReporter();
reporter.todoCount = 1;

var runStatus = {
previousFailCount: 2
};

var output = reporter.finish(runStatus);
compareLineOutput(t, output, [
'',
' ' + colors.todo('1 todo') + time,
' ' + colors.error('2 previous failures in test files that were not rerun')
]);
t.end();
});

test('empty results after reset', function (t) {
var reporter = miniReporter();

reporter.failCount = 1;
reporter.reset();

var output = reporter.finish();
var output = reporter.finish({});
t.is(output, '\n');
t.end();
});
Expand Down
36 changes: 36 additions & 0 deletions test/reporters/verbose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,42 @@ test('results with errors', function (t) {
t.end();
});

test('results with 1 previous failure', function (t) {
var reporter = createReporter();

var runStatus = createRunStatus();
runStatus.passCount = 1;
runStatus.exceptionCount = 1;
runStatus.previousFailCount = 1;

var output = reporter.finish(runStatus);
compareLineOutput(t, output, [
'',
' ' + colors.pass('1 test passed') + time,
' ' + colors.error('1 uncaught exception'),
' ' + colors.error('1 previous failure in test files that were not rerun')
]);
t.end();
});

test('results with 2 previous failures', function (t) {
var reporter = createReporter();

var runStatus = createRunStatus();
runStatus.passCount = 1;
runStatus.exceptionCount = 1;
runStatus.previousFailCount = 2;

var output = reporter.finish(runStatus);
compareLineOutput(t, output, [
'',
' ' + colors.pass('1 test passed') + time,
' ' + colors.error('1 uncaught exception'),
' ' + colors.error('2 previous failures in test files that were not rerun')
]);
t.end();
});

test('full-width line when sectioning', function (t) {
var reporter = createReporter();

Expand Down
Loading