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 all 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
18 changes: 16 additions & 2 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.
- `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 @@ -625,6 +628,11 @@ no-op.

* [`AbortSignal`][] Can be used to abort test subtasks when the test has been aborted.

> **Warning**
> On Node.js v14.x, this feature won't be available unless you pass the
> `--experimental-abortcontroller` CLI flag or added an external global polyfill
> for `AbortController`.

```js
test('top level test', async (t) => {
await fetch('some/uri', { signal: t.signal });
Expand Down Expand Up @@ -694,6 +702,12 @@ The name of the suite.

* [`AbortSignal`][] Can be used to abort test subtasks when the test has been aborted.

> **Warning**
> On Node.js v14.x, this feature won't be available unless you pass the
> `--experimental-abortcontroller` CLI flag or added an external global polyfill
> for `AbortController`.


[`AbortSignal`]: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
[TAP]: https://testanything.org/
[`SuiteContext`]: #class-suitecontext
Expand Down
30 changes: 27 additions & 3 deletions lib/internal/abort_controller.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,30 @@
'use strict'

module.exports = {
AbortController,
AbortSignal
if (typeof AbortController === 'undefined') {
// Node.js AbortController implementation is behind a CLI flag.
// This is a minimal mock to make the code work if the native
// implementation in not available.
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
}
}
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