GeezerVison

@aduh95 I'm also hoping to standardise how the tests are written. Making them all ESM instead of a mix avoids someone wondering why some are one and others are another (there's no real reason—the CJS ones are just older).

How about I tackle that in a separate PR? I think if this lands first, then the CJS → ESM PR will have nothing to be reviewed.

PS My phone was sleeping when your comment came in, so I didn't see your message for the commit I just pushed.

There are some lint errors in the commonjs test files that I don't know how to fix:

node-core/require-common-first Mandatory module "common" must be loaded before any other modules

The files are already requiring common at the very top (immediately below 'use strict';),

node-core/async-iife-no-unused-result The result of an immediately-invoked async function needs to be used (e.g. with .then(common.mustCall()))

It's complaining about the (async () => {})() wrapper I added to the CJS tests in order to dynamic import the new spawnAsPromised helper.

The files are already requiring common at the very top

It expects require('../common'), not require('../common/index.js').

It's complaining about the (async () => {})() wrapper I added to the CJS tests in order to dynamic import the new spawnAsPromised helper.

As the error message says, you need to use its result (i.e. add .then(common.mustCall())).

@GeoffreyBooth I assume you would like this to be included in a future v16.x release. The correct label for this is lts-watch-v16.x. Releasers/backporters will add backport-requested-v16.x if they have trouble cherry-picking the commit when a release is being prepared.

@targos Sorry, thanks for the correction 😄 FYI this fixes a bug introduced by an earlier PR that hasn’t been released on 16 yet; see https://openjs-foundation.slack.com/archives/C01810KG1EF/p1659460515402819?thread_ts=1658853609.909249&cid=C01810KG1EF. @cspotcode will add a comment so that hopefully this fix goes out in the same release as the bug (so that the bug never actually gets released in 16.x).

@GeoffreyBooth There is a v16.x release proposal at #44098. This commit would normally not be eligible for inclusion in LTS because it's not been in a Current release yet. If there is a good reason to include it, please add a comment to #44098

I think even better would be to just pull the commit that introduces the bug from the current release proposal, if it’s in there, and then that commit could go out in the same release as this one according to the usual schedule. (Waiting on which commit includes the bug.)

Sorry, what bug does this fix?

I think there's a bit of confusion, this PR doesn't fix it, it's the thing you said you'd address in a PR after this PR lands. If everyone's ok waiting a few hours for me to post all the details, I can do that later today. Won't be able to right this moment.

Ah, yes indeed. I shall put that bugfix together tomorrow (it's a simple fix). I wanted to wait for this to land to avoid a merge conflict in with the new test case for that bug.