Skip to content

Commit

Permalink
Only treat native errors as errors
Browse files Browse the repository at this point in the history
* Track worker errors. They're not native due to nodejs/node#48716, but we want to treat them as such anyway.
* Only treat native errors as errors
* Remove is-error dependency
* Document edge case where `error instanceof Error` can be true, yet AVA does not recognize `error` as an error

See also #2911 for an earlier attempt.
  • Loading branch information
novemberborn authored Jul 31, 2023
1 parent f2726f1 commit e58f466
Show file tree
Hide file tree
Showing 13 changed files with 107 additions and 16 deletions.
12 changes: 12 additions & 0 deletions docs/08-common-pitfalls.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,18 @@ Translations: [Français](https://github.com/avajs/ava-docs/blob/main/fr_FR/docs

If you use [ESLint](https://eslint.org), you can install [eslint-plugin-ava](https://github.com/avajs/eslint-plugin-ava). It will help you use AVA correctly and avoid some common pitfalls.

## Error edge cases

The `throws()` and `throwsAsync()` assertions use the Node.js built-in [`isNativeError()`](https://nodejs.org/api/util.html#utiltypesisnativeerrorvalue) to determine whether something is an error. This only recognizes actual instances of `Error` (and subclasses).

Note that the following is not a native error:

```js
const error = Object.create(Error.prototype);
```

This can be surprising, since `error instanceof Error` returns `true`.

## AVA in Docker

If you run AVA in Docker as part of your CI, you need to fix the appropriate environment variables. Specifically, adding `-e CI=true` in the `docker exec` command. See [#751](https://github.com/avajs/ava/issues/751).
Expand Down
5 changes: 3 additions & 2 deletions lib/assert.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {isNativeError} from 'node:util/types';

import concordance from 'concordance';
import isError from 'is-error';
import isPromise from 'is-promise';

import concordanceOptions from './concordance-options.js';
Expand Down Expand Up @@ -163,7 +164,7 @@ function validateExpectations(assertion, expectations, numberArgs) { // eslint-d
// Note: this function *must* throw exceptions, since it can be used
// as part of a pending assertion for promises.
function assertExpectations({assertion, actual, expectations, message, prefix, savedError}) {
if (!isError(actual)) {
if (!isNativeError(actual)) {
throw new AssertionError({
assertion,
message,
Expand Down
4 changes: 2 additions & 2 deletions lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {Worker} from 'node:worker_threads';
import Emittery from 'emittery';

import {controlFlow} from './ipc-flow-control.cjs';
import serializeError from './serialize-error.js';
import serializeError, {tagWorkerError} from './serialize-error.js';

let workerPath = new URL('worker/base.js', import.meta.url);
export function _testOnlyReplaceWorkerPath(replacement) {
Expand Down Expand Up @@ -134,7 +134,7 @@ export default function loadFork(file, options, execArgv = process.execArgv) {
});

worker.on('error', error => {
emitStateChange({type: 'worker-failed', err: serializeError('Worker error', false, error, file)});
emitStateChange({type: 'worker-failed', err: serializeError('Worker error', false, tagWorkerError(error), file)});
finish();
});

Expand Down
4 changes: 2 additions & 2 deletions lib/plugin-support/shared-workers.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import events from 'node:events';
import {pathToFileURL} from 'node:url';
import {Worker} from 'node:worker_threads';

import serializeError from '../serialize-error.js';
import serializeError, {tagWorkerError} from '../serialize-error.js';

const LOADER = new URL('shared-worker-loader.js', import.meta.url);

Expand Down Expand Up @@ -34,7 +34,7 @@ function launchWorker(filename, initialData) {
const launched = {
statePromises: {
available: waitForAvailable(worker),
error: events.once(worker, 'error').then(([error]) => error),
error: events.once(worker, 'error').then(([error]) => tagWorkerError(error)),
},
exited: false,
worker,
Expand Down
17 changes: 15 additions & 2 deletions lib/serialize-error.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import path from 'node:path';
import process from 'node:process';
import {fileURLToPath, pathToFileURL} from 'node:url';
import {isNativeError} from 'node:util/types';

import cleanYamlObject from 'clean-yaml-object';
import concordance from 'concordance';
import isError from 'is-error';
import StackUtils from 'stack-utils';

import {AssertionError} from './assert.js';
Expand Down Expand Up @@ -145,8 +145,21 @@ function trySerializeError(error, shouldBeautifyStack, testFile) {
return retval;
}

const workerErrors = new WeakSet();
export function tagWorkerError(error) {
// Track worker errors, which aren't native due to https://github.com/nodejs/node/issues/48716.
// Still include the check for isNativeError() in case the issue is fixed in the future.
if (isNativeError(error) || error instanceof Error) {
workerErrors.add(error);
}

return error;
}

const isWorkerError = error => workerErrors.has(error);

export default function serializeError(origin, shouldBeautifyStack, error, testFile) {
if (!isError(error)) {
if (!isNativeError(error) && !isWorkerError(error)) {
return {
avaAssertionError: false,
nonErrorObject: true,
Expand Down
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@
"globby": "^13.2.1",
"ignore-by-default": "^2.1.0",
"indent-string": "^5.0.0",
"is-error": "^2.2.2",
"is-plain-object": "^5.0.0",
"is-promise": "^4.0.0",
"matcher": "^5.0.0",
Expand Down
13 changes: 13 additions & 0 deletions test/assertions/fixtures/throws-async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import test from 'ava';

test('throws native error', async t => {
await t.throwsAsync(async () => {
throw new Error('foo');
});
});

test('throws object that extends the error prototype', async t => {
await t.throwsAsync(async () => {
throw Object.create(Error.prototype);
});
});
13 changes: 13 additions & 0 deletions test/assertions/fixtures/throws.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import test from 'ava';

test('throws native error', t => {
t.throws(() => {
throw new Error('foo');
});
});

test('throws object that extends the error prototype', t => {
t.throws(() => {
throw Object.create(Error.prototype);
});
});
28 changes: 28 additions & 0 deletions test/assertions/snapshots/test.js.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,31 @@ Generated by [AVA](https://avajs.dev).
't.true(true) passes',
't.truthy(1) passes',
]

## throws requires native errors

> passed tests
[
'throws native error',
]

> failed tests
[
'throws object that extends the error prototype',
]

## throwsAsync requires native errors

> passed tests
[
'throws native error',
]

> failed tests
[
'throws object that extends the error prototype',
]
Binary file modified test/assertions/snapshots/test.js.snap
Binary file not shown.
12 changes: 12 additions & 0 deletions test/assertions/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,15 @@ test('happy path', async t => {
const result = await fixture(['happy-path.js']);
t.snapshot(result.stats.passed.map(({title}) => title));
});

test('throws requires native errors', async t => {
const result = await t.throwsAsync(fixture(['throws.js']));
t.snapshot(result.stats.passed.map(({title}) => title), 'passed tests');
t.snapshot(result.stats.failed.map(({title}) => title), 'failed tests');
});

test('throwsAsync requires native errors', async t => {
const result = await t.throwsAsync(fixture(['throws-async.js']));
t.snapshot(result.stats.passed.map(({title}) => title), 'passed tests');
t.snapshot(result.stats.failed.map(({title}) => title), 'failed tests');
});
10 changes: 5 additions & 5 deletions types/assertions.d.cts
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ export type Assertions = {
snapshot: SnapshotAssertion;

/**
* Assert that the function throws [an error](https://www.npmjs.com/package/is-error). If so, returns the error value.
* Assert that the function throws a native error. If so, returns the error value.
*/
throws: ThrowsAssertion;

/**
* Assert that the async function throws [an error](https://www.npmjs.com/package/is-error), or the promise rejects
* Assert that the async function throws a native error, or the promise rejects
* with one. If so, returns a promise for the error value, which must be awaited.
*/
throwsAsync: ThrowsAsyncAssertion;
Expand Down Expand Up @@ -295,7 +295,7 @@ export type SnapshotAssertion = {

export type ThrowsAssertion = {
/**
* Assert that the function throws [an error](https://www.npmjs.com/package/is-error). If so, returns the error value.
* Assert that the function throws a native error. If so, returns the error value.
* The error must satisfy all expectations. Returns undefined when the assertion fails.
*/
<ErrorType extends ErrorConstructor | Error>(fn: () => any, expectations?: ThrowsExpectation<ErrorType>, message?: string): ThrownError<ErrorType> | undefined;
Expand All @@ -306,13 +306,13 @@ export type ThrowsAssertion = {

export type ThrowsAsyncAssertion = {
/**
* Assert that the async function throws [an error](https://www.npmjs.com/package/is-error). If so, returns the error
* Assert that the async function throws a native error. If so, returns the error
* value. Returns undefined when the assertion fails. You must await the result. The error must satisfy all expectations.
*/
<ErrorType extends ErrorConstructor | Error>(fn: () => PromiseLike<any>, expectations?: ThrowsExpectation<ErrorType>, message?: string): Promise<ThrownError<ErrorType> | undefined>;

/**
* Assert that the promise rejects with [an error](https://www.npmjs.com/package/is-error). If so, returns the
* Assert that the promise rejects with a native error. If so, returns the
* rejection reason. Returns undefined when the assertion fails. You must await the result. The error must satisfy all
* expectations.
*/
Expand Down

0 comments on commit e58f466

Please sign in to comment.