fix: make test runner work even if there is no AbortSignal support
#36
Conversation
The out file comes from nodejs/node, the whole point is to use the same tests as Node.js to ensure we are shipping a similar API. Making separate out files would 1) a lot of work 2) harder to review 3) not as useful. |
I am working on |
It's unlikely to ever be backported to v14.x, so I'm not sure it's going to help us. |
Fixes a breaking change for Node.js v14.x that was introduced in 3.2.0. Fixes: nodejs#35
aduh95
force-pushed
the
fix-v14.x-breaking-change
branch
from
1b3f8bb
to
c9fa8a0
Compare
| @@ -84,6 +84,10 @@ const main = async () => { | |||
| for await (const dirent of dir) { | |||
| const ext = extname(dirent.name) | |||
| if (ext === '.js' || ext === '.mjs') { | |||
| if (typeof AbortSignal === 'undefined' && dirent.name.startsWith('test_runner_abort')) { | |||
| console.log('no AbortSignal support, skipping', dirent.name) | |||
There was a problem hiding this comment.
would the smoothest flow be to include an AbortSignal ponyfill for node 14? Are there any major concerns including one?
There was a problem hiding this comment.
I think this would be the nicest option, as long as node 14 is in maintenance lts
There was a problem hiding this comment.
I couldn't find any satisfying ponyfill. Note that we are running tests both with and without --experimental-abort-controller flag, folks who want/need that functionality should pass this flag.
There was a problem hiding this comment.
right, I think this is a good compromise 👍
README.md
Outdated
| @@ -339,7 +342,7 @@ internally. | |||
| - `only` {boolean} If truthy, and the test context is configured to run | |||
| `only` tests, then this test will be run. Otherwise, the test is skipped. | |||
| **Default:** `false`. | |||
| * `signal` {AbortSignal} Allows aborting an in-progress test. | |||
| - `signal` {AbortSignal} Allows aborting an in-progress test (except on Node.js v14.x). | |||
There was a problem hiding this comment.
| - `signal` {AbortSignal} Allows aborting an in-progress test (except on Node.js v14.x). | |
| - `signal` {AbortSignal} Allows aborting an in-progress test (needs `--experimental-abortcontroller` on Node.js v14.x). |
I believe the previous statement wasn't correct?
There was a problem hiding this comment.
Well now that I think about it, users are still allowed to pass an AbortSignal, which they can't pass if they don't have AbortController support themselves anyway. The part that doesn't work is context.signal, I should put the warning on that section instead.
Fixes a breaking change for Node.js v14.x that was introduced in 3.2.0. This also skips some
test/messagethat rely onAbortSignalsupport. I suggest we land this and release in a semver-patch, then we revert and create a new semver-major. Thoughts?Fixes: #35