fix(assert): hint at reference-equality when assertEquals diff is empty (#6878)#7158
fix(assert): hint at reference-equality when assertEquals diff is empty (#6878)#7158LeSingh1 wants to merge 3 commits into
Conversation
…6878) `assertEquals` throws "Values are not equal" but renders an empty diff when actual/expected stringify identically yet are deemed unequal — the common case is a function property compared by reference. The empty diff leaves users hunting for invisible differences. Detect the case (diff result has zero `added`/`removed` entries) and append a one-line hint explaining that functions, Promises, Requests, Blobs, and other built-ins are compared by reference, so two distinct instances are never equal even when their representations match. Skip the hint on real textual diffs and on string-vs-string comparisons (which always produce visible add/remove if they differ). Two new tests cover the hint firing path and the negative case. Full assert suite (141 tests) stays green. Direction matches @lionel-rowe's suggestion in the issue thread.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7158 +/- ##
=======================================
Coverage 94.63% 94.63%
=======================================
Files 629 630 +1
Lines 51894 51908 +14
Branches 9373 9377 +4
=======================================
+ Hits 49108 49122 +14
Misses 2217 2217
Partials 569 569 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bartlomieju
left a comment
There was a problem hiding this comment.
This is a clean fix and, I think, the right approach to #6878. By keying off an empty diff (no added/removed lines) rather than walking the value tree, it avoids false positives — an object with an identical function reference on both sides but a different scalar field still produces a real diff line, so the hint correctly stays silent. It also adds no extra traversal and re-invokes no getters. And because @std/internal/format runs with depth/iterableLimit/strAbbreviateSize: Infinity, two values can't stringify identically merely from inspect truncation, so an empty diff really does imply a reference-compared distinction — the broader wording (Promises, Requests, Blobs, distinct Symbols, Weak* types) is accurate, not just plausible.
Only minor cleanups inline; nothing blocking.
Process note: this overlaps with your #7165, which tackles the same issue via a containsFunction tree-walk. The two are mutually exclusive. I'd land this one and close #7165 — it's simpler, avoids the false-positive and getter-re-invocation issues, and produces a more general/accurate hint. Worth cross-linking so they aren't reviewed independently.
| // a hint pointing at the likely cause. | ||
| if ( | ||
| !stringDiff && | ||
| diffResult.every((r) => r.type !== "added" && r.type !== "removed") |
There was a problem hiding this comment.
DiffResult.type is "added" | "removed" | "common", so this reads more directly as diffResult.every((r) => r.type === "common").
| message = `${message}\n` + | ||
| " Note: values stringify identically but are not structurally equal. " + | ||
| "Functions, Promises, Requests, Blobs, and other built-ins are compared by " + | ||
| "reference, so two distinct instances are never equal even when their " + | ||
| "representations match."; |
There was a problem hiding this comment.
Minor: the Note: text hard-codes 4 leading spaces to align with buildMessage's [Diff] block — an implicit coupling to that formatter's indentation. Fine to leave, but a one-line comment noting why the 4 spaces are there would help the next person who touches it.
| let caught: AssertionError | undefined; | ||
| try { | ||
| assertEquals( | ||
| { x: 1, y: () => 2 }, | ||
| { x: 1, y: () => 2 }, | ||
| ); | ||
| } catch (e) { | ||
| caught = e as AssertionError; | ||
| } | ||
| if (!caught) throw new Error("Expected assertEquals to throw"); | ||
| assertStringIncludes(caught.message, "Values are not equal"); | ||
| assertStringIncludes( | ||
| caught.message, | ||
| "stringify identically but are not structurally equal", | ||
| ); | ||
| assertStringIncludes(caught.message, "compared by reference"); | ||
| }, |
There was a problem hiding this comment.
Style nit: this file already imports assertThrows, which returns the thrown error — that's cleaner than the hand-rolled try/catch + if (!caught) throw:
const error = assertThrows(
() => assertEquals({ x: 1, y: () => 2 }, { x: 1, y: () => 2 }),
AssertionError,
);
assertStringIncludes(error.message, "Values are not equal");
assertStringIncludes(error.message, "stringify identically but are not structurally equal");
assertStringIncludes(error.message, "compared by reference");| let caught: AssertionError | undefined; | ||
| try { | ||
| assertEquals({ x: 1 }, { x: 2 }); | ||
| } catch (e) { | ||
| caught = e as AssertionError; | ||
| } | ||
| if (!caught) throw new Error("Expected assertEquals to throw"); | ||
| assertEquals( | ||
| caught.message.includes("stringify identically"), | ||
| false, | ||
| "Hint should only fire when the diff is empty", | ||
| ); |
There was a problem hiding this comment.
Same assertThrows-returns-the-error simplification applies here. For the negative check, assert(!error.message.includes("stringify identically")) (or assertNotMatch) is clearer than assertEquals(..., false, msg).
Fixes #6878.
When
assertEqualsdecides two values are unequal but they stringify identically (typically because a property is a function compared by reference), the rendered diff is empty:That leaves users hunting for invisible differences. @lionel-rowe pointed at the underlying cause in the thread (functions, Promises, Requests, Blobs etc. fall back to reference equality) and suggested the diff should "explicitly state when there are invisible differences."
Fix
After building the diff, check whether it contains any
added/removedentries. If not, append:The hint is skipped on string-vs-string comparisons (which always produce a visible diff when they differ) and on real textual diffs (so existing failures aren't noisier).
Tests
Two cases in
assert/equals_test.ts:hints at reference-equality when objects with function props stringify identically (#6878)— reproduces the exact issue body's call and asserts the new hint text is present.does not append reference-equality hint when there is a real textual diff—assertEquals({ x: 1 }, { x: 2 })must NOT include the hint.