Skip to content

fix(code-quality): close the false negatives found adopting 0.4.0 - #61

Merged
Shironex merged 7 commits into
mainfrom
fix/code-quality-false-negatives
Oct 1, 2026
Merged

Shironex merged 7 commits into
mainfrom
fix/code-quality-false-negatives

Conversation

@Shironex

@Shironex Shironex commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

What

Closes #60. Each case from the issue went in as a RuleTester case first and was red before its fix.

  • typed-mock-over-double-cast follows a mock created first and put in the object by name: const fn = vi.fn(); const svc = { fn } as unknown as Service;, also { run: fn }, nested, through a chain, and when the name is assigned its mock later (let get; beforeEach(() => { get = jest.fn(); })). Left alone: a mock that arrives through a spread, an import, a parameter or a helper's return value.
  • no-message-only-throw-assertion reads a message held in a variable: an identifier whose variable is initialised with a string, a template, a regex or RegExp(...) and never assigned again is a message check, and no longer counts as a class pin. Left alone: an import, a parameter, a call result, another variable, a reassigned binding.
  • no-vacuous-expect no longer treats a container / baseElement from any call as a render root. A root from a render* call counts for every presence check, as before. A root from another call counts only when the assertion is DOM-specific (container.firstChild, toBeInTheDocument, toBeVisible, not.toBeEmptyDOMElement), so const { container } = await docker.inspect(id); expect(container).not.toBeNull() is accepted. This one is a false positive, so its new cases are valid ones.
  • no-sleep-in-unit-tests no longer exempts a whole file because one test fakes its timers. A fakeTimerMethods call covers the test it is in, else the describe it is in, else the whole file. A sleep helper defined in the file is judged by its callers.

Also in here, found on the way:

  • Crash fix: no-sleep-in-unit-tests threw Cannot read properties of null (reading 'type') on a setTimeout(...) call at the top level of a unit test file. Reproduced against main on ESLint 10 and 9, fixed in its own commit.
  • Refactor: runnerName and isSelfOrAncestor moved into utils/ast instead of gaining another copy. no-conditional-expect and no-swallowed-assertion still carry their own identical runnerName; they are not touched here.

Trade-offs worth a look in review

  • no-vacuous-expect: const { container } = setup(); expect(container).not.toBeNull() is no longer reported, because nothing in the syntax shows that a setup() result is a DOM node. toBeTruthy / toBeDefined on the same binding still report, as soleWeakExpect instead of soleRenderRootExpect.
  • no-sleep-in-unit-tests: only helpers bound to a name (function sleep, const sleep = ...) are followed to their callers. A sleep in an object method, a class method or a function passed to a wrapper (vi.fn((ms) => new Promise(...))) is judged by where it is written, so one defined outside the suite that fakes the timers is now reported. Timers installed at the top level, in a top-level hook or by a top-level helper keep the old file-wide exemption.
  • typed-mock-over-double-cast: a binding counts as a mock if any write to it is one, and only property values are followed ({ items: [fn] } and { get: () => fn } stay silent).

Each commit is one rule, so any of the three can be dropped on its own.

Checklist

  • A changeset in .changeset/ (bun run changeset), written for a consumer reading the changelog, that says whether a project spreading recommended will see new errors. Skip only if no published package changed. (patch. A project spreading recommended sees no new errors: the only rule of the four that is in recommended, no-vacuous-expect, reports less. The three opt-in rules can report more.)
  • Tests: bun run build && bun run typecheck && bun run test is green locally (test runs on ESLint 10 and then 9). (code-quality: 449 tests on each leg, up from 393. bun run docs:build is green too.)
  • Rule doc examples: every ts/tsx fence in docs/rules/<rule>.md is labelled bad, good or prose, and I ran them (bunx vitest run tests/docs/plugins.test.ts -t <rule> in packages/eslint-test-utils).
  • If a rule was added or removed: EXPECTED_RULE_COUNT in site/scripts/parity.test.ts is updated, bun run docs:readmes has regenerated the README table and doc header, and the preset either lists the rule at error or names it in OMITTED_FROM_PRESETS with a reason. (Not applicable: no rule added or removed, no description or preset change.)

…l timer

A `setTimeout(...)` call at the top level of a unit test file made the
rule throw "Cannot read properties of null (reading 'type')". The walk
to the enclosing function ran past `Program`, whose `parent` is null at
runtime. The walk now stops there.
`runnerName` and `isSelfOrAncestor` move from no-vacuous-expect and
no-message-only-throw-assertion into utils/ast, so the next rule that
needs them imports them instead of carrying another copy. No behaviour
change.
…r-double-cast

The rule only saw a mock-factory call written inside the double-cast
object, so `const fn = vi.fn(); const svc = { fn } as unknown as Service`
passed. A property value that names a variable initialised with, or
assigned, a mock call (or a chain rooted at one) now counts as a mock,
shorthand or not and at any depth. Property keys, member names and
destructured names are not followed.

Refs #60
…ly-throw-assertion

An identifier argument was always taken for an error class, so
`const expected = /no access/; expect(run).toThrow(expected)` passed and
even counted as the class pin for a later message check. An identifier
whose variable is initialised with a string, a template, a regex or a
`RegExp(...)` and never assigned again is now a message check. An import,
a parameter, a call result and a reassigned binding are still read as a
class.

Refs #60
…render call

no-vacuous-expect treated a `container` / `baseElement` that came from
any call as a render root, so `const { container } = await
docker.inspect(id); expect(container).not.toBeNull()` was reported as a
render smoke test. A root from a `render*` call still counts for every
presence check. A root from another call counts only when the assertion
is DOM-specific: it reads a DOM member off the root, or the matcher
exists only for DOM nodes.

Refs #60
…-tests

One `useFakeTimers()` call anywhere exempted every timer promise in the
file, including a real sleep in a test on real timers. A fake-timer call
now covers the test it is in, else the suite it is in, else the whole
file, so top-level installs behave as before. A sleep helper defined in
the file is judged by its callers: silent when every call to it runs
under fake timers, reported where it sleeps otherwise.

Refs #60
@Shironex Shironex added false positive A rule reports code it should accept false negative A rule stays silent on code it should report rule change A change to what an existing rule flags, its options or its preset severity crash A rule throws, or a plugin fails to load, resolve or install pkg: code-quality @noctcore/eslint-plugin-code-quality labels Oct 1, 2026
@Shironex
Shironex merged commit d45dc31 into main Oct 1, 2026
8 checks passed
@Shironex Shironex self-assigned this Oct 1, 2026
@github-actions github-actions Bot mentioned this pull request Oct 1, 2026
@Shironex
Shironex deleted the fix/code-quality-false-negatives branch October 3, 2026 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crash A rule throws, or a plugin fails to load, resolve or install false negative A rule stays silent on code it should report false positive A rule reports code it should accept pkg: code-quality @noctcore/eslint-plugin-code-quality rule change A change to what an existing rule flags, its options or its preset severity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(code-quality): false negatives found adopting 0.4.0

1 participant