Mocha: all async test should catch errors

[alexey2baranov]

Hi!

it("some desc", async function(done){
    try{
        //some test
        done();
    }
    catch(err){
        done(err);
    }
));

should become just short

it("some desc", async function(done){
    //some test
    done();
));

it is very simple to diagnose if it function return Promise catch and pass their error into test done() function.

Thanks

[Munter]

What is this about?

[boneskull]

@alexey2baranov done shouldn't be used with async; omit it and you'll have better luck (though, like @Munter, I don't quite understand this issue either)

[alexey2baranov]

OK. I think it will be better if I show you one one full test hire

        it("should publicate event", async function (done) {
            try {
                await WAMP.session.subscribe("ru.kopa.model.Zemla.id2.change", function () { //this is async callback
                    done();
                });
                zemla.note = zemlaNote;
                await zemla.save();
            }
            catch (err) {
                done(err);
            }
        });

As you can see in some cases I can not just omit done(). If I do this the test will success when model saved but not when event happened.

I would like to mocha have some helper wrapper around each async test function and we could write this test simple like this:

        it("should publicate event", async function (done) {
                await WAMP.session.subscribe("ru.kopa.model.Zemla.id2.change", function () {
                    done();
                });
                zemla.note = zemlaNote;
                await zemla.save();
            }
        });

the only think i ask is to add .catch(err=>done(err)) after each async test function. I will be very very happy.

[Munter]

As previously mentioned, don't use done in async functions. What you are doing in your dream code example is calling done before you reached the last two lines, which I assume is not the intended behavior if you want to test those lines. Try it without the done callback

[alexey2baranov]

@Munter
@boneskull

As i pointed before i can't omit done callback. In this case test will success if zemla.save() success. But the target for this test is to make shure event is fired. This is why test name is "should publicate event"

You should see my code again please. this code executes on client side. It save model and should receive "change" event. I added some comments to it you can read it easyer

it("should publicate event", async function (done) {
    await WAMP.session.subscribe("ru.kopa.model.Zemla.id2.change", function () { //awaiting WAMP registration (done() not called hire!!)
        done();  //done called inside callback after zemla.save() awaited
    });
    zemla.note = zemlaNote;
    await zemla.save();  //saving model -> server publicates event -> subscription callback executed and done() called
});

this test executes perfect by this command (with babel)

/usr/bin/node --use-strict /usr/lib/node_modules/mocha/bin/_mocha --timeout 2000 --require babel-register --require source-map-support --ui bdd --reporter /home/alexey2baranov/WebStorm-162.1447.27/plugins/NodeJS/js/mocha-intellij/lib/mochaIntellijReporter.js /home/alexey2baranov/htdocs/kp-client/test/model/RemoteModel.test.js

It pass success with try-catch and without it both. The only disadvantage of short version is when WAMP.session.subscribe() or zemla.save() throws error test does not finished with error but by timeout. This is very uncomfortablу

[alexey2baranov]

I'm asking about very simple feature to add .catch(err) handler for every promised test and call done(err)

testPromisedFunction.catch(err=>done(err))

@boneskull
this is very important for IoT application where models asyncronys publicate there events for other parts of system. Each model covered by tests with very-very same design
try{ publicating - handling - done() } catch(err=>done(err))

I would like so simplify it to
publicating - handling - done()

[Munter]

@alexey2baranov

I'm asking about very simple feature to add .catch(err) handler for every promised test and call done(err)

I would ask you to try and implement that and make all tests pass before you make judgement on simplicity here.

Also, if you really need the done callback, then drop the async and await. That should give you the behavior you want

[alexey2baranov]

@Munter
async/await is the future of JavaScript. It is available in Microsoft Edge (stable) and Google Chrome (by the Experimental Javascript flag), very close in Firefox and one of the most awaited fearute of nodejs. It is incorrect to say "drop async/await and write your test old way".

If this feature is hard to implement i can understand it and wait as far as needed. But i would like to hear this feature is accepted and will be implemented.

[Munter]

It looks to me like what you are testing is event callbacks. This doesn't fit well within the async/await paradigm. It could be promise wrapped if you like to write extra code. But really, what you are testing here fits best within the callback paradigm, so I find your insistence on using async where it doesn't apply a bit weird.

The argument that async/await is the future of javascript is invalid. It is a future of javascript. So are Promises, and so are callbacks. Everything is not going to be rewritten into async, so callbacks will stick around for any foreseeable future as well.

Your sense of entitlement is misplaced, and frankly making me a bit annoyed, so I'm going to stop following this thread

[alexey2baranov]

@Munter
your persistence surprises me and it becomes very interesting. Async/await is not a paradigm it is just syntax sugar over regular Promises. When you have promised library like autobahnjs you have to use it, not callback.

Anyway I will totally agree with you and leave this topic if you can rewrite the test in best way with callback or other way you want. it is very simple 5 line test so you can do it very fast.

sorry for my English. as you can see it is not my mother language. hope this will not stop our dialog

[ScottFreeCode]

the only think i ask is to add .catch(err=>done(err)) after each async test function.

Mocha already catches promise errors as test failures more or less like that in effect -- if the done callback is not used (see #2407 for discussion). For example:

it("should handle async promises", async function() {
  const result = await first()
  return await last(result)
})

function first() {
  return Promise.resolve(42)
}

function last(result) {
  throw new Error("This function did not work!")
  return result.toString()
}

1) should handle async promises

0 passing (170ms)
1 failing

1) should handle async promises:
Error: This function did not work!
at last (...)

I'm not sure how event callbacks fit into this sort of behavior, however. It might help if I could look at some documentation for the API being used in the test.

Tangentially, if you use done in this sort of test without the error:

it("should handle async promises", async function(done) {
  const result = await first()
  await last(result)
  done()
})

function first() {
  return Promise.resolve(42)
}

function last(result) {
  //throw new Error("This function did not work!")
  return result.toString()
}

1) should handle async promises

0 passing (155ms)
1 failing

1) should handle async promises:
Error: Resolution method is overspecified. Specify a callback _or_ return a Promise; not both.

I haven't found a way to use an async test and done and have it pass, because of that, so I am not sure how the test code you've shown us could work when an error doesn't occur; even if I setTimeout(done, 1000) I still end up getting the "Resolution method is overspecified" test result... At least, in 3.x. I'd have to do some digging to find out what happens in all these cases on 2.x, but I don't know that it's worth it -- the situation that 3.x disallows is generally buggy in 2.x (either on the part of the test, or on the part of Mocha, or both).

Getting a timeout when done is used and a promise error occurs is basically a duplicate of #2407; depending on whether there's a simple way to deal with this API, testing an API that uses both promises and event-handling callbacks might be added to the considerations on that issue. That being said, as brought up in that issue, the problem is not specifically the use of promises inside a test but rather returning the promise out of the test function (which is implicit in async test functions), so you should be able to use the promises directly in a regular test function and have it work, even if that is a little less sugary:

it("should publish event", function (done) {
    WAMP.session.subscribe("ru.kopa.model.Zemla.id2.change", function () {
        done();
    }).then(function() {
        zemla.note = zemlaNote;
        zemla.save();
    });
});

Then again, as far as I can tell the de-asynced version of your example test doesn't even need promises (assuming that there aren't side effects that make the timing wrong for this):

it("should publish event", function (done) {
    WAMP.session.subscribe("ru.kopa.model.Zemla.id2.change", function () {
        done();
    })
    zemla.note = zemlaNote;
    zemla.save();
});

(P.S. In case it saves anyone else any time, I got the async functions to run in Node with --require babel-register and the following .babelrc file:)

{
  "presets": [
    "es2015",
    "stage-3"
  ],
  "plugins": [
    "transform-runtime"
  ]
}

[burka]

Just in case someone else stumbles upon this, it works out of the box.

describe('async/await', function () {
    it('should fail on throw', async function () {
        throw new Error('Why.')
    });
});

That test fails as expected.