Repository: mochajs/mocha

Problem statement:
throw undefined
``` js
it('expected', function () {
    throw 'foo';
});
it('unexpected', function () {
    throw undefined;
});
```

then

``` sh
  1) expected
  ✓ unexpected
```

in 1.21.5

throw undefined
``` js
it('expected', function () {
    throw 'foo';
});
it('unexpected', function () {
    throw undefined;
});
```

then

``` sh
  1) expected
  ✓ unexpected
```

in 1.21.5



Hints:
I'm working on this issue right now.

I agree that throwing `undefined` should make the test fail. What about throwing `null`?

Current behaviour is: a test passes if thrown either `undefined` or `null`.  UNLESS you are throwing async on an async test, due to `Runner.prototype.uncaught`. It even generates an error with the message `'Caught undefined error, did you throw without specifying what?'`. Only failing in that case must not be an intended behaviour.

Checking if the thrown exception is `undefined` can only be done in the `catch` block, since passing `undefined` to the callback means 'no error here'. (Well, you could pass to the callback a reference to a global object that means `undefined`, and then check the reference later, but that's quite ugly)

I made changes so that throwing `undefined` or `null` makes the test fail. However, it breaks the following project test case (line 228 at test/runnable.js):
`Runnable(title, fn), .run(fn), when async, when an exception is thrown, should not throw its own exception if passed a non-object`.

If I make it so that throwing `null` doesn't make a test fail whereas throwing `undefined` does make a test fail, the Runnable project test case does not break. But that's because it only checks throwing `null`, and not throwing `undefined`, which I believe is also a non-object.

I will send a PR with some changes and new tests. I'm marking as pending the Runnable test that breaks until we decide something.

IMO, if you are throwing, it should make the test fail.

I'm working on this issue right now.

I agree that throwing `undefined` should make the test fail. What about throwing `null`?

Current behaviour is: a test passes if thrown either `undefined` or `null`.  UNLESS you are throwing async on an async test, due to `Runner.prototype.uncaught`. It even generates an error with the message `'Caught undefined error, did you throw without specifying what?'`. Only failing in that case must not be an intended behaviour.

Checking if the thrown exception is `undefined` can only be done in the `catch` block, since passing `undefined` to the callback means 'no error here'. (Well, you could pass to the callback a reference to a global object that means `undefined`, and then check the reference later, but that's quite ugly)

I made changes so that throwing `undefined` or `null` makes the test fail. However, it breaks the following project test case (line 228 at test/runnable.js):
`Runnable(title, fn), .run(fn), when async, when an exception is thrown, should not throw its own exception if passed a non-object`.

If I make it so that throwing `null` doesn't make a test fail whereas throwing `undefined` does make a test fail, the Runnable project test case does not break. But that's because it only checks throwing `null`, and not throwing `undefined`, which I believe is also a non-object.

I will send a PR with some changes and new tests. I'm marking as pending the Runnable test that breaks until we decide something.

IMO, if you are throwing, it should make the test fail.


Patch:
diff --git a/lib/runnable.js b/lib/runnable.js
--- a/lib/runnable.js
+++ b/lib/runnable.js
@@ -4,7 +4,8 @@
 
 var EventEmitter = require('events').EventEmitter
   , debug = require('debug')('mocha:runnable')
-  , milliseconds = require('./ms');
+  , milliseconds = require('./ms')
+  , utils = require('./utils');
 
 /**
  * Save timer references to avoid Sinon interfering (see GH-237).
@@ -226,7 +227,7 @@ Runnable.prototype.run = function(fn){
         done();
       });
     } catch (err) {
-      done(err);
+      done(utils.getError(err));
     }
     return;
   }
@@ -243,7 +244,7 @@ Runnable.prototype.run = function(fn){
       callFn(this.fn);
     }
   } catch (err) {
-    done(err);
+    done(utils.getError(err));
   }
 
   function callFn(fn) {
diff --git a/lib/runner.js b/lib/runner.js
--- a/lib/runner.js
+++ b/lib/runner.js
@@ -540,7 +540,7 @@ Runner.prototype.uncaught = function(err){
     }.call(err) ? err : ( err.message || err ));
   } else {
     debug('uncaught undefined exception');
-    err = new Error('Caught undefined error, did you throw without specifying what?');
+    err = utils.undefinedError();
   }
   err.uncaught = true;
 
diff --git a/lib/utils.js b/lib/utils.js
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -398,3 +398,25 @@ exports.lookupFiles = function lookupFiles(path, extensions, recursive) {
 
   return files;
 };
+
+/**
+ * Generate an undefined error with a message warning the user.
+ *
+ * @return {Error}
+ */
+
+exports.undefinedError = function(){
+  return new Error('Caught undefined error, did you throw without specifying what?');
+};
+
+/**
+ * Generate an undefined error if `err` is not defined.
+ *
+ * @param {Error} err
+ * @return {Error}
+ */
+
+exports.getError = function(err){
+  return err || exports.undefinedError();
+};
+


Test patch:
diff --git a/test/acceptance/throw.js b/test/acceptance/throw.js
new file mode 100644
--- /dev/null
+++ b/test/acceptance/throw.js
@@ -0,0 +1,111 @@
+var mocha = require('../../')
+  , Suite = mocha.Suite
+  , Runner = mocha.Runner
+  , Test = mocha.Test;
+
+describe('a test that throws', function () {
+  var suite, runner;
+
+  beforeEach(function(){
+    suite = new Suite(null, 'root');
+    runner = new Runner(suite);
+  })
+
+  this.timeout(50);
+
+  describe('undefined', function (){
+    it('should not pass if throwing sync and test is sync', function(done) {
+      var test = new Test('im sync and throw undefined sync', function(){
+        throw undefined;
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    })
+
+    it('should not pass if throwing sync and test is async', function(done){
+      var test = new Test('im async and throw undefined sync', function(done2){
+        throw undefined;
+        process.nexTick(done2);
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    });
+
+    it('should not pass if throwing async and test is async', function(done){
+      var test = new Test('im async and throw undefined async', function(done2){
+        process.nexTick(function(){
+          throw undefined;
+          done2();
+        });
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    })
+  })
+
+  describe('null', function (){
+    it('should not pass if throwing sync and test is sync', function(done) {
+      var test = new Test('im sync and throw null sync', function(){
+        throw null;
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    })
+
+    it('should not pass if throwing sync and test is async', function(done){
+      var test = new Test('im async and throw null sync', function(done2){
+        throw null;
+        process.nexTick(done2);
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    });
+
+    it('should not pass if throwing async and test is async', function(done){
+      var test = new Test('im async and throw null async', function(done2){
+        process.nexTick(function(){
+          throw null;
+          done2();
+        });
+      });
+      suite.addTest(test);
+      runner = new Runner(suite);
+      runner.on('end', function(){
+        runner.failures.should.equal(1);
+        test.state.should.equal('failed');
+        done();
+      });
+      runner.run();
+    })
+  })
+})
\ No newline at end of file
diff --git a/test/runnable.js b/test/runnable.js
--- a/test/runnable.js
+++ b/test/runnable.js
@@ -225,7 +225,7 @@ describe('Runnable(title, fn)', function(){
           });
         })
 
-        it('should not throw its own exception if passed a non-object', function (done) {
+        it.skip('should not throw its own exception if passed a non-object', function (done) {
           var test = new Runnable('foo', function(done) {
             throw null;
             process.nextTick(done);