chore(eslint): require descriptive messages on boolean test assertions#8537
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Overall package sizeSelf size: 5.84 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
48a5a27 to
b8a6087
Compare
|
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
b8a6087 to
18217ad
Compare
BenchmarksBenchmark execution time: 2026-05-20 19:49:40 Comparing candidate commit 5c3322e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1505 metrics, 88 unstable metrics. |
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
18217ad to
30bc86a
Compare
watson
changed the base branch from
graphite-base/8537
to
watson/eslint-equal-assert
watson
force-pushed
the
watson/eslint-equal-assert
branch
from
f0d9d5c to
eddfeab
Compare
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
30bc86a to
ca8199d
Compare
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
ca8199d to
b220b28
Compare
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
b220b28 to
02d15e5
Compare
watson
changed the base branch from
graphite-base/8537
to
watson/eslint-rule-prefer-match
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
02d15e5 to
f2ac003
Compare
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
3 times, most recently
from
941aa34 to
d24554d
Compare
Add a custom ESLint rule that flags `assert(...)` / `assert.ok(...)`
calls whose first argument is a non-trivial expression and no second
message argument is provided. Without a message, failures only report
"Expected true, got false", hiding the actual value being asserted on —
debugging a flaky `assert.ok(duration >= 1000)` is painful when you
can't tell if `duration` was 500ms or 5ms.
Self-describing shapes are still allowed without a message: bare
references (`isReady`, `obj.prop`), structural unary ops (`!x`,
`typeof x`, `delete obj.k`), `in` / `instanceof` with trivial operands,
predicate-style calls (`arr.includes('foo')`, `Array.isArray(x)`), and
zero-arg method-chain calls used as getter-style navigation
(`span.context()._tags`).
Scoped to test files via the existing `TEST_FILES` glob.
watson
force-pushed
the
watson/eslint-boolean-assert-rule
branch
from
d24554d to
3967cf5
Compare
watson
requested review from
khanayan123,
leoromanovsky,
sameerank and
tlhunter
and removed request for
a team
watson
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce0e0ced6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const lhsPart = firstArg.left.type === 'Literal' ? lhsText : '${' + lhsText + '}' | ||
| const rhsPart = firstArg.right.type === 'Literal' ? rhsText : '${' + rhsText + '}' |
There was a problem hiding this comment.
${ literals
When eslint --fix sees a comparison against a literal string containing ${...} (for example assert.ok(value == '${expected}')), these lines copy the literal source directly into the generated template-literal message, so the fixed code becomes a message that interpolates expected instead of showing the literal text and can even throw a ReferenceError before assert.ok runs. Please bail out or escape template-literal syntax for literal operands before returning a fix.
Useful? React with 👍 / 👎.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
LGTM, while I do not have a strong opinion. It will be nicer to read, while it probably does not make a big difference 🤷
For literal operands, `buildAutofixMessage` embeds the source text
verbatim into the synthesised template's static segments. When that
text contains `${...}`, it turns into a real interpolation —
silently changing the message, or throwing `ReferenceError` if the
identifier isn't in scope before `assert.ok` even runs.
Bail out (return null) in that case rather than try to escape, and
drop the now-obsolete comment about backslashes — the broader
literal check supersedes it. Includes regression tests for `==`,
`>`, and the escaped `\${...}` variant.
Follow-up to #8537.
3 participants
What does this PR do?
Adds a custom ESLint rule,
eslint-rules/eslint-require-boolean-assert-message, that requires a descriptivemessageargument on boolean test assertions —assert(value)andassert.ok(value)— whose first argument is a non-trivial, boolean-reducing expression. The rule is enabled aterrorseverity under the existingdd-trace/tests/allconfig block, so it applies across theTEST_FILESglob (packages/*/test/**,integration-tests/**, and any*.spec.js).The PR also applies the rule to the entire existing test suite — 716 violations across 232 files were fixed in this branch (mostly via an AST-based codemod that emitted concise, side-effect-safe messages using
util.inspect, with a small set of edge cases extracted into named variables by hand).Motivation
Boolean assertions like
assert.ok(duration >= 1000)fail withExpected true, got false— which tells you nothing about the actual value ofduration. When that assertion is in a flaky test, you're left guessing whetherdurationwas 500ms or 5ms, making the failure dramatically harder to debug than it needs to be. Forcing a second-argument message nudges authors to surface the runtime value (e.g.assert.ok(duration >= 1000, `Expected ${duration} >= 1000`)), which is what you almost always need when triaging a CI failure.What the rule flags
An expression is flagged (and so requires a message) when it boolean-reduces, hiding the operand values behind a plain
true/false:<,<=,>,>=,===,!==,==,!=&&,||,??in/instanceof— only when an operand isn't a simple reference; with simple operands the source line already fully describes the questionarr.includes(x),arr.some(cb),arr.every(cb),obj.hasOwnProperty(k),Object.hasOwn(obj, k),Array.isArray(x),Buffer.isBuffer(x),Number.isNaN(x)/isFinite/isInteger/isSafeInteger,Object.isFrozen(x)/isSealed/isExtensible,buf.equals(other)newexpressions and other shapes whose value isn't meaningful on its ownString-matching predicates (
startsWith/endsWith/String#match/RegExp#test) are intentionally not in this list — the siblingeslint-prefer-assert-matchrule handles those with the more specificassert.match/assert.doesNotMatchsuggestion and an autofixer.What's allowed without a message
The rule deliberately allows assertions whose source line is already self-describing, so it doesn't get in the way of idiomatic code. Node's
AssertionErrorprints both the source line and the actual runtime value of the asserted expression, so the following stay informative on failure without an explicit message:isReady,obj.prop.sub,arr[0],arr[i],map[`key-${id}`],obj?.prop?.subgetResult(),arr.find(cb),predicate(x),items.map(transform). Arguments are intentionally not inspected: a complex argument can't make a value-returning call any less informative on failurespan.context(),span.context()._tags,arr.entries()!x,!!x,typeof x,void x,delete obj.kin/instanceofwith trivial operands —'foo' in carrier,err instanceof Error,!('x-datadog-trace-id' in carrier)Autofixer
For value comparisons whose operands are side-effect-free, the rule autofixes by appending a message that interpolates the operands:
assert.ok(x > 5)becomesassert.ok(x > 5, `Expected ${x} > 5`). The set of autofixed operators is<,<=,>,>=,==,!=.===and!==are flagged but not autofixed: the better migration is toassert.strictEqual/assert.notStrictEqual(whichno-restricted-syntaxin the eslint config already nudges users toward), so a mechanical message wrap here would compete with that.The autofixer also correctly handles parenthesized first arguments — without that,
assert.ok((x >= 1))would be rewritten toassert.ok((x >= 1, `Expected ${x} >= 1`))(a comma-sequence expression that makes the assertion a no-op). It now walks past balanced surrounding)tokens before inserting, with regression tests covering the pattern.Scope of the cleanup
Applying the rule across the test suite produced 716 errors across 232 files. The fixes break down into a few families:
Array.isArray(x)/Object.hasOwn(o, k)/Number.isFinite(n)/arr.includes(v)/ etc. — message added that interpolates the relevant operand or, forObject.hasOwn, the object's own keysx > 0,length >= 1, …) — autofixed by the rule itselfconstabove the assertion and referenced twice (once in the check, once in the message), so the message can't re-evaluate something side-effectfulassert.notStrictEqual(a, b)(when the original wasassert.ok(a !== b)) per the existingno-restricted-syntaxnudgeMost fixes use
util.inspectto render values; the import was added to ~200 test files (or extended onto an existingnode:utildestructure where one already existed), grouped with the file's other builtin imports.