Skip to content

Commit b8a6087

Browse files
committed
chore(eslint): require messages on boolean test assertions
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.
1 parent 54f0666 commit b8a6087

3 files changed

Lines changed: 331 additions & 0 deletions

File tree

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
// Boolean assertions like `assert(value)` and `assert.ok(value)` produce a useless failure message
2+
// ("Expected true, got false") when no second-argument message is provided. That makes debugging
3+
// flaky tests painful: you know the expression was falsy, but not what value it actually had.
4+
//
5+
// This rule requires a message argument whenever the first argument is "non-trivial" — i.e. an
6+
// expression whose failure mode would not, on its own, tell you what was being asserted on.
7+
// Trivial (allowed without a message) shapes are those whose source line is fully self-describing:
8+
//
9+
// - Literals and references: `true`, `'foo'`, `isReady`, `obj.prop.sub`, `arr[0]`, `obj?.prop`
10+
// - Logical/structural unary ops on a trivial operand: `!x`, `!!x`, `typeof x`, `delete obj.k`
11+
// - `in` / `instanceof` with trivial operands: `'foo' in carrier`, `err instanceof Error`
12+
// - Calls where the callee and all arguments are trivial: `arr.includes('foo')`,
13+
// `Array.isArray(x)`, `carrier.hasOwnProperty('x-datadog-trace-id')`
14+
// - Zero-argument method calls in a chain (callee is a MemberExpression): `span.context()`,
15+
// `arr.entries()`. These act as getter-style navigation and are treated as part of a path.
16+
//
17+
// Non-trivial shapes (require a message) include value comparisons (`x > 5`, `x === 'foo'`),
18+
// logical combinations (`x && y`), dynamic indexing (`arr[i]`), zero-argument plain function
19+
// calls (`getResult()`), and anything whose actual value would be useful for debugging the
20+
// failure.
21+
22+
/** @typedef {import('estree').Node} Node */
23+
/** @typedef {import('estree').CallExpression} CallExpression */
24+
25+
const ASSERT_CALL_NAMES = ['assert', 'assert.ok']
26+
27+
// Unary operators that don't hide a "value of interest": they just transform a reference into a
28+
// boolean/type/undefined. `+`, `-`, `~` are excluded — those hide numeric value differences.
29+
const TRIVIAL_UNARY_OPERATORS = new Set(['!', 'typeof', 'void', 'delete'])
30+
31+
// Binary operators that ask a structural yes/no question with both operands inspectable from
32+
// source. Excludes value comparisons (`===`, `==`, `<`, `>`, etc.) which hide the actual value.
33+
const TRIVIAL_BINARY_OPERATORS = new Set(['in', 'instanceof'])
34+
35+
export default {
36+
meta: {
37+
type: 'problem',
38+
docs: {
39+
description:
40+
'Require a message argument on boolean assertions (`assert(value)` / `assert.ok(value)`) ' +
41+
'whose first argument is a non-trivial expression, so failure messages reveal what was asserted.',
42+
recommended: true,
43+
},
44+
schema: [],
45+
messages: {
46+
missingMessage:
47+
'`{{name}}(...)` with a non-trivial first argument should pass a descriptive message as the ' +
48+
'second argument. Without it, failures only report "Expected true, got false" without any ' +
49+
'context about the actual value (e.g. `{{name}}({{expr}}, `expected …, got ${actual}`)`).',
50+
},
51+
},
52+
53+
create (context) {
54+
return {
55+
CallExpression (node) {
56+
const calleeName = getMatchedAssertName(node.callee)
57+
if (calleeName === undefined) return
58+
59+
if (node.arguments.length === 0) return
60+
61+
const firstArg = node.arguments[0]
62+
63+
if (firstArg.type === 'SpreadElement') return
64+
65+
if (node.arguments.length >= 2) return
66+
67+
if (isTrivialExpression(firstArg)) return
68+
69+
const sourceCode = context.getSourceCode()
70+
71+
context.report({
72+
node,
73+
messageId: 'missingMessage',
74+
data: {
75+
name: calleeName,
76+
expr: sourceCode.getText(firstArg),
77+
},
78+
})
79+
},
80+
}
81+
},
82+
}
83+
84+
/**
85+
* @param {Node} callee
86+
* @returns {string | undefined}
87+
*/
88+
function getMatchedAssertName (callee) {
89+
for (const name of ASSERT_CALL_NAMES) {
90+
const parts = name.split('.')
91+
92+
if (parts.length === 1) {
93+
if (callee.type === 'Identifier' && callee.name === parts[0]) {
94+
return name
95+
}
96+
} else if (
97+
callee.type === 'MemberExpression' &&
98+
!callee.computed &&
99+
!callee.optional &&
100+
callee.object.type === 'Identifier' &&
101+
callee.object.name === parts[0] &&
102+
callee.property.type === 'Identifier' &&
103+
callee.property.name === parts[1]
104+
) {
105+
return name
106+
}
107+
}
108+
109+
return undefined
110+
}
111+
112+
/**
113+
* A "trivial" expression is one whose source text already describes what is being asserted on,
114+
* so a failure of "Expected true, got false" is informative enough on its own. See the file
115+
* header for the full taxonomy.
116+
*
117+
* @param {Node} node
118+
* @returns {boolean}
119+
*/
120+
function isTrivialExpression (node) {
121+
if (node.type === 'ChainExpression') {
122+
return isTrivialExpression(node.expression)
123+
}
124+
125+
if (
126+
node.type === 'Literal' ||
127+
node.type === 'Identifier' ||
128+
node.type === 'ThisExpression' ||
129+
node.type === 'Super'
130+
) {
131+
return true
132+
}
133+
134+
if (node.type === 'MemberExpression') {
135+
if (node.computed) {
136+
if (node.property.type !== 'Literal') return false
137+
} else if (node.property.type !== 'Identifier') {
138+
return false
139+
}
140+
141+
return isTrivialExpression(node.object)
142+
}
143+
144+
if (node.type === 'UnaryExpression') {
145+
return TRIVIAL_UNARY_OPERATORS.has(node.operator) && isTrivialExpression(node.argument)
146+
}
147+
148+
if (node.type === 'BinaryExpression') {
149+
return TRIVIAL_BINARY_OPERATORS.has(node.operator) &&
150+
isTrivialExpression(node.left) &&
151+
isTrivialExpression(node.right)
152+
}
153+
154+
if (node.type === 'CallExpression') {
155+
// A zero-arg plain function call (`getResult()`) is opaque — there's nothing in the source
156+
// line that hints at the returned value. But a zero-arg call whose callee is a member access
157+
// (`span.context()`, `arr.entries()`) typically acts as getter-style navigation in a chain
158+
// and is treated as part of the path.
159+
if (node.arguments.length === 0) {
160+
return getCalleeBody(node.callee).type === 'MemberExpression' && isTrivialExpression(node.callee)
161+
}
162+
163+
for (const arg of node.arguments) {
164+
if (arg.type === 'SpreadElement') return false
165+
if (!isTrivialExpression(arg)) return false
166+
}
167+
168+
return isTrivialExpression(node.callee)
169+
}
170+
171+
return false
172+
}
173+
174+
/**
175+
* Strips a leading optional-chain wrapper to expose the underlying callee, so `foo?.bar` is seen
176+
* as a MemberExpression (not a ChainExpression) when we're classifying a `CallExpression`.
177+
*
178+
* @param {Node} callee
179+
* @returns {Node}
180+
*/
181+
function getCalleeBody (callee) {
182+
return callee.type === 'ChainExpression' ? callee.expression : callee
183+
}
Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
import { RuleTester } from 'eslint'
2+
import rule from './eslint-require-boolean-assert-message.mjs'
3+
4+
const ruleTester = new RuleTester({
5+
languageOptions: { ecmaVersion: 2022 },
6+
})
7+
8+
ruleTester.run('eslint-require-boolean-assert-message', /** @type {import('eslint').Rule.RuleModule} */ (rule), {
9+
valid: [
10+
// Bare references and literal-like first args are self-describing.
11+
'assert(isReady)',
12+
'assert.ok(isReady)',
13+
'assert(this)',
14+
'assert.ok(true)',
15+
'assert.ok(obj.prop)',
16+
'assert.ok(a.b.c.d)',
17+
'assert.ok(arr[0])',
18+
"assert.ok(obj['key'])",
19+
'assert.ok(obj?.prop)',
20+
'assert.ok(obj?.prop?.sub)',
21+
22+
// Structural unary ops on a trivial operand.
23+
'assert.ok(!isReady)',
24+
'assert.ok(!!isReady)',
25+
'assert(!obj.prop)',
26+
'assert.ok(typeof x)',
27+
'assert.ok(delete obj.foo)',
28+
29+
// `in` and `instanceof` with trivial operands — the source line fully describes the question.
30+
"assert.ok('foo' in carrier)",
31+
"assert.ok(!('x-datadog-trace-id' in carrier))",
32+
"assert.ok(!('x-b3-traceid' in carrier))",
33+
'assert.ok(err instanceof Error)',
34+
'assert.ok(!(err instanceof TypeError))',
35+
36+
// Calls (≥1 arg) where callee and all args are trivial.
37+
"assert.ok(arr.includes('foo'))",
38+
"assert.ok(!arr.includes('foo'))",
39+
'assert.ok(Array.isArray(x))',
40+
"assert.ok(carrier.hasOwnProperty('x-datadog-trace-id'))",
41+
'assert.ok(predicate(x))',
42+
43+
// Zero-arg method calls in a chain (getter-style navigation).
44+
'assert.ok(span.context())',
45+
'assert.ok(span.context()._tags)',
46+
"assert.ok(!(APM_TRACING_ENABLED_KEY in span.context()._tags))",
47+
'assert.ok(Object.hasOwn(span.context()._tags, APM_TRACING_ENABLED_KEY))',
48+
'assert.ok(arr.entries())',
49+
50+
// Non-trivial first argument with a message is fine.
51+
'assert(x > 5, `duration was ${x}`)',
52+
"assert.ok(x > 5, 'expected x > 5')",
53+
"assert.ok(x === 'foo', 'x should be foo')",
54+
"assert.ok(x && y, 'both should be truthy')",
55+
'assert.ok(arr[i], `arr[${i}] should be set`)',
56+
"assert.ok(getResult(), 'getResult should return truthy')",
57+
58+
// Calls we don't target.
59+
'assert.strictEqual(x, 5)',
60+
'assert.deepStrictEqual(x, { foo: 1 })',
61+
'assert.match(text, /foo/)',
62+
"assert.fail('nope')",
63+
'foo.assert(x > 5)',
64+
'somethingElse(x > 5)',
65+
66+
// Spread first argument is opaque to us; don't flag.
67+
'assert(...args)',
68+
'assert.ok(...args)',
69+
],
70+
invalid: [
71+
// Value comparisons hide the actual value.
72+
{
73+
code: 'assert.ok(duration >= 1000)',
74+
errors: [{ messageId: 'missingMessage' }],
75+
},
76+
{
77+
code: 'assert.ok(duration < 1050)',
78+
errors: [{ messageId: 'missingMessage' }],
79+
},
80+
{
81+
code: "assert(x === 'foo')",
82+
errors: [{ messageId: 'missingMessage' }],
83+
},
84+
{
85+
code: 'assert.ok(x !== y)',
86+
errors: [{ messageId: 'missingMessage' }],
87+
},
88+
89+
// Logical combinations.
90+
{
91+
code: 'assert.ok(x && y)',
92+
errors: [{ messageId: 'missingMessage' }],
93+
},
94+
{
95+
code: 'assert.ok(a || b)',
96+
errors: [{ messageId: 'missingMessage' }],
97+
},
98+
99+
// Zero-arg plain function calls (Identifier callee) are still opaque.
100+
{
101+
code: 'assert.ok(getResult())',
102+
errors: [{ messageId: 'missingMessage' }],
103+
},
104+
105+
// Call with a non-trivial argument (computed indexing with dynamic key).
106+
{
107+
code: "assert.ok(arr.includes(items[i]))",
108+
errors: [{ messageId: 'missingMessage' }],
109+
},
110+
111+
// Computed member access with dynamic key.
112+
{
113+
code: 'assert.ok(arr[i])',
114+
errors: [{ messageId: 'missingMessage' }],
115+
},
116+
{
117+
code: 'assert.ok(map[`key-${id}`])',
118+
errors: [{ messageId: 'missingMessage' }],
119+
},
120+
121+
// `in` / `instanceof` with a non-trivial operand.
122+
{
123+
code: 'assert.ok(getKey() in carrier)',
124+
errors: [{ messageId: 'missingMessage' }],
125+
},
126+
{
127+
code: 'assert.ok(make() instanceof Error)',
128+
errors: [{ messageId: 'missingMessage' }],
129+
},
130+
131+
// Unary op on a non-trivial argument (call result).
132+
{
133+
code: 'assert.ok(!getResult())',
134+
errors: [{ messageId: 'missingMessage' }],
135+
},
136+
137+
// NewExpression is non-trivial.
138+
{
139+
code: 'assert.ok(new Foo())',
140+
errors: [{ messageId: 'missingMessage' }],
141+
},
142+
],
143+
})
144+
145+
console.log('eslint-require-boolean-assert-message: all tests passed')

eslint.config.mjs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import eslintEnvAliases from './eslint-rules/eslint-env-aliases.mjs'
2020
import eslintLogPrintfStyle from './eslint-rules/eslint-log-printf-style.mjs'
2121
import eslintNonPrefixEnvNames from './eslint-rules/eslint-non-prefix-env-names.mjs'
2222
import eslintProcessEnv from './eslint-rules/eslint-process-env.mjs'
23+
import eslintRequireBooleanAssertMessage from './eslint-rules/eslint-require-boolean-assert-message.mjs'
2324
import eslintRequireExportExists from './eslint-rules/eslint-require-export-exists.mjs'
2425
import eslintSafeTypeOfObject from './eslint-rules/eslint-safe-typeof-object.mjs'
2526
import eslintTimerUnref from './eslint-rules/eslint-timer-unref.mjs'
@@ -383,6 +384,7 @@ export default [
383384
'eslint-non-prefix-env-names': eslintNonPrefixEnvNames,
384385
'eslint-safe-typeof-object': eslintSafeTypeOfObject,
385386
'eslint-log-printf-style': eslintLogPrintfStyle,
387+
'eslint-require-boolean-assert-message': eslintRequireBooleanAssertMessage,
386388
'eslint-require-export-exists': eslintRequireExportExists,
387389
'eslint-timer-unref': eslintTimerUnref,
388390
},
@@ -734,6 +736,7 @@ export default [
734736
n: eslintPluginN,
735737
},
736738
rules: {
739+
'eslint-rules/eslint-require-boolean-assert-message': 'error',
737740
'mocha/consistent-spacing-between-blocks': 'off',
738741
'mocha/max-top-level-suites': ['error', { limit: 1 }],
739742
'mocha/no-mocha-arrows': 'off',

0 commit comments

Comments
 (0)