[BUG] Async and Sync Tests don't run as intended with @tapjs/mocha-global

[barelyhuman]

Is there an existing issue for this?

• I have searched the existing issues

Have you read the CONTRIBUTING guide on posting bugs, and CODE_OF_CONDUCT?

• yes I read the things

This issue exists in the latest tap version

• I am using the latest tap

Description

A lot of test cases written in the mocha style would have multiple it blocks and each of them could either be async or sync and while it works well when all of them are sync, if even one of them is async the remaining tests just fail

Workaround

The current workaround it to use thenable promise chains with the done callback but it isn't optimal to rewrite all tests in a larger codebase.

Error

it() calls may not be nested

Reproduction

https://github.com/barelyhuman/tap-mocha-async-issue

Environment

~/lab/tap-issues main
❯ which tap
tap not found

~/lab/tap-issues main
❯ npm ls tap
tap-issue-repro@1.0.0 /Users/sid/lab/tap-issues
└── tap@18.7.0

~/lab/tap-issues main
❯ npx tap config list
# vim: set filetype=yaml :

# from package.json
mocha-globals: true
plugin:
  - "@tapjs/mocha-globals"

# env, cli, and defaults
color: true
coverage-report:
  - text
exclude:
  - "**/@(fixture*(s)|dist)/**"
include:
  - "**/@(test?(s)|__test?(s)__)/**/*.@(js|cjs|mjs|tap|cts|jsx|mts|ts|tsx)"
  - "**/*.@(test?(s)|spec).@(js|cjs|mjs|tap|cts|jsx|mts|ts|tsx)"
  - "**/test?(s).@(js|cjs|mjs|tap|cts|jsx|mts|ts|tsx)"
jobs: 6
reporter: base
snapshot-clean-cwd: true
timeout: 30

~/lab/tap-issues main
❯ npx tap plugin list
@tapjs/after
@tapjs/after-each
@tapjs/asserts
@tapjs/before
@tapjs/before-each
@tapjs/filter
@tapjs/fixture
@tapjs/intercept
@tapjs/mock
@tapjs/node-serialize
@tapjs/snapshot
@tapjs/spawn
@tapjs/stdin
@tapjs/typescript
@tapjs/worker
@tapjs/mocha-globals

~/lab/tap-issues main
❯ uname -a
Darwin Reapers-MacBook-Air-2.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:19 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T8103 arm64

[isaacs]

Thanks for the repo. I see what's going on, I think it's fixable.

[barelyhuman]

wasn't able to raise a PR for it cause of other work but it's the handling of the returned promise in the plugin that doesn't seem to wait for the dynamically created 'it' blocks.

[isaacs]

Hm, this is a little tricky, actually. Along with #1008, I think there needs to be some extra info attached to the Test objects indicating whether it's a describe or an it function. Which, annoying, because it's all just a Test, and it'd be nice if the Mocha style was more consistent in this way like tap is, but whatever 😅

If there was a way internally to indicate that a Test is designated as a suite vs a it() block, then this problem gets a lot easier, but it's also kind of a rewrite of the mocha plugin.