fix(code-quality): close the false negatives found adopting 0.4.0 - #61
Merged
Merged
Conversation
…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
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-castfollows 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-assertionreads a message held in a variable: an identifier whose variable is initialised with a string, a template, a regex orRegExp(...)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-expectno longer treats acontainer/baseElementfrom any call as a render root. A root from arender*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), soconst { container } = await docker.inspect(id); expect(container).not.toBeNull()is accepted. This one is a false positive, so its new cases arevalidones.no-sleep-in-unit-testsno longer exempts a whole file because one test fakes its timers. AfakeTimerMethodscall covers the test it is in, else thedescribeit 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:
no-sleep-in-unit-teststhrewCannot read properties of null (reading 'type')on asetTimeout(...)call at the top level of a unit test file. Reproduced againstmainon ESLint 10 and 9, fixed in its own commit.runnerNameandisSelfOrAncestormoved intoutils/astinstead of gaining another copy.no-conditional-expectandno-swallowed-assertionstill carry their own identicalrunnerName; 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 asetup()result is a DOM node.toBeTruthy/toBeDefinedon the same binding still report, assoleWeakExpectinstead ofsoleRenderRootExpect.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
.changeset/(bun run changeset), written for a consumer reading the changelog, that says whether a project spreadingrecommendedwill see new errors. Skip only if no published package changed. (patch. A project spreadingrecommendedsees no new errors: the only rule of the four that is inrecommended,no-vacuous-expect, reports less. The three opt-in rules can report more.)bun run build && bun run typecheck && bun run testis green locally (testruns on ESLint 10 and then 9). (code-quality: 449 tests on each leg, up from 393.bun run docs:buildis green too.)ts/tsxfence indocs/rules/<rule>.mdis labelledbad,goodorprose, and I ran them (bunx vitest run tests/docs/plugins.test.ts -t <rule>inpackages/eslint-test-utils).EXPECTED_RULE_COUNTinsite/scripts/parity.test.tsis updated,bun run docs:readmeshas regenerated the README table and doc header, and the preset either lists the rule aterroror names it inOMITTED_FROM_PRESETSwith a reason. (Not applicable: no rule added or removed, no description or preset change.)