Unit testing async functions in node/express

Question

I am looking for the cleanest pattern to test middleware that contains async code using Mocha, Node's assert library, and Sinon. Here's a paired-down example of the function I'm testing.

function some_middleware(req,res, next){
    var input = req.body;
    if (some_test(input)) {
        lib.async_function(input)
            .then((returned_object) => {
                res.locals.data = returned_object;
                res.render('stop-list');
            })
            .catch((err) => {
                res.render('message', {message: err})
            })
        }
    else{ next() }
}

None of these functions return the promise returned by lib.async_function(). Returning the promise would make testing in Mocha much easier. We could just change:

... 
return lib.async_function(input) 
...

we could test like this use:

it('Should responsd set locals.data to fetched object', function(){
    // Stubs -- most of this goes in a beforeEach block but here for the example
    var res = {render: sinon.stub(), locals: {}}
    var req = {body: "returned_value" }
    var next = sinon.stub()
    var async_stub = sinon.stub('lib', async_function)
    async_stub.resolves(fakedata.stoptimes)

    next.throws("Next Shouldn't be called")
    return some_middleware(req, res, next)
    .then((ret) => assert.equals(res.locals.data, "returned_value" ))

})

Question one: Is it a bad practice to insist functions return a value, which isn't required, simply to make testing easier? Are there any side effects of this?

If making all the async middleware functions return a promise is a bad idea, we can still design tests, but they start to get a little wordy. For example, we could use the stubbed out res.render() and test like this:

it('Should responsd set locals.data to fetched object', function(done){
    // Stubs
    var res = {locals: {}}
    var req = {body: "returned_value" }
    var next = sinon.stub()
    var async_stub = sinon.stub('lib', async_function)
    async_stub.resolves(fakedata.stoptimes)

    next.throws("Next Shouldn't be called")
    res.render = result => {
        try {
            assert.equals(res.locals.data, "returned_value" )
            done()
        } catch (e) {done(e)}
    }
    some_middleware(req, res, next)

})

We need to wrap the assert in a try/catch block otherwise done() never gets called when the test fails and the test times-out. This seems like a lot of code for such a simple test. Which leads to:

Question two: is there a better way or an accepted best-practice that gracefully handles test failures?

Answer 1

Is it a bad practice to insist functions return a value, which isn't required, simply to make testing easier? Are there any side effects of this?

I wouldn't say it's bad practise, having to provide anchors for test purposes isn't uncommon. Side affects would really boil down to how & where it's being used, in this case it looks very much like Express middleware and nowhere in the docs does it mention any rules about not returning values. Furthermore, if you dig into the source you'll find that middleware return values are ignored.

I think in your case returning a Promise from the middleware is the ideal solution.