-
Notifications
You must be signed in to change notification settings - Fork 676
fix(assert): hint that function properties compare by reference #7165
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,41 @@ import { format } from "@std/internal/format"; | |
|
|
||
| import { AssertionError } from "./assertion_error.ts"; | ||
|
|
||
| // Walks `value` (avoiding cycles) and returns true if any own property, | ||
| // array element, Map value, or Set element is a function. Used to surface | ||
| // a hint when assertEquals throws on inputs whose only difference is a | ||
| // function property, which prints as an identical-looking `[Function: …]` | ||
| // in the diff but compares by reference. | ||
| function containsFunction(value: unknown, seen: WeakSet<object>): boolean { | ||
| if (typeof value === "function") return true; | ||
| if (value === null || typeof value !== "object") return false; | ||
| if (seen.has(value as object)) return false; | ||
| seen.add(value as object); | ||
| if (value instanceof Map) { | ||
| for (const v of value.values()) { | ||
| if (containsFunction(v, seen)) return true; | ||
| } | ||
| return false; | ||
| } | ||
| if (value instanceof Set) { | ||
| for (const v of value.values()) { | ||
| if (containsFunction(v, seen)) return true; | ||
| } | ||
| return false; | ||
| } | ||
| for (const k of Reflect.ownKeys(value as object)) { | ||
| if ( | ||
| containsFunction( | ||
| (value as Record<string | symbol, unknown>)[k], | ||
| seen, | ||
| ) | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Make an assertion that `actual` and `expected` are equal, deeply. If not | ||
| * deeply equal, then throw. | ||
|
|
@@ -54,6 +89,14 @@ export function assertEquals<T>( | |
| const msgSuffix = msg ? `: ${msg}` : "."; | ||
| let message = `Values are not equal${msgSuffix}`; | ||
|
|
||
| if ( | ||
| containsFunction(actual, new WeakSet()) || | ||
| containsFunction(expected, new WeakSet()) | ||
| ) { | ||
| message += | ||
| "\n Note: function properties are compared by reference, so two distinct functions print the same as `[Function: name]` but are not equal."; | ||
| } | ||
|
Comment on lines
+92
to
+98
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main concern: this heuristic is too broad and produces noisy false positives. The note fires whenever a function exists anywhere in either operand, even when the function is identical on both sides and unrelated to the inequality. Common case: const handler = () => {};
assertEquals({ x: 1, onClick: handler }, { x: 2, onClick: handler });Here Suggest gating on the genuinely-confusing condition: the formatted strings are identical but the values are unequal. That's exactly the issue's scenario, never fires when the diff is already meaningful, and skips the tree walk in the common case: const actualString = format(actual);
const expectedString = format(expected);
if (
actualString === expectedString &&
(containsFunction(actual, new WeakSet()) ||
containsFunction(expected, new WeakSet()))
) {
message += "\n Note: ...";
}This requires moving the check below the |
||
|
|
||
| const actualString = format(actual); | ||
| const expectedString = format(expected); | ||
| const stringDiff = (typeof actual === "string") && | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,6 +207,57 @@ Deno.test({ | |
| }, | ||
| }); | ||
|
|
||
| Deno.test({ | ||
| name: | ||
| "assertEquals() adds a function-by-reference hint when functions are present", | ||
| fn() { | ||
| // https://github.com/denoland/std/issues/6878 | ||
| // Identical-looking function properties print as `[Function: y]` on | ||
| // both sides, which makes the failure confusing. The hint clarifies | ||
| // that functions compare by reference. | ||
| const error = assertThrows( | ||
| () => | ||
| assertEquals( | ||
| { x: 1, y: () => 2 }, | ||
| { x: 1, y: () => 2 }, | ||
| ), | ||
| AssertionError, | ||
| ); | ||
| const message = stripAnsiCode((error as AssertionError).message); | ||
| if (!message.includes("function properties are compared by reference")) { | ||
| throw new Error( | ||
| `expected message to include the function-reference hint, got:\n${message}`, | ||
| ); | ||
|
Comment on lines
+226
to
+230
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor style: this file already imports from |
||
| } | ||
|
|
||
| // Also fires for function values nested in arrays and Maps. | ||
| const arrayError = assertThrows( | ||
| () => assertEquals([() => 1], [() => 1]), | ||
| AssertionError, | ||
| ); | ||
| if ( | ||
| !stripAnsiCode((arrayError as AssertionError).message).includes( | ||
| "function properties are compared by reference", | ||
| ) | ||
| ) { | ||
| throw new Error("expected hint for array of functions"); | ||
| } | ||
|
|
||
| // Does NOT fire when neither side has any function values. | ||
| const plainError = assertThrows( | ||
| () => assertEquals({ x: 1 }, { x: 2 }), | ||
| AssertionError, | ||
| ); | ||
| if ( | ||
| stripAnsiCode((plainError as AssertionError).message).includes( | ||
| "function properties are compared by reference", | ||
| ) | ||
| ) { | ||
| throw new Error("hint must not appear when there are no functions"); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| Deno.test({ | ||
| name: "assertEquals() matches same Set with object keys", | ||
| fn() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reflect.ownKeys+value[k]invokes getters, including throwing ones.format()already inspects values so this is mostly pre-existing, but it means getters get invoked an extra time on the failure path, and a throwing getter would surface as a confusing error inside the assertion failure rather than the originalAssertionError. Worth atry/catcharound the property access. (TheactualString === expectedStringgate suggested above also shrinks this exposure to the rare identical-string case.)