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

Only treat native errors as errors #3229

Merged
merged 3 commits into from
Jul 31, 2023
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
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