tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

regression? 4.x.x vs 5.x.x -- planned tests don't wait inside async functions #529

Closed talmobi closed 4 years ago

talmobi commented 4 years ago

works in both 4.13.3 and 5.0.0/5.0.1

test( function ( t ) {
  t.plan( 1 )
  setTimeout( function () {
    t.pass()
  }, 1000 )
} )

works in 4.13.3, but not in 5.0.0/5.0.1

test( async function ( t ) {
  t.plan( 1 )
  setTimeout( function () {
    t.pass()
  }, 1000 )
} )

in 5.x.x code falls through immediately and ends the test.

tested on node v12.18.3

ljharb commented 4 years ago

This is expected. If your test callback returns a promise, you don't need an explicit t.pass - but also, you can't mix setTimeout with promises like this.

If you do:

test(async function (t) {
  t.plan(1);
  return new Promise((resolve) => {
    setTimeout(function () {
      resolve();
    }, 1000);
  });
});

i would expect it to pass in both v4 and v5.

talmobi commented 4 years ago

That won't work because the planned tests haven't been fulfilled. The setTimeout is superficial. The point is that during t.plan, in v4, the test would wait for the plans to be fulfilled (or timeoutAfter).

The only way to get similar behaviour is to return a never resolving promise in v5, which seems weird to me. Or to bypass it completely and wrap the whole non-async test inside an async IIFE.

works even without resolving:

test( async function ( t ) {
  t.plan( 1 )
  return new Promise( function ( resolve ) {
    setTimeout( function () {
      t.pass()
    }, 1000 )
  } )
} )

In summary: in v4 the implicit promise resolving during async tests would be effectively ignored during t.plan. I guess this was a bug and not a feature?

ljharb commented 4 years ago

I think any usage of async function in v4 was a bug, since tape did not expect any test callbacks to return anything.

In general, any use if setTimeout/callbacks, mixed with Promises, is a bug unless the callback usage is properly wrapped in new Promise.

What I do see in both v4 and v5, is that:

test('', async function (t) { t.plan(1); t.end(); });

will fail, and:

test('', async function (t) { t.plan(1); t.pass(); });

passes (which is the expected behavior, since "pass" counts as an assertion but "end" does not).

When i wrap the t.end/t.pass calls in a non-promise-wrapped setTimeout, they both fail in v5 and both pass in v4, and when I wrap them in a promise-wrapped setTimeout with a resolve call, i get the expected behavior in both v4 and v5 (that the "end" version fails, and the "pass" version passes"

In other words, it seems to me that v5 is behaving properly when setTimeout and promises are mixed correctly, and that indeed, this was not behaving properly in v4.

talmobi commented 4 years ago

Yeah I guess that makes sense.

Will have to use the non-async callback and wrap the test code inside an async IIFE instead -- which seems like the more technically correct way, too -- but not as convenient, I guess.

ljharb commented 4 years ago

That's an approach, but i'd suggest wrapping all non-promise async code in a Promise to keep everything clean :-)

talmobi commented 4 years ago

Yeah but I'm mixing event listeners and async/await promise code together ;_;

sample

server = await createServer()
server.on('connect', function (socket) {
socket.write(...)
socket.on('data', d => t.equal(d, 'hello')
})

client = await connect()
client.write(...)
ljharb commented 4 years ago

a Promise happens 0 or 1 times, an event happens 0, 1, or n times, so mixing promises and events rarely works well. In other words, you might want to connect.then() rather than using async function?

talmobi commented 4 years ago

We can agree on how promises and events work.

We can't agree on not mixing them together. I guess it depends what you mean by mixing. Certainly promises don't work as event listeners. I see no reason not to await for promises as opposed to using .then or regular callbacks. I mean that's what they're intended for.

ljharb commented 4 years ago

The issue isn’t using the await, it’s that doing so forces your test callback to be an async function and thus return a promise, which means that tape (and every other test runner that understands promises) expects that promise to signal the completion of your test.

talmobi commented 4 years ago

Which is why I'll be wrapping it inside an async IIFE instead to bypass the promise behaviour but still get to use await.

But if what you say is true, then why does a test that returns a promise that never resolves complete when its plan count is reached?

Seems like there's some inconsistency there. Like I initially said, to me it meant that when t.plan is used both its promise had to be resolved and its plan count reached for the test to complete. Which isn't currently the case. The test will end when either plan count is reached or the promise is resolved.

Anyway, I guess that's the intended behaviour.

ljharb commented 4 years ago

Yes - t.plan is really not intended for use with "returning a promise", it's for the older style of asynchronous callbacks.