Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make test runner work even if there is no AbortSignal support #36

Merged
merged 3 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,14 @@ jobs:
strategy:
matrix:
node: ['14', '16', '18']
include:
- node: '14'
env: --experimental-abortcontroller --no-warnings
steps:
- uses: actions/checkout@v3
- uses: actions/setup-node@v3
with:
node-version: ${{ matrix.node }}
- run: npm ci
- run: npm test
- name: Run test with experimental flag
run: npm test
env:
NODE_OPTIONS: ${{ matrix.env }}
NODE_OPTIONS: --experimental-abortcontroller --no-warnings
21 changes: 12 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ Minimal dependencies, with full test suite.
Differences from the core implementation:

- Doesn't hide its own stack frames.
- Requires `--experimental-abortcontroller` CLI flag to work on Node.js v14.x.
- Some features require the use of `--experimental-abortcontroller` CLI flag to
work on Node.js v14.x. It's recommended to pass
`NODE_OPTIONS='--experimental-abortcontroller --no-warnings'` in your env if
you are testing on v14.x.

## Docs

Expand Down Expand Up @@ -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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- `skip` {boolean|string} If truthy, the test is skipped. If a string is
provided, that string is displayed in the test results as the reason for
skipping the test. **Default:** `false`.
Expand Down Expand Up @@ -448,7 +451,7 @@ same as [`it([name], { todo: true }[, fn])`][it options].
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand All @@ -472,7 +475,7 @@ describe('tests', async () => {
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand All @@ -496,7 +499,7 @@ describe('tests', async () => {
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand All @@ -521,7 +524,7 @@ describe('tests', async () => {
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand Down Expand Up @@ -552,7 +555,7 @@ exposed as part of the API.
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand Down Expand Up @@ -580,7 +583,7 @@ test('top level test', async (t) => {
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `signal` {AbortSignal} Allows aborting an in-progress hook (except on Node.js v14.x).
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.
Expand Down Expand Up @@ -664,7 +667,7 @@ execution of the test function. This function does not return a value.
- `skip` {boolean|string} If truthy, the test is skipped. If a string is
provided, that string is displayed in the test results as the reason for
skipping the test. **Default:** `false`.
- `signal` {AbortSignal} Allows aborting an in-progress test.
- `signal` {AbortSignal} Allows aborting an in-progress test (except on Node.js v14.x).
- `todo` {boolean|string} If truthy, the test marked as `TODO`. If a string
is provided, that string is displayed in the test results as the reason why
the test is `TODO`. **Default:** `false`.
Expand Down
28 changes: 25 additions & 3 deletions lib/internal/abort_controller.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,28 @@
'use strict'

module.exports = {
AbortController,
AbortSignal
if (typeof AbortController === 'undefined') {
module.exports = {
AbortController: class AbortController {
#eventListeners = new Set()
signal = {
aborted: false,
addEventListener: (_, listener) => {
this.#eventListeners.add(listener)
},
removeEventListener: (_, listener) => {
this.#eventListeners.delete(listener)
}
}

abort () {
this.signal.aborted = true
this.#eventListeners.forEach(listener => listener())
}
}
}
} else {
module.exports = {
AbortController,
AbortSignal
}
}
4 changes: 2 additions & 2 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ function expectsError (validator, exact) {
}, exact)
}

if (typeof AbortSignal.timeout !== 'function') {
if (typeof AbortSignal !== 'undefined' && typeof AbortSignal.timeout !== 'function') {
// `AbortSignal.timeout` is not available on Node.js 14.x, we need to polyfill
// it because some tests are using it. End-users don't need to it.

Expand All @@ -121,7 +121,7 @@ if (typeof AbortSignal.timeout !== 'function') {
}
}

if (process.version.startsWith('v14.') || process.version.startsWith('v16.')) {
if (typeof AbortSignal !== 'undefined' && (process.version.startsWith('v14.') || process.version.startsWith('v16.'))) {
// Implementation of AbortSignal and AbortController differ slightly with the
// v18.x one, creating some difference on the TAP output which makes the tests
// fail. We are overriding the built-ins to make the test pass, however that's
Expand Down
4 changes: 4 additions & 0 deletions test/message.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would the smoothest flow be to include an AbortSignal ponyfill for node 14? Are there any major concerns including one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be the nicest option, as long as node 14 is in maintenance lts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I think this is a good compromise 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

continue
}
const filePath = join(MESSAGE_FOLDER, dirent.name)
const expected = filePath.replace(/\.m?js$/, '.out')
const testFile = await fs.open(filePath)
Expand Down