Prevent accidentally doing async operations in sync tests

[sindresorhus]

Would be good to have a rule to prevent people from mistakenly doing async operations in a sync test test().

A common pitfall is doing:

test(t => {
    fetch().then(data => {
        t.is(data, 'foo');
    });
});

Here they forgot to return the promise, so AVA treats it as a synchronous test.

It should have been:

test(t => {
    return fetch().then(data => {
        t.is(data, 'foo');
    });
});

We could look for assertions inside .then() that isn't returned. Might need code path analysis for less obvious cases.

We could potentially detect incorrect usage of callbacks here too:

test(t => {
    fetch((err, data) => {
        t.is(data, 'bar');
    });
});

Here we could detect that there are assertions inside a callback with a err argument first.

This should have been:

test.cb(t => {
    fetch((err, data) => {
        t.is(data, 'bar');
        t.end();
    });
});

[sindresorhus]

Is there anything we can do in AVA core at runtime to detect any cases like this? Like, is it possible to detect if an async operation is initiated inside a function?

[jfmengels]

Sounds good to me. I think it might actually be fine to simply report when .then() is used, without looking at assertions. If you're thening something without returning it, that is pretty odd.

We'd have to try handling this case correctly though

test(t => {
    var foo = fetch().then(data => {
        t.is(data, 'foo');
    });
    return foo; // Should be ok
});

I have my doubts about the callback version. I think it'd work (if we search for more error names e, error, ...), and the false positives would be low, but this would probably fail (but it's unlikely so fine with a eslint-disable-line IMO):

test(t => {
    var errors = foo();
    var errorsString = errors.reduce(function(err, newError) {
      return err + '\n' + newError.message;
    });
    t.true(errorsString === 'Error1\nError2');
});

Is there anything we can do in AVA core at runtime to detect any cases like this? Like, is it possible to detect if an async operation is initiated inside a function?

Override Promise functions to set a flag saying that a promise has been used? Could help but would not help when using Promise libraries like Bluebird that were not imported in tests.

[sindresorhus]

I think it might actually be fine to simply report when .then() is used

👍

We'd have to try handling this case correctly though

Yes, that's the kind of case I thought of when mentioning code path analysis.

I have my doubts about the callback version. I think it'd work (if we search for more error names e, error, ...), and the false positives would be low, but this would probably fail (but it's unlikely so fine with a eslint-disable-line IMO):

I think that's fine. That applies to many of the built-in ESLint rules too, which also uses the first argument err as an indicator.

Override Promise functions to set a flag saying that a promise has been used? Could help but would not help when using Promise libraries like Bluebird that were not imported in tests.

Better than nothing. AsyncWrap in Node.js might let us track this correctly.