--test-name-pattern not working for async describe suites

[rstagi]

Version

v22.5.1

Platform

Darwin Kernel Version 23.4.0

Subsystem

test runner

What steps will reproduce the bug?

With a suite like the following:

// test.mjs
import { describe, test } from "node:test";

describe("async describe", async (t) => {
  await test("some test");
});

describe("sync describe", () => {
  test("some test");
});

Run the tests with --test-name-pattern="some test":

$ node --test-name-pattern="some test" test.mjs
▶ sync describe
  ✔ some test (0.362916ms)
▶ sync describe (1.022417ms)
ℹ tests 1
ℹ suites 1
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 6.460416

How often does it reproduce? Is there a required condition?

always

What is the expected behavior? Why is that the expected behavior?

I would expect both the test suites async describe and sync describe to work.

What do you see instead?

Only the sync describe suite seem to be working.

Additional information

By running the tests without any test name pattern, they all get executed:

$ node test.mjs
▶ async describe
  ✔ some test (0.477375ms)
▶ async describe (1.367375ms)
▶ sync describe
  ✔ some test (0.066833ms)
▶ sync describe (0.200708ms)
ℹ tests 2
ℹ suites 2
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 7.35

[jakecastelli]

The test with await ran but the reporter didn't collect it.

This code example works prior to v22.0 - points me to #52221

cc. @cjihrig do you mind taking a look? 👀

[rstagi]

Thank you for the pointer @jakecastelli 🙏

While we wait for @cjihrig, I'll take a look myself too. I'll let you know if I find anything ⚙️

[EddieAbbondanzio]

I took a look out of curiosity and it seems to be an async bug related to the logic that changes a suite from filtered to not filtered in postBuild here.

When the example above is ran, the sync suite gets filtered set to false before it's started, but the async suite doesn't get filtered set to false until after it was started which causes the reporter to not collect it.

@cjihrig would probably know best though.

[cjihrig]

Yes, without testing it, I believe what @EddieAbbondanzio is correct. I think the correct fix is to wait for the suites to be finished building before beginning to execute anything.

[cjihrig]

I've thought about this a bit, and I think waiting for the suites to complete is necessary, but not enough to fix this. I'm going to add another commit to #54011 that drops the returned promise from the top level test() and suite() APIs. The reason is that tests return a promise that resolves once the test runs. If we delay running tests until promises returned by tests resolve, we'll be waiting a long time.