Calling done(obj) for async test should fail it even if "obj" is not instance of Error

[eldargab]

Hi. I suddenly found that async test fails only when done() function called with instance of Error. That's seames to be unexpected because we typically considering as error any truthy object. In my case done() wasn't called explicitly and there was a bug wich passed to done() wrong object, but nevertheless all tests where passing.

[tj]

what was passed to done? (err, whatever) should always be an Error

[eldargab]

We can do something like

 // async
  if (this.async) {
    try {
      this.fn(function(err){
        if (err instanceof Error) return done(err);
        if (err) { // fail for any truthy object
          done(new Error(
            util.format('Exception occured: %s', util.inspect(err))
          ));
          return;
        }
        done();
      });
    } catch (err) {
      done(err);
    }
    return;
  }

So for done(err) err should not always be an instance of Error

[tj]

your example has an Error each time

[eldargab]

I send pull request to explain more clearly

[tj]

oh I see you mean in mocha, sorry I missed that. No I don't think we should error on that, people should not use strings for errors etc

[eldargab]

Surely it's a bad idea to use strings as an errors. But what if it's done unintentionaly or it's came from third party code?

For example:

    it('upload', function (done) {
        var content = 'Hello world!';
        a.chain
            (uploadTestFile, content)
            (function (blob) {
                blob.should.have.property('filename').equal('file.txt');
                blob.should.have.property('md5').equal(md5_string);
                blob.should.have.property('length').equal(content.length);
                return null;
            })
        ()(done);
    });

uploadTestFile() crashes by passing result to callback's first argument. Assertions are not run. Async library just calles done() directly. And ups test is green. Convention is to check if (err) { ..., not (err instanceof Error). Breaking conventions is unexpectable.

Anyway thanks for very clean and simple library)

[tj]

I get the concern, the main reason for allowing non-errors is that you can use done as a way to assert things:

emitter.on('something', done)
emitter.emit('something')

or:

request(app)
.get('/)
.expect('Some body', done);

vs a closure with done(), granted it's not much work but hey. Let me think on this for a bit

[eldargab]

May be if pattern

emitter.on('something', done)
emitter.emit('something')

realy common it is reasonable to invent something like

emitter.on('something', done.successfully)
emitter.emit('something')

where done.successfully() completes test entirely ignoring arguments?