Skip to content

Loading…

Stop test after first assertion failure, and handle exceptions thrown from callbacks #177

Closed
wants to merge 1 commit into from
@gilad61

This pull request contains two suggested fixes:

  1. Currently when an exception is thrown from an asynchronous callback the process crashes and subsequent tests don't run. Instead we should catch the exception and fail the test gracefully with test.done(exception).

  2. Currently when any assertion fails the test continues and all other assertions are called. This is not a common behavior. The test should fail and stop after the first assertion failed, and move on to the next test.

@Sannis

It would be good improvement.

@epall

+1

@dbaba

+1

@spiffytech

Can we please get this merged in? This functionality is crucial for me in a testing library.

@fxa

to 1. of course +1
to 2. I am torn. When writing new tests, I want to stop on the first error. When running my unit tests during a build, I don't expect an error. When my code (or my tests) are erroneous, the first thing I want to have is an overview.
For example, when ALL tests fail, propably I made an infrastructure thingy false. If I only see, that the first test fails, I would be mislead and investigate in this test.
I think, the best way would be an OPTION, whether to fail fast or not.
failpoint: firstFailedAssert | firstFailedTest | never

@spiffytech

@fxa: (2) is not a request to abort the entire test suite when an error is encountered. It is a request to abort a particular test function when an assertion fails in that function. All other test functions in the test suite would still be run like normal.

@fxa

@ spiffytech
Sorry for my misunderstanding!
Forget my previous comment and it is a
+1 to all from me

@mreinstein
Collaborator

I think this is a good change. One thing to keep in mind is it's breaking the public API, introducing behavior that people may be depending on. Let's tentatively plan on doing this in the 1.0.0 release.

@ctalkington

:+1: this would be awesome

@fidanov

+1 from me too, if this is not accepted soon I am changing to something else

@isacssouza

+1, it's been 10 months since this pull request, is it ever going to be merged?

@mreinstein
Collaborator

@isacssouza I'm not intentionally ignoring this merge. The problem is I'm not super familiar with nodeunit internals, so I'm hesitant to just merge in code without having a deeper understanding. I get the 2 points of the merge; to basically fix the test handling on uncaught exceptions and failing tests.

Can you please add some unit tests for this? I'll feel a bit better about the merge if I see some basic tests in place.

@isacssouza

Thanks for the follow up, @mreinstein Unfortunately I'm also not familiar with this code. @gilad61, can you help us here?

I'll try to look into this when I have time.

@mlogan

+1

(I normally hate to "me too" things, but since this has been open for a while...)

@mreinstein
Collaborator

I'm all for doing this, still waiting for unit tests!

@zippo445

+1 I would really need this feature. I might not be understanding the problem correctly but the following solution would work for me: https://gist.github.com/zippo445

just added two try/catch around fn.call(context, test) in the wrapTest function in core.js file....
should I fork and make a pull request out of this?

@mreinstein
Collaborator

Again, I'm all for merging this. I'm still waiting for some unit tests for this feature before including it.

@zippo445

I wrote this little test: https://gist.github.com/zippo445/6733044 it could be added to test/test-testcase.js hope it's enough to go with the merge. let me know.

@isacssouza
@zippo445

so..... is the simple test above ok? if not, can you please explain would be missing to make it robust enough?

@bsrik77

+1

@glukki

+1

@YuukanOO

Totally need this +1

@Granjow

+42

Would be quite helpful to get err in this example instead of just the «could not be created» message when err is not null!

    user.save( function ( err, user ) {
            test.ok( err === null, err );
            test.ok( user != null, 'User could not be created' );
            test.done();
    } );
@glukki

@Granjow sorry, why don't you use test.ifError( err )?

@Granjow

@glukki Valid point. Changed it, thanks for the hint. If err is an error, I still get the message of the second test for user != null though.

@caolan
Owner

This should probably be done with domains today. Not sure how best to handle this in the browser case.

@caolan caolan closed this
@statico

@caolan Did you mean to close this issue if it's not fixed?

@caolan
Owner

@statico close the pull request, yes... the issue itself still needs attention :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 1, 2012
  1. @gilad61
Showing with 7 additions and 6 deletions.
  1. +6 −0 lib/core.js
  2. +1 −6 lib/types.js
View
6 lib/core.js
@@ -65,6 +65,12 @@ exports.runTest = function (name, fn, opt, callback) {
var start = new Date().getTime();
var test = types.test(name, start, options, callback);
+ // Fail the test gracefully if an exception was thrown from a callback
+ process.removeAllListeners('uncaughtException');
+ process.on('uncaughtException', function (exception) {
+ test.done(exception);
+ });
+
try {
fn(test);
}
View
7 lib/types.js
@@ -79,12 +79,7 @@ var assertWrapper = function (callback) {
return function () {
var message = arguments[arity - 1];
var a = exports.assertion({method: new_method, message: message});
- try {
- assert[assert_method].apply(null, arguments);
- }
- catch (e) {
- a.error = e;
- }
+ assert[assert_method].apply(null, arguments);
callback(a);
};
};
Something went wrong with that request. Please try again.