Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 4 additions & 0 deletions docs/test/writing-tests.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,8 @@ A tester registered in a test file applies until that file ends. A tester regist

To compare nested values inside a tester, call `this.equals(a, b, customTesters)`. `customTesters` is the third argument the tester receives.

A tester returns its answer synchronously. If a tester returns a promise or another thenable, the matcher throws a `TypeError`.

## Test State

`expect.getState()` returns information about the running test. Custom matchers can read the properties below from `this`.
Expand Down Expand Up @@ -873,6 +875,8 @@ test("the callback is a function", () => {
});
```

The `predicate` of `expect.toSatisfy()` and an `asymmetricMatch()` method return their answer synchronously, like an [equality tester](#custom-equality). If one of them returns a promise or another thenable, the matcher throws a `TypeError`.

## Best Practices

### Use Descriptive Test Names
Expand Down
6 changes: 6 additions & 0 deletions packages/bun-types/test.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,9 @@ declare module "bun:test" {
* to the next tester, and then to the built-in rules. Testers registered by a test file last
* until that file ends. Testers registered by a preload script apply to every file.
*
* A tester returns its answer synchronously. If it returns a promise or another thenable, the
* matcher throws a `TypeError`.
*
* @example
* expect.addEqualityTesters([
* (a, b) => (a instanceof Volume && b instanceof Volume ? a.equals(b) : undefined),
Expand Down Expand Up @@ -1839,6 +1842,9 @@ declare module "bun:test" {
* `message` describes the predicate. It has no effect on the match. It is printed after the
* predicate wherever the matcher is printed, such as in a failure message or a snapshot.
*
* `predicate` returns its answer synchronously. If it returns a promise or another thenable,
* the matcher throws a `TypeError`.
*
* @example
* const isOdd = (n: number) => n % 2 === 1;
* expect({ count: 3 }).toEqual({ count: expect.toSatisfy(isOdd) });
Expand Down
31 changes: 25 additions & 6 deletions src/jsc/bindings/bindings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,29 @@ static NEVER_INLINE std::optional<bool> runEqualityTesters(JSGlobalObject* globa
return verdict != 0;
}

// Must match `Answerer` in expect_deferred.rs.
enum class ExpectAnswerer : uint8_t {
EqualityTester,
AsymmetricMatch,
IsEqualNode,
SatisfyPredicate,
Contains,
};

extern "C" void Expect__refuseThenable(JSC::JSGlobalObject* globalObject, JSC::EncodedJSValue answer, ExpectAnswerer who);

// `Answerer::ask` of expect_deferred.rs: a promise that user code returns is no answer, so it throws.
static ALWAYS_INLINE bool askPredicate(JSGlobalObject* globalObject, ThrowScope& scope, ExpectAnswerer who, JSValue function, JSValue thisValue, const ArgList& arguments)
{
JSValue answer = JSC::call(globalObject, function, JSC::getCallData(function), thisValue, arguments);
RETURN_IF_EXCEPTION(scope, false);
if (answer.isObject()) [[unlikely]] {
Expect__refuseThenable(globalObject, JSValue::encode(answer), who);
RETURN_IF_EXCEPTION(scope, false);
}
return answer.toBoolean(globalObject);
}

// Rules out most objects without a property lookup.
static ALWAYS_INLINE bool mayHaveAsymmetricMatchMethod(JSGlobalObject* globalObject, JSValue value)
{
Expand Down Expand Up @@ -755,9 +778,7 @@ static NEVER_INLINE std::optional<bool> matchUserAsymmetricMatcher(JSGlobalObjec
JSValue other = methodOfA ? b : a;
MarkedArgumentBuffer arguments;
arguments.append(other ? other : jsUndefined());
JSValue matched = JSC::call(globalObject, method, JSC::getCallData(method), methodOfA ? a : b, arguments);
RETURN_IF_EXCEPTION(scope, false);
return matched.toBoolean(globalObject);
return askPredicate(globalObject, scope, ExpectAnswerer::AsymmetricMatch, method, methodOfA ? a : b, arguments);
}

// jsdom and happy-dom nodes are class instances, sometimes wrapped in a Proxy.
Expand Down Expand Up @@ -854,9 +875,7 @@ static NEVER_INLINE std::optional<bool> domNodesDequal(JSGlobalObject* globalObj

MarkedArgumentBuffer arguments;
arguments.append(o2);
JSValue result = JSC::call(globalObject, isEqualNode, JSC::getCallData(isEqualNode), o1, arguments);
RETURN_IF_EXCEPTION(scope, std::nullopt);
return result.toBoolean(globalObject);
return askPredicate(globalObject, scope, ExpectAnswerer::IsEqualNode, isEqualNode, o1, arguments);
}

JSValue getIndexWithoutAccessors(JSGlobalObject* globalObject, JSObject* obj, uint64_t i)
Expand Down
7 changes: 3 additions & 4 deletions src/runtime/test_runner/expect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use bun_ptr::RefPtr;

use super::bun_test::{self};
use super::diff_format::DiffFormatter;
use self::expect_deferred::{Asked, ExpectDeferred, MatcherCall, Pass, Question};
use self::expect_deferred::{Answerer, Asked, ExpectDeferred, MatcherCall, Pass, Question};
use super::execution::ExpectAssertions;
use super::jest::Jest;
use super::pretty_format::JestPrettyFormat;
Expand Down Expand Up @@ -1504,9 +1504,8 @@ impl Expect {
predicate: JsLocal<'_>,
) -> JsResult<JSValue> {
let global_this = cx.global();
let pass = predicate
.to_js()
.call(global_this, JSValue::UNDEFINED, &[received.to_js()])?
let pass = Answerer::SatisfyPredicate
.ask(global_this, predicate.to_js(), JSValue::UNDEFINED, &[received.to_js()])?
Comment thread
robobun marked this conversation as resolved.
Resolved
.to_boolean();
let result = JSValue::create_empty_object(global_this, 1);
result.put(global_this, b"pass", JSValue::from(pass));
Expand Down
3 changes: 2 additions & 1 deletion src/runtime/test_runner/expect/addEqualityTesters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use bun_jsc::{JSGlobalObject, JSValue, JsClass as _, JsResult};

use super::super::jest::{FileColumns as _, Jest};
use crate::generated_bindgen::expect::js_ExpectMatcherUtils as js;
use super::expect_deferred::Answerer;
use super::{Expect, ExpectMatcherContext, ExpectMatcherUtils, Flags};

impl Expect {
Expand Down Expand Up @@ -131,7 +132,7 @@ impl Expect {
tester,
));
}
let verdict = tester.call(global, context, &[a, b, testers])?;
let verdict = Answerer::EqualityTester.ask(global, tester, context, &[a, b, testers])?;
if !verdict.is_undefined() {
return Ok(Some(verdict.to_boolean()));
}
Expand Down
3 changes: 2 additions & 1 deletion src/runtime/test_runner/expect/toContain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use bun_jsc::{JSGlobalObject, JSValue, JsResult, VM};
use bun_core::strings;

use super::Expect;
use super::expect_deferred::Answerer;
use super::get_signature;
use super::throw;

Expand Down Expand Up @@ -68,7 +69,7 @@ impl Expect {
)));
}
let contains = value.get(global, "contains")?.unwrap_or(JSValue::UNDEFINED);
pass = contains.call(global, value, &[expected])?.to_boolean();
pass = Answerer::Contains.ask(global, contains, value, &[expected])?.to_boolean();
} else if value.is_iterable(global)? {
let mut expected_entry = ExpectedEntry {
global,
Expand Down
68 changes: 68 additions & 0 deletions src/runtime/test_runner/expect_deferred.rs
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,74 @@ impl Pass {
}
}

/// Must match `ExpectAnswerer` in bindings.cpp.
#[derive(Copy, Clone)]
pub(crate) enum Answerer {
EqualityTester,
AsymmetricMatch,
IsEqualNode,
SatisfyPredicate,
Contains,
}

impl Answerer {
const ALL: [Answerer; 5] = [
Answerer::EqualityTester,
Answerer::AsymmetricMatch,
Answerer::IsEqualNode,
Answerer::SatisfyPredicate,
Answerer::Contains,
];

/// Calls a user function whose answer decides a comparison at once. A promise is no answer, so it throws.
#[inline]
pub(crate) fn ask(
self,
global: &JSGlobalObject,
function: JSValue,
this: JSValue,
arguments: &[JSValue],
) -> JsResult<JSValue> {
let answer = function.call(global, this, arguments)?;
if answer.is_object() {
self.refuse_thenable(global, answer)?;
}
Ok(answer)
}

#[cold]
#[optimize(size)]
fn refuse_thenable(self, global: &JSGlobalObject, answer: JSValue) -> JsResult<()> {
let returned = if let Some(promise) = answer.as_any_promise() {
// The error below reports it. As an unhandled rejection it would also fail whichever test runs then.
promise.set_handled(global.vm());
Comment thread
robobun marked this conversation as resolved.
Resolved
"a promise"
} else if answer.get(global, "then")?.is_some_and(JSValue::is_callable) {
"a thenable"
} else {
return Ok(());
};
let who = match self {
Answerer::EqualityTester => "An equality tester",
Answerer::AsymmetricMatch => "asymmetricMatch()",
Answerer::IsEqualNode => "isEqualNode()",
Answerer::SatisfyPredicate => "The predicate of expect.toSatisfy()",
Answerer::Contains => "contains()",
};
Err(global.throw_type_error(format_args!(
"{who} returned {returned}. It has to return its answer synchronously"
)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 (optional) Suites that import expect from "vitest" and register an async equality tester pass today in Vitest 5.0.3 and on the base branch, but every toEqual that reaches the tester now fails with a TypeError. expect_deferred.rs:601-603 throws for both module flavours; nothing in refuse_thenable checks the vitest flag, although the runner otherwise keys behaviour on the imported module (toContain.rs:64 is_vitest). Fix: either gate the refusal so the "vitest" flavour keeps Vitest's truthy coercion, or get the maintainer's explicit decision recorded in the PR before merge and add a changelog note for migrating suites. [also at: src/runtime/test_runner/expect_deferred.rs:601 - Suites that import from "vitest" and run under Bun now fail with a TypeError where real Vitest 5.0.3 and the base branch pass, because the vitest flavor gets the same refusal as bun:test. expect_deferred.rs:594-601 throws for every Answerer regardless of module flavor, and the PR's own table shows Vitest counts an async tester's or asymmetricMatch()'s promise as true.]

Why this was flagged

Trigger: a project migrating from Vitest runs under Bun with import { expect } from "vitest" and has expect.addEqualityTesters([async (a, b) => ...]) or expect.toSatisfy(async ...) in a preload or setup file, which Vitest 5.0.3 accepts (the promise counts as true). Entry: Expect__runEqualityTesters (addEqualityTesters.rs:145) from C++ deep equality for every toEqual/toStrictEqual/toHaveBeenCalledWith in those files. The new code at expect_deferred.rs:589-603 throws regardless of flavour; the PR's own table shows Vitest passes 21 of 42 rows where this PR fails. On the base branch such suites run green under the "vitest" module. The PR text itself asks the maintainer whether the "vitest" flavour must keep Vitest's rule, so the decision is unresolved at merge time. Remedy: branch on the vitest flavour or record the decision.

Verification: src/runtime/test_runner/expect_deferred.rs refuse_thenable throws a TypeError for a promise with no flavour check; addEqualityTesters.rs:135 now routes every tester call through Answerer::EqualityTester.ask(...), so on the base branch import { expect } from "vitest"; expect.addEqualityTesters([async () => false]); expect({a:1}).toEqual({a:1}) passed and after this PR it throws.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No change for now. The rule for the "vitest" module is a question to the maintainer in the PR description ("Second question" in Notes). The Vitest result for this input is the silent pass that this PR removes, so the PR applies one rule to both modules until a maintainer decides.

Comment thread
robobun marked this conversation as resolved.
Resolved
}

/// Called by C++ deep equality with an object that `who` returned. Leaves an exception pending if it is refused.
#[unsafe(no_mangle)]
pub(crate) extern "C" fn Expect__refuseThenable(global: &JSGlobalObject, answer: JSValue, who: u8) {
if let Err(error) = Self::ALL[usize::from(who)].refuse_thenable(global, answer) {
bun_jsc::host_fn::host_call_error_value(global, error);
}
}
}

/// A matcher call waiting on a promise. [`Pass::wait_for`] throws it and `Expect::matcher_threw` catches it.
pub(crate) struct ExpectDeferred {
held: JsCell<Option<RunningEntry>>,
Expand Down
91 changes: 91 additions & 0 deletions test/js/bun/test/expect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5369,6 +5369,97 @@ describe("expect()", () => {
expect(() => expect({ n: 1 }).toMatchObject({ n: throws })).toThrow(new RangeError("thrown by the matcher"));
});

// Jest and Vitest take the promise itself for the answer, and don't call an async asymmetricMatch() at all.
test_skipIf(!isBun)("asymmetricMatch() that returns a promise or another thenable throws", async () => {
const refused = (/** @type {string} */ returned) =>
`asymmetricMatch() returned ${returned}. It has to return its answer synchronously`;
const rejectsLater = Promise.withResolvers();
class InheritsThen {
then() {}
}
const called = jest.fn();
called(5);
const cases = /** @type {const} */ ([
[{ asymmetricMatch: async () => true }, "a promise"],
[{ asymmetricMatch: async () => false }, "a promise"],
[{ asymmetricMatch: () => Promise.resolve(undefined) }, "a promise"],
[{ asymmetricMatch: () => Promise.reject(new Error("rejected before it was returned")) }, "a promise"],
[{ asymmetricMatch: () => rejectsLater.promise }, "a promise"],
[{ asymmetricMatch: () => ({ then() {} }) }, "a thenable"],
[{ asymmetricMatch: () => new InheritsThen() }, "a thenable"],
]);
for (const [matcher, returned] of cases) {
for (const assertion of [
() => expect(5).toEqual(matcher),
() => expect(matcher).toEqual(5),
() => expect([5]).not.toEqual([matcher]),
() => expect({ n: 5 }).toStrictEqual({ n: matcher }),
() => expect({ n: 5, other: 1 }).toMatchObject({ n: matcher }),
() => expect([5]).toContainEqual(matcher),
() => expect(called).toHaveBeenCalledWith(matcher),
() => expect({ n: 5, other: 1 }).toEqual(expect.objectContaining({ n: matcher })),
() => expect([5]).toEqual(expect.arrayContaining([matcher])),
]) {
expect(assertion).toThrow(TypeError);
expect(assertion).toThrow(new TypeError(refused(returned)));
}
}

// An unhandled rejection is reported once the microtasks have run, and fails the test that is running then.
rejectsLater.reject(new Error("rejected after the matcher threw"));
await new Promise(resolve => setImmediate(resolve));
});

test_skipIf(!isBun)(
"a promise from asymmetricMatch() is refused on the re-run of a matcher that waited",
async () => {
expect.extend({
async toPassLater() {
await 0;
return { pass: true, message: () => "" };
},
});
const asymmetricMatch = jest.fn(async () => true);
const assertion = expect([1, 5]).toEqual([ANY(expect).toPassLater(), { asymmetricMatch }]);
expect(asymmetricMatch).toHaveBeenCalledTimes(0);
await expect(assertion).rejects.toThrow(
new TypeError("asymmetricMatch() returned a promise. It has to return its answer synchronously"),
);
expect(asymmetricMatch).toHaveBeenCalledTimes(1);
},
);

test_skipIf(!isBun)("an object from asymmetricMatch() is an answer unless its `then` is a function", () => {
const answering = (/** @type {unknown} */ answer) => ({ asymmetricMatch: () => answer });
for (const answer of [{}, { then: true }, [], () => {}]) {
expect([5]).toEqual([answering(answer)]);
expect(() => expect([5]).not.toEqual([answering(answer)])).toThrow("toEqual");
}

const then = jest.fn(() => undefined);
expect([5]).toEqual([answering(Object.defineProperty({}, "then", { get: then }))]);
expect(then).toHaveBeenCalledTimes(1);

const reason = new RangeError("then was read");
const throwingGetter = {
get then() {
throw reason;
},
};
const throwingTrap = new Proxy(
{},
{
get() {
throw reason;
},
},
);
for (const answer of [throwingGetter, throwingTrap]) {
expect(() => expect([5]).toEqual([answering(answer)])).toThrow(reason);
expect(() => expect({ n: 5 }).not.toMatchObject({ n: answering(answer) })).toThrow(reason);
}
});

test("two matchers are compared as plain objects", () => {
const never = () => false;
expect({ asymmetricMatch: never }).toEqual({ asymmetricMatch: never });
Expand Down
29 changes: 29 additions & 0 deletions test/js/bun/test/jest-extended.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,35 @@ describe("jest-extended", () => {
expect(make).toThrow("Expected a function");
}
});

test("expect.toSatisfy() throws if its predicate returns a promise or another thenable", async () => {
const refused = returned =>
`The predicate of expect.toSatisfy() returned ${returned}. It has to return its answer synchronously`;
const rejectsLater = Promise.withResolvers();
for (const [predicate, returned] of [
[async () => true, "a promise"],
[async () => false, "a promise"],
[() => Promise.reject(new Error("rejected before it was returned")), "a promise"],
[() => rejectsLater.promise, "a promise"],
[() => ({ then() {} }), "a thenable"],
]) {
for (const assertion of [
() => expect(7).toEqual(expect.toSatisfy(predicate)),
() => expect(7).not.toEqual(expect.toSatisfy(predicate)),
() => expect({ a: 7 }).toEqual({ a: expect.not.toSatisfy(predicate) }),
() => expect([7]).toContainEqual(expect.toSatisfy(predicate)),
() => expect.toSatisfy(predicate).asymmetricMatch(7),
]) {
expect(assertion).toThrow(TypeError);
expect(assertion).toThrow(new TypeError(refused(returned)));
}
}
expect(7).toEqual(expect.toSatisfy(() => ({ then: true })));

// An unhandled rejection is reported once the microtasks have run, and fails the test that is running then.
rejectsLater.reject(new Error("rejected after the matcher threw"));
await new Promise(resolve => setImmediate(resolve));
});
}

// Array
Expand Down
29 changes: 29 additions & 0 deletions test/js/bun/test/printing/dom-nodes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,35 @@ describe("equality", () => {
for (const falsy of [0, "", null, undefined]) expect(returning(falsy)).not.toEqual(h("i"));
});

test("isEqualNode() that returns a promise or another thenable throws", async () => {
const returning = (returned: unknown) => Object.assign(h("i"), { isEqualNode: () => returned });
const refused = (returned: string) =>
`isEqualNode() returned ${returned}. It has to return its answer synchronously`;
const rejectsLater = Promise.withResolvers<boolean>();
for (const [returned, described] of [
[Promise.resolve(true), "a promise"],
[Promise.resolve(false), "a promise"],
[Promise.reject(new Error("rejected before it was returned")), "a promise"],
[rejectsLater.promise, "a promise"],
[{ then() {} }, "a thenable"],
] as const) {
for (const assertion of [
() => expect(returning(returned)).toEqual<Node>(h("i")),
() => expect(returning(returned)).not.toEqual(h("i")),
() => expect(returning(returned)).toStrictEqual<Node>(h("i")),
() => expect({ n: returning(returned) }).toMatchObject({ n: h("i") }),
]) {
expect(assertion).toThrow(TypeError);
expect(assertion).toThrow(new TypeError(refused(described)));
}
}
expect(returning({ then: true })).toEqual<Node>(h("b"));

// An unhandled rejection is reported once the microtasks have run, and fails the test that is running then.
rejectsLater.reject(new Error("rejected after the matcher threw"));
await new Promise(resolve => setImmediate(resolve));
});

test("toMatchObject uses isEqualNode() for two nodes and properties for a plain pattern", () => {
expect({ n: h("i", { a: "1" }), x: 1 }).toMatchObject({ n: h("i", { a: "1" }) });
expect({ n: h("i", { a: "1" }), x: 1 }).not.toMatchObject({ n: h("i", { a: "2" }) });
Expand Down
Loading
Loading