diff --git a/.changeset/code-quality-false-negatives.md b/.changeset/code-quality-false-negatives.md new file mode 100644 index 0000000..8b1cc37 --- /dev/null +++ b/.changeset/code-quality-false-negatives.md @@ -0,0 +1,19 @@ +--- +'@noctcore/eslint-plugin-code-quality': patch +--- + +Close four gaps in the test-discipline rules and fix a crash. + +**A project that spreads `recommended` as-is sees no new errors.** The only rule here that is in `recommended`, `no-vacuous-expect`, reports less than before. The other three rules are opt-in, and a project that enabled them can see new errors: + +- `typed-mock-over-double-cast` now sees a mock that was created first and put in the object by name: `const fn = vi.fn(); const svc = { fn } as unknown as Service;`, also as `{ run: fn }`, nested in an inner object, through a chain (`jest.fn().mockResolvedValue(...)`) and when the name is assigned its mock later (`let get; beforeEach(() => { get = jest.fn(); })`). A mock that arrives through a spread, an import, a parameter or a helper's return value is still not followed. +- `no-message-only-throw-assertion` now reads a message held in a variable: `const expected = /no access/; expect(run).toThrow(expected)` is a message check, for a string, a template, a regex literal or `new RegExp(...)`, as long as the variable is initialised where it is declared and never assigned again. Such a variable no longer counts as a class pin for a later message check on the same subject either. An import, a parameter and a call result are still read as an error class. +- `no-sleep-in-unit-tests` no longer exempts a whole file because one test fakes its timers. A `fakeTimerMethods` call now covers the test it is in, else the `describe` it is in (directly or in a `beforeEach` / `beforeAll`), else the whole file, so a file that installs fake timers at the top level behaves as before. A real sleep in a sibling test or suite is reported. A sleep helper defined in the file is judged by its callers: it stays silent when every call to it runs under fake timers. + +Reports less: + +- `no-vacuous-expect` no longer treats a `container` / `baseElement` from any call as a render root. A root from a `render*` call still counts for every presence check. A root from another call (`setup()`, `docker.inspect(id)`) counts only when the assertion is DOM-specific: it reads `firstChild`, `innerHTML` and the like off the root, or uses `toBeInTheDocument`, `toBeVisible` or `not.toBeEmptyDOMElement`. So `const { container } = await docker.inspect(id); expect(container).not.toBeNull();` is accepted, and so is the same check on the root of a render helper whose name does not start with `render`. A weak matcher there (`toBeTruthy`, `toBeDefined`) is still reported, as `soleWeakExpect`. + +Crash fixed: + +- `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. diff --git a/packages/eslint-plugin-code-quality/docs/rules/no-message-only-throw-assertion.md b/packages/eslint-plugin-code-quality/docs/rules/no-message-only-throw-assertion.md index 1225f63..d3faeaf 100644 --- a/packages/eslint-plugin-code-quality/docs/rules/no-message-only-throw-assertion.md +++ b/packages/eslint-plugin-code-quality/docs/rules/no-message-only-throw-assertion.md @@ -49,14 +49,24 @@ negated and whose argument: snapshot of the error (`toThrowErrorMatchingSnapshot()`, `toThrowErrorMatchingInlineSnapshot()`) records only its message, so it is a message check too, whatever its argument. +A message held in a variable is the same check: an identifier argument bound to one of those values +(`const expected = /no access/`, also with `as const`) is read through its declaration, as long as +the variable is initialised where it is declared and never assigned again. + It reports inside and outside test callbacks, so a shared helper that asserts `.rejects.toThrow()` is reported where it is written. -```ts bad filename=src/lib/api-path.test.ts reports=3 +```ts bad filename=src/lib/api-path.test.ts reports=4 +const REFUSAL = /non-relative api path/; + it('rejects an absolute url', () => { expect(() => toApiPath('https://evil.test')).toThrow('Refusing a non-relative api path'); }); +it('rejects a protocol-relative url', () => { + expect(() => toApiPath('//evil.test')).toThrow(REFUSAL); +}); + it('rejects an unknown queue', () => { expect(() => metrics.onModuleInit()).toThrow(new RegExp(`Queue "${missing}" is declared`)); }); @@ -67,10 +77,17 @@ export async function expectRefusal(promise: Promise) { ``` ```ts good filename=src/lib/api-path.test.ts +const REFUSAL = /non-relative api path/; + it('rejects an absolute url', () => { expect(() => toApiPath('https://evil.test')).toThrow(UnsafePathError); }); +it('rejects a protocol-relative url', () => { + expect(() => toApiPath('//evil.test')).toThrow(UnsafePathError); + expect(() => toApiPath('//evil.test')).toThrow(REFUSAL); +}); + it('rejects an unknown queue', () => { expect(() => metrics.onModuleInit()).toThrow( expect.objectContaining({ name: 'QueueConfigError', queue: missing }), @@ -134,8 +151,10 @@ it('refuses', async () => { Vitest compares it like `toEqual`, so its class name takes part. See Options for Jest. - `.not.toThrow()` and `.resolves.not.toThrow()`: a negated throw assertion has no class to pin. - `.rejects.toMatchObject(...)` and the other value matchers on their own. -- A message held in a variable (`toThrow(expectedMessage)`): the rule reads syntax, not values, and - treats it as it would an error class. +- A message the rule cannot read from the variable's own declaration: one that arrives through an + import, a parameter, a call result, another variable (`const expected = MESSAGE`) or a binding + that is assigned again. The rule reads syntax, not values, so it treats such an identifier as it + would an error class. ## Options diff --git a/packages/eslint-plugin-code-quality/docs/rules/no-sleep-in-unit-tests.md b/packages/eslint-plugin-code-quality/docs/rules/no-sleep-in-unit-tests.md index d081356..89b0e91 100644 --- a/packages/eslint-plugin-code-quality/docs/rules/no-sleep-in-unit-tests.md +++ b/packages/eslint-plugin-code-quality/docs/rules/no-sleep-in-unit-tests.md @@ -31,7 +31,7 @@ it('announces the new route', async () => { ## What it flags In a unit test file (a path ending in one of `testFileSuffixes`, and not containing any -`integrationMarkers`) that never installs fake timers: +`integrationMarkers`), outside the tests and suites that install fake timers: - a `new Promise` whose executor calls `setTimeout` (or `globalThis.setTimeout`) with a callback that resolves it: `setTimeout(resolve, n)` or `setTimeout(() => resolve(value), n)`, in an @@ -65,6 +65,54 @@ it('stops the child', async () => { }); ``` +### Fake timers cover a test or a suite, not the file + +A `fakeTimerMethods` call (`vi.useFakeTimers()`, `jest.useFakeTimers()`) covers the test it is +written in. Written in a suite, directly or in a `beforeEach` / `beforeAll` hook, it covers that +`describe` and the suites nested in it. Written outside any test or suite (at the top level, in a +top-level hook, or in a top-level helper such as `installClock()`), it covers the whole file. A timer +promise outside every covered test and suite runs on the real clock and is reported. + +A sleep helper defined in the file (`const sleep = (ms) => new Promise(...)`, `function sleep`) is +judged by its callers. It is silent when every call to it in the file is covered, also through +another helper, and reported where it sleeps when a test on real timers calls it or nothing in the +file does. + +```ts bad filename=src/lib/poller.test.ts +it('polls every second', async () => { + vi.useFakeTimers(); + const poller = start(); + await vi.advanceTimersByTimeAsync(1_000); + expect(poller.ticks).toBe(1); + vi.useRealTimers(); +}); + +it('stops', async () => { + const poller = start(); + poller.stop(); + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(poller.ticks).toBe(0); +}); +``` + +```ts good filename=src/lib/poller.test.ts +beforeEach(() => vi.useFakeTimers()); +afterEach(() => vi.useRealTimers()); + +it('polls every second', async () => { + const poller = start(); + await vi.advanceTimersByTimeAsync(1_000); + expect(poller.ticks).toBe(1); +}); + +it('stops', async () => { + const poller = start(); + poller.stop(); + await vi.advanceTimersByTimeAsync(100); + expect(poller.ticks).toBe(0); +}); +``` + ## What it does not flag - A zero or omitted delay while `allowZeroDelay` is on: `setTimeout(resolve, 0)` yields one @@ -78,9 +126,11 @@ it('stops the child', async () => { - `setImmediate`, `process.nextTick` and `queueMicrotask` inside a promise. - A `setTimeout` that is not a promise executor's resolve, such as a timer the code under test schedules. -- Any timer promise in a file that calls a `fakeTimerMethods` method anywhere (`vi.useFakeTimers()`, - `jest.useFakeTimers()`, in a `beforeEach` or a test). Under fake timers the wait is virtual and the - test drives it with `advanceTimersByTimeAsync`, so it costs no wall-clock time. +- A timer promise in a test or a suite that installs fake timers with a `fakeTimerMethods` method + (`vi.useFakeTimers()`, `jest.useFakeTimers()`), and any timer promise in a file that installs them + at the top level or in a top-level `beforeEach`. Under fake timers the wait is virtual and the + test drives it with `advanceTimersByTimeAsync`, so it costs no wall-clock time. A sleep helper + defined in the file is covered when every call to it is. - Files that are not unit tests, or whose path contains an `integrationMarkers` entry. - A sleep helper imported from another module (`import { sleep } from './test-utils'`): the rule reads one file at a time, so it reports the helper where it is defined, if that file is a unit @@ -113,8 +163,11 @@ it('reads a slow body', async () => { }); ``` -The file-level check is coarse: one `useFakeTimers()` call anywhere exempts every timer promise in -the file, including one in a test that runs on real timers. +Inside a covered test or suite the check is still coarse: the order of the calls is not read, so a +sleep after `useRealTimers()` in the same test or suite stays exempt. Only helpers bound to a name +(`function sleep`, `const sleep = ...`) are followed to their callers; a sleep in an object method +or in a function passed to a wrapper is judged by where it is written. Fake timers installed from +another module or a setup file are not seen, so a timer promise that relies on them is reported. ## Options @@ -123,7 +176,7 @@ the file, including one in a test that runs on real timers. | `testFileSuffixes` | `string[]` | `.test` / `.spec` with `.ts`, `.tsx`, `.js`, `.jsx` | A file is a unit test when its path ends with one of these. | | `integrationMarkers` | `string[]` | `.integration.test.`, `.integration.spec.`, `.e2e.test.`, `.e2e.spec.`, `.e2e-spec.`, `/integration/`, `/e2e/` | A test file whose path, relative to the ESLint working directory, contains one of these is skipped. | | `allowZeroDelay` | `boolean` | `true` | Accept a zero or omitted delay. | -| `fakeTimerMethods` | `string[]` | `["useFakeTimers"]` | Methods (on any receiver) that install fake timers. A file that calls one is not checked. | +| `fakeTimerMethods` | `string[]` | `["useFakeTimers"]` | Methods (on any receiver) that install fake timers. A call covers the test it is in, else the suite it is in, else the whole file; timer promises there are not checked. | ```js 'noctcore-code-quality/no-sleep-in-unit-tests': ['error', { diff --git a/packages/eslint-plugin-code-quality/docs/rules/no-vacuous-expect.md b/packages/eslint-plugin-code-quality/docs/rules/no-vacuous-expect.md index a194478..f231ad5 100644 --- a/packages/eslint-plugin-code-quality/docs/rules/no-vacuous-expect.md +++ b/packages/eslint-plugin-code-quality/docs/rules/no-vacuous-expect.md @@ -74,7 +74,17 @@ matcher is a presence check: `toBeInTheDocument`, `toBeTruthy`, `toBeDefined`, ` `not.toBeNull`, `not.toBeUndefined`, `not.toBeFalsy`, `not.toBeEmptyDOMElement`, or `not.toBe('')` (also `not.toEqual('')` / `not.toStrictEqual('')`). -```tsx bad filename=src/TurnstileField.test.tsx reports=2 +What the call proves decides which of those presence checks count: + +- A `render*` call (`render(...)`, `renderWithProviders(...)`, `rtl.render(...)`, also after + `await`) proves the root is a DOM node, so every presence check on it counts, + `expect(container).not.toBeNull()` included. +- Any other call (`setup()`, `mount(Card)`) could return anything, so its root counts only when + the assertion itself is DOM-specific: the subject reads one of the members above off the root + (`container.firstChild`), or the matcher exists only for DOM nodes (`toBeInTheDocument`, + `toBeVisible`, `not.toBeEmptyDOMElement`). + +```tsx bad filename=src/TurnstileField.test.tsx reports=3 it('renders without crashing', () => { const { container } = render(); expect(container).not.toBeEmptyDOMElement(); @@ -84,6 +94,11 @@ it('renders the card', () => { const { container } = render(); expect(container.firstChild).toBeInTheDocument(); }); + +it('mounts the form', () => { + const { container } = setup(); + expect(container.firstChild).not.toBeNull(); +}); ``` ```tsx good filename=src/TurnstileField.test.tsx @@ -96,6 +111,11 @@ it('renders the card', () => { render(); expect(screen.getByRole('heading', { name: 'Q3' })).toBeInTheDocument(); }); + +it('mounts the form', () => { + setup(); + expect(screen.getByRole('form', { name: 'Sign in' })).toBeVisible(); +}); ``` ## What it does not flag @@ -108,9 +128,12 @@ it('renders the card', () => { (`ship.container`), or a binding initialised from something other than a `render*` call (`const container = await docker.inspect(id)`). A binding assigned later (`let container; beforeEach(() => ({ container } = render(...)))`) is not followed either. - The reverse also holds: a `container` destructured from a call that is not a render - (`const { container } = await docker.inspect(id)`) is treated as a render root; set `renderRoots` - for such a suite. +- A root from a call that is not a `render*` function, checked with a matcher that says nothing + about the DOM: `const { container } = await docker.inspect(id); expect(container).not.toBeNull()` + is accepted, since nothing shows that `container` is a DOM node. A weak matcher there + (`toBeTruthy`, `toBeDefined`) is still reported, as `soleWeakExpect`. The same goes for a render + helper whose name does not start with `render`: `const { container } = setup(); + expect(container).not.toBeNull()` is not reported. - A render root checked for real content (`expect(container.textContent).toBe('Hello')`), a query on the root (`container.querySelector('nav')`), or a presence check next to another assertion. diff --git a/packages/eslint-plugin-code-quality/docs/rules/typed-mock-over-double-cast.md b/packages/eslint-plugin-code-quality/docs/rules/typed-mock-over-double-cast.md index 4ba56d5..6876ba6 100644 --- a/packages/eslint-plugin-code-quality/docs/rules/typed-mock-over-double-cast.md +++ b/packages/eslint-plugin-code-quality/docs/rules/typed-mock-over-double-cast.md @@ -52,6 +52,32 @@ const host: jest.Mocked> = { }; ``` +A mock created first and placed in the object by name counts too, shorthand (`{ send }`) or not +(`{ run: send }`): the name must be bound to a mock-function call, or to a chain that starts with +one, by its initialiser or by a plain assignment (`let get; beforeEach(() => { get = jest.fn(); })`). + +```ts bad filename=src/mail/mailer.spec.ts reports=2 +const send = vi.fn(); +const mailer = { send } as unknown as Mailer; + +let get: jest.Mock; +beforeEach(() => { + get = jest.fn().mockReturnValue('smtp://localhost'); + config = { get } as unknown as ConfigService; +}); +``` + +```ts good filename=src/mail/mailer.spec.ts +const send = vi.fn(); +const mailer: Mocked> = { send }; + +let get: jest.Mock; +beforeEach(() => { + get = jest.fn().mockReturnValue('smtp://localhost'); + config = { get } satisfies Partial; +}); +``` + ## What it does not flag - A single cast (`as jest.Mocked>`): TypeScript still checks that the two types @@ -59,6 +85,9 @@ const host: jest.Mocked> = { - A double cast of an object with no mock in it (a data fixture), or of something that is not an object literal (`existingDouble as unknown as T`). - `as unknown` on its own. +- A mock the rule cannot trace to its factory call in the same file: one that arrives through a + spread (`{ ...mocks }`), an import, a parameter, a destructuring or a helper's return value + (`{ send: makeSend() }`). - A target matching `allowTargets`. ## Options diff --git a/packages/eslint-plugin-code-quality/src/rules/no-message-only-throw-assertion.ts b/packages/eslint-plugin-code-quality/src/rules/no-message-only-throw-assertion.ts index 6ef3993..6dcf4f1 100644 --- a/packages/eslint-plugin-code-quality/src/rules/no-message-only-throw-assertion.ts +++ b/packages/eslint-plugin-code-quality/src/rules/no-message-only-throw-assertion.ts @@ -1,7 +1,8 @@ -import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, ASTUtils, type TSESTree } from '@typescript-eslint/utils'; import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema'; import { createRule } from '../createRule'; +import { isSelfOrAncestor, runnerName, unwrapTypeWrappers } from '../utils/ast'; const RULE_NAME = 'no-message-only-throw-assertion'; @@ -126,16 +127,6 @@ function enclosingBlock(node: TSESTree.Node): TSESTree.Node { return current; } -/** True when `ancestor` is `node` or contains it. */ -function isSelfOrAncestor(ancestor: TSESTree.Node, node: TSESTree.Node): boolean { - for (let current: TSESTree.Node | undefined = node; current; current = current.parent) { - if (current === ancestor) { - return true; - } - } - return false; -} - /** Decompose `expect(x).rejects.not.toThrow(y)` into its parts, or null. */ function matcherChain(node: TSESTree.CallExpression): MatcherChain | null { const callee = node.callee; @@ -185,23 +176,6 @@ function calleePath(callee: TSESTree.Expression): string | null { return null; } -/** Root identifier of a test callee: `it`, `test.concurrent`, `it.each(table)`. */ -function runnerName(callee: TSESTree.Node): string | null { - let current: TSESTree.Node = callee; - for (;;) { - if (current.type === AST_NODE_TYPES.MemberExpression) { - current = current.object; - } else if (current.type === AST_NODE_TYPES.CallExpression) { - current = current.callee; - } else if (current.type === AST_NODE_TYPES.TaggedTemplateExpression) { - current = current.tag; - } else { - break; - } - } - return current.type === AST_NODE_TYPES.Identifier ? current.name : null; -} - function isTestCallback(node: FunctionNode): boolean { const parent = node.parent; if (parent.type !== AST_NODE_TYPES.CallExpression || !parent.arguments.includes(node)) { @@ -358,6 +332,44 @@ export const noMessageOnlyThrowAssertionRule = createRule reference.isWrite() && !reference.init) + ) { + return null; + } + return declarator.init; + } + + /** + * True for a message check written inline or held in a variable + * (`const expected = /no access/`). One hop only: a variable initialised + * from another variable is not followed. + */ + function isMessageCheck(argument: TSESTree.Node): boolean { + if (isMessageArgument(argument)) { + return true; + } + if (argument.type !== AST_NODE_TYPES.Identifier) { + return false; + } + const init = constantInit(argument); + return init !== null && isMessageArgument(unwrapTypeWrappers(init)); + } + /** True when `.rejects.(argument)` pins more than the message. */ function rejectsPinsClass(matcher: string, argument: TSESTree.Node | undefined): boolean { const comparesInstance = @@ -383,7 +395,7 @@ export const noMessageOnlyThrowAssertionRule = createRule({ const findings: { node: TSESTree.Node; delay: string; handle: string | null }[] = []; /** Source text of every handle passed to `clearTimeout` / `clearInterval` in the file. */ const clearedHandles = new Set(); - let fakesTimers = false; + /** Test and suite callbacks that install fake timers. */ + const fakedScopes = new Set(); + /** True when fake timers are installed outside any test or suite, so for every test. */ + let fakesWholeFile = false; + + /** True when `node` sits in a test or suite that installs fake timers, or the file does. */ + function isUnderFakeTimers(node: TSESTree.Node): boolean { + if (fakesWholeFile) { + return true; + } + let current: TSESTree.Node | undefined = node; + for (; current != null; current = current.parent) { + if (fakedScopes.has(current)) { + return true; + } + } + return false; + } + + /** + * True when `node` only runs under fake timers: it sits in a test or suite + * that installs them, or in a helper that the file calls, and only from + * such places. `seen` holds the helpers already entered in this query, so + * helpers that call each other are judged by their other callers and a + * helper reached twice is walked once. + */ + function runsOnlyUnderFakeTimers(node: TSESTree.Node, seen: Set): boolean { + if (isUnderFakeTimers(node)) { + return true; + } + const helper = enclosingHelper(node); + if (helper === null) { + return false; + } + if (seen.has(helper.fn)) { + return true; + } + seen.add(helper.fn); + const variable = context.sourceCode + .getDeclaredVariables(helper.declaration) + .find((candidate) => candidate.identifiers.includes(helper.id)); + const uses = (variable?.references ?? []).filter( + (reference) => reference.isRead() && !isSelfOrAncestor(helper.fn, reference.identifier), + ); + return uses.length > 0 && uses.every((use) => runsOnlyUnderFakeTimers(use.identifier, seen)); + } function record( node: TSESTree.Node, @@ -306,7 +409,12 @@ export const noSleepInUnitTestsRule = createRule({ callee.type === AST_NODE_TYPES.MemberExpression && fakeTimerMethods.has(staticPropertyName(callee) ?? '') ) { - fakesTimers = true; + const scope = fakeTimerScope(node); + if (scope === null) { + fakesWholeFile = true; + } else { + fakedScopes.add(scope); + } return; } if (callee.type === AST_NODE_TYPES.Identifier && sleepFunctions.has(callee.name)) { @@ -341,13 +449,13 @@ export const noSleepInUnitTestsRule = createRule({ } }, 'Program:exit'(): void { - if (fakesTimers) { - return; - } for (const { node, delay, handle } of findings) { if (handle !== null && clearedHandles.has(handle)) { continue; } + if (runsOnlyUnderFakeTimers(node, new Set())) { + continue; + } context.report({ node, messageId: 'sleepInUnitTest', data: { delay } }); } }, diff --git a/packages/eslint-plugin-code-quality/src/rules/no-vacuous-expect.ts b/packages/eslint-plugin-code-quality/src/rules/no-vacuous-expect.ts index 6d3f550..0fd1e41 100644 --- a/packages/eslint-plugin-code-quality/src/rules/no-vacuous-expect.ts +++ b/packages/eslint-plugin-code-quality/src/rules/no-vacuous-expect.ts @@ -8,6 +8,7 @@ import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema'; import { createRule } from '../createRule'; +import { runnerName } from '../utils/ast'; const RULE_NAME = 'no-vacuous-expect'; @@ -58,16 +59,17 @@ const RENDER_ROOT_MEMBERS = new Set([ 'textContent', ]); +/** Presence matchers that exist only for DOM nodes, so their subject is one whatever returned it. */ +const DOM_ONLY_MATCHERS = new Set(['toBeInTheDocument', 'toBeVisible', 'not.toBeEmptyDOMElement']); + /** Matchers that, on a render root, only prove the render produced something. */ const RENDER_ROOT_PRESENCE_MATCHERS = new Set([ - 'toBeInTheDocument', + ...DOM_ONLY_MATCHERS, 'toBeTruthy', 'toBeDefined', - 'toBeVisible', 'not.toBeNull', 'not.toBeUndefined', 'not.toBeFalsy', - 'not.toBeEmptyDOMElement', ]); /** Negated equality matchers that are presence checks when compared with `''`. */ @@ -117,6 +119,12 @@ interface MatcherCall { readonly matcher: string; } +/** + * What a render root comes from: a `render*` call, which proves it is a DOM node, or some other + * call (`setup()`, `docker.inspect(id)`), which does not. + */ +type RootOrigin = 'render' | 'call'; + function isExpectCall(node: TSESTree.Node): node is TSESTree.CallExpression { return ( node.type === AST_NODE_TYPES.CallExpression && @@ -183,6 +191,14 @@ function isRenderCall(node: TSESTree.Node | null): boolean { return name !== null && name.startsWith('render'); } +/** The origin a call gives its result, or null when `node` is not a call. */ +function callOrigin(node: TSESTree.Node | null): RootOrigin | null { + if (node?.type !== AST_NODE_TYPES.CallExpression) { + return null; + } + return isRenderCall(node) ? 'render' : 'call'; +} + /** True when `matcher(expected)` only proves the subject is present or non-empty. */ function isPresenceMatcher(matcher: string, expected: TSESTree.Node | undefined): boolean { if (RENDER_ROOT_PRESENCE_MATCHERS.has(matcher)) { @@ -211,23 +227,6 @@ function calleePath(callee: TSESTree.Expression): string | null { return null; } -/** Root identifier of a test callee: `it`, `test.concurrent`, `it.each(table)`, ``test.each`...` ``. */ -function runnerName(callee: TSESTree.Node): string | null { - let current: TSESTree.Node = callee; - for (;;) { - if (current.type === AST_NODE_TYPES.MemberExpression) { - current = current.object; - } else if (current.type === AST_NODE_TYPES.CallExpression) { - current = current.callee; - } else if (current.type === AST_NODE_TYPES.TaggedTemplateExpression) { - current = current.tag; - } else { - break; - } - } - return current.type === AST_NODE_TYPES.Identifier ? current.name : null; -} - function isTestCallback(node: TestCallback): boolean { const parent = node.parent; if (parent.type !== AST_NODE_TYPES.CallExpression || parent.arguments[1] !== node) { @@ -312,55 +311,63 @@ export const noVacuousExpectRule = createRule({ return null; } - /** True for a value that is a render result: a call, or a binding initialised from one. */ - function isRenderResult(node: TSESTree.Node): boolean { - if (unwrapAwait(node)?.type === AST_NODE_TYPES.CallExpression) { - return true; + /** The origin of a call result: a call, or a binding initialised from one. Else null. */ + function resultOrigin(node: TSESTree.Node): RootOrigin | null { + const direct = callOrigin(unwrapAwait(node)); + if (direct !== null || node.type !== AST_NODE_TYPES.Identifier) { + return direct; } - if (node.type !== AST_NODE_TYPES.Identifier) { - return false; - } - const init = unwrapAwait(declaratorOf(node)?.init); - return init?.type === AST_NODE_TYPES.CallExpression; + return callOrigin(unwrapAwait(declaratorOf(node)?.init)); } /** - * True for a render root that comes from a render: `const { container } = render(...)`, + * The origin of a render root that comes from a call: `const { container } = render(...)`, * `const container = render(...).container`, `const container = renderIntoDocument(...)`, - * `view.container` where `view = render(...)`, or `render(...).container`. + * `view.container` where `view = render(...)`, or `render(...).container`. Null for a name + * that is not a render root or does not come from a call. A root bound whole + * (`const container = call()`) must come from a `render*` call: only a destructure or a + * member read says the call returned an object with a root on it. */ - function isRenderRoot(node: TSESTree.Node): boolean { + function rootOrigin(node: TSESTree.Node): RootOrigin | null { if (node.type === AST_NODE_TYPES.MemberExpression) { const name = memberName(node); - return name !== null && renderRoots.has(name) && isRenderResult(node.object); + return name !== null && renderRoots.has(name) ? resultOrigin(node.object) : null; } if (node.type !== AST_NODE_TYPES.Identifier || !renderRoots.has(node.name)) { - return false; + return null; } const declarator = declaratorOf(node); const init = unwrapAwait(declarator?.init); if (declarator === null || init === null) { - return false; + return null; } if (declarator.id.type === AST_NODE_TYPES.ObjectPattern) { - return init.type === AST_NODE_TYPES.CallExpression; + return callOrigin(init); + } + if (isRenderCall(init)) { + return 'render'; } - return isRenderCall(init) || (init.type === AST_NODE_TYPES.MemberExpression && isRenderRoot(init)); + return init.type === AST_NODE_TYPES.MemberExpression ? rootOrigin(init) : null; } - /** True for an expect subject that is the render root or a presence-only member of it. */ - function isRenderRootSubject(node: TSESTree.Node | undefined): boolean { + /** + * True for an expect subject that is the render root or a presence-only member of it. A + * root from a call that is not a `render*` function counts only when the assertion is + * DOM-specific: the subject reads a DOM member off it, or `matcher` exists only for DOM nodes. + */ + function isRenderRootSubject(node: TSESTree.Node | undefined, matcher: string): boolean { if (node === undefined) { return false; } - if (isRenderRoot(node)) { - return true; + const origin = rootOrigin(node); + if (origin !== null) { + return origin === 'render' || DOM_ONLY_MATCHERS.has(matcher); } if (node.type !== AST_NODE_TYPES.MemberExpression) { return false; } const name = memberName(node); - return name !== null && RENDER_ROOT_MEMBERS.has(name) && isRenderRoot(node.object); + return name !== null && RENDER_ROOT_MEMBERS.has(name) && rootOrigin(node.object) !== null; } function enter(node: TestCallback): void { @@ -403,7 +410,7 @@ export const noVacuousExpectRule = createRule({ if (frame !== undefined) { frame.assertions += 1; if ( - isRenderRootSubject(call.root.arguments[0]) && + isRenderRootSubject(call.root.arguments[0], call.matcher) && isPresenceMatcher(call.matcher, node.arguments[0]) ) { frame.weak = { node, matcher: call.matcher, messageId: 'soleRenderRootExpect' }; diff --git a/packages/eslint-plugin-code-quality/src/rules/typed-mock-over-double-cast.ts b/packages/eslint-plugin-code-quality/src/rules/typed-mock-over-double-cast.ts index 34cde27..38afacc 100644 --- a/packages/eslint-plugin-code-quality/src/rules/typed-mock-over-double-cast.ts +++ b/packages/eslint-plugin-code-quality/src/rules/typed-mock-over-double-cast.ts @@ -1,9 +1,9 @@ -import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, ASTUtils, type TSESTree } from '@typescript-eslint/utils'; import type { JSONSchema4 } from '@typescript-eslint/utils/json-schema'; import { createRule } from '../createRule'; import { matchesAny } from '../utils/allowMatch'; -import { walkSome } from '../utils/ast'; +import { unwrapTypeWrappers, walkSome } from '../utils/ast'; const RULE_NAME = 'typed-mock-over-double-cast'; @@ -122,6 +122,54 @@ export const typedMockOverDoubleCastRule = createRule({ return path !== null && mockFactories.has(path); } + /** True for a mock call or a call chain rooted at one (`jest.fn().mockResolvedValue(1)`). */ + function isMockValue(node: TSESTree.Node): boolean { + const value = unwrapTypeWrappers(node); + if (value.type !== AST_NODE_TYPES.CallExpression) { + return false; + } + return ( + isMockCall(value) || + (value.callee.type === AST_NODE_TYPES.MemberExpression && isMockValue(value.callee.object)) + ); + } + + /** True when `identifier` is the direct target of a write of a mock: `fn = jest.fn()`. */ + function isMockWrite(identifier: TSESTree.Node): boolean { + const parent = identifier.parent; + if (parent?.type === AST_NODE_TYPES.VariableDeclarator) { + return parent.id === identifier && parent.init !== null && isMockValue(parent.init); + } + return ( + parent?.type === AST_NODE_TYPES.AssignmentExpression && + parent.operator === '=' && + parent.left === identifier && + isMockValue(parent.right) + ); + } + + /** + * True for a property value that names a mock created earlier: `{ fn }` or + * `{ run: fn }` where `fn` is initialised with, or assigned, a mock. + */ + function isMockReference(node: TSESTree.Node): boolean { + const parent = node.parent; + if ( + node.type !== AST_NODE_TYPES.Identifier || + parent?.type !== AST_NODE_TYPES.Property || + parent.value !== node || + parent.parent.type !== AST_NODE_TYPES.ObjectExpression + ) { + return false; + } + const variable = ASTUtils.findVariable(context.sourceCode.getScope(node), node); + return ( + variable?.references.some( + (reference) => reference.isWrite() && isMockWrite(reference.identifier), + ) ?? false + ); + } + /** Report an object of mocks cast through `unknown` / `any` / `never` to a real type. */ function checkCast(node: Cast): void { const inner = node.expression; @@ -133,7 +181,12 @@ export const typedMockOverDoubleCastRule = createRule({ ) { return; } - if (!walkSome(inner.expression, keys, isMockCall)) { + const holdsMock = walkSome( + inner.expression, + keys, + (child) => isMockCall(child) || isMockReference(child), + ); + if (!holdsMock) { return; } const text = context.sourceCode.getText(node.typeAnnotation); diff --git a/packages/eslint-plugin-code-quality/src/utils/ast.ts b/packages/eslint-plugin-code-quality/src/utils/ast.ts index d1300d2..e1c7978 100644 --- a/packages/eslint-plugin-code-quality/src/utils/ast.ts +++ b/packages/eslint-plugin-code-quality/src/utils/ast.ts @@ -28,6 +28,48 @@ export function isStaticMemberAccess( return node.property.type === AST_NODE_TYPES.Identifier && node.property.name === propertyName; } +/** True when `ancestor` is `node` or contains it. */ +export function isSelfOrAncestor(ancestor: TSESTree.Node, node: TSESTree.Node): boolean { + // `parent` is null on the Program node at runtime, whatever the types say. + for (let current: TSESTree.Node | undefined = node; current != null; current = current.parent) { + if (current === ancestor) { + return true; + } + } + return false; +} + +/** Root identifier of a runner callee: `it`, `test.concurrent`, `describe.each(table)`, ``test.each`...` ``. */ +export function runnerName(callee: TSESTree.Node): string | null { + let current: TSESTree.Node = callee; + for (;;) { + if (current.type === AST_NODE_TYPES.MemberExpression) { + current = current.object; + } else if (current.type === AST_NODE_TYPES.CallExpression) { + current = current.callee; + } else if (current.type === AST_NODE_TYPES.TaggedTemplateExpression) { + current = current.tag; + } else { + break; + } + } + return current.type === AST_NODE_TYPES.Identifier ? current.name : null; +} + +/** `x` for `x as T`, `x`, `x satisfies T` and `x!`, else the node itself. */ +export function unwrapTypeWrappers(node: TSESTree.Node): TSESTree.Node { + let current = node; + while ( + current.type === AST_NODE_TYPES.TSAsExpression || + current.type === AST_NODE_TYPES.TSTypeAssertion || + current.type === AST_NODE_TYPES.TSSatisfiesExpression || + current.type === AST_NODE_TYPES.TSNonNullExpression + ) { + current = current.expression; + } + return current; +} + /** Child keys per node type, from the parser (`context.sourceCode.visitorKeys`). */ export type VisitorKeys = Readonly>; diff --git a/packages/eslint-plugin-code-quality/tests/rules/no-message-only-throw-assertion.test.ts b/packages/eslint-plugin-code-quality/tests/rules/no-message-only-throw-assertion.test.ts index 8f11e5f..1957112 100644 --- a/packages/eslint-plugin-code-quality/tests/rules/no-message-only-throw-assertion.test.ts +++ b/packages/eslint-plugin-code-quality/tests/rules/no-message-only-throw-assertion.test.ts @@ -106,6 +106,44 @@ ruleTester.run('no-message-only-throw-assertion', noMessageOnlyThrowAssertionRul }, // Not an expect chain. { code: 'emitter.toThrow();' }, + // An identifier that is not bound to a message value is read as a class. + { code: "const Expected = NotFoundError; it('refuses', () => { expect(() => run()).toThrow(Expected); });" }, + { + code: "class QuotaError extends Error {} it('refuses', () => { expect(() => run()).toThrow(QuotaError); });", + }, + { + code: "import { NotFoundError } from './errors'; it('refuses', async () => { await expect(load(1)).rejects.toThrow(NotFoundError); });", + }, + // A parameter, a call result and a reassigned binding are not followed. + { + code: "it.each([NotFoundError, GoneError])('refuses with %p', (expected) => { expect(() => run()).toThrow(expected); });", + }, + { code: "it('refuses', () => { const expected = buildError(); expect(() => run()).toThrow(expected); });" }, + { + code: ` + it('refuses', () => { + let expected = 'No access'; + expected = ForbiddenError; + expect(() => run()).toThrow(expected); + }); + `, + }, + { code: "it('refuses', () => { let expected; expected = /no access/; expect(() => run()).toThrow(expected); });" }, + // allowMessageOnly accepts a message held in a variable like any other message check. + { + code: "const MESSAGE = 'No access'; it('refuses', () => { expect(() => run()).toThrow(MESSAGE); });", + options: [{ allowMessageOnly: true }], + }, + // A message constant next to a class pin on the same subject. + { + code: ` + const MESSAGE = 'Employee not found.'; + it('refuses', async () => { + await expect(service.find(id)).rejects.toThrow(NotFoundError); + await expect(service.find(id)).rejects.toThrow(MESSAGE); + }); + `, + }, ], invalid: [ // Settly shape: a Prisma rejection asserted with no argument. @@ -292,6 +330,58 @@ ruleTester.run('no-message-only-throw-assertion', noMessageOnlyThrowAssertionRul `, errors: [{ messageId: 'messageOnlyThrow' }], }, + // A message held in a variable is still a message check. + { + code: ` + it('refuses', async () => { + const expected = /no access/; + await expect(run()).rejects.toThrow(expected); + }); + `, + errors: [{ messageId: 'messageOnlyThrow', data: { matcher: 'toThrow' }, line: 4 }], + }, + { + code: "it('refuses', () => { const expected = 'No access'; expect(() => run()).toThrowError(expected); });", + errors: [{ messageId: 'messageOnlyThrow', data: { matcher: 'toThrowError' } }], + }, + { + code: "it('refuses', () => { const expected = `Queue ${name} missing`; expect(() => run()).toThrow(expected); });", + errors: [{ messageId: 'messageOnlyThrow' }], + }, + { + code: "it('refuses', () => { const expected = new RegExp(`Queue \"${missing}\" is declared`); expect(() => run()).toThrow(expected); });", + errors: [{ messageId: 'messageOnlyThrow' }], + }, + // A module-level constant, a `let` that is never reassigned and an `as const` string. + { + code: ` + const MESSAGE = 'Employee not found.'; + it('refuses', () => { expect(() => run()).toThrow(MESSAGE); }); + `, + errors: [{ messageId: 'messageOnlyThrow', line: 3 }], + }, + { + code: "it('refuses', async () => { let expected = /no access/; await expect(run()).rejects.toThrow(expected); });", + errors: [{ messageId: 'messageOnlyThrow' }], + }, + { + code: "const MESSAGE = 'No access' as const; it('refuses', () => { expect(() => run()).toThrow(MESSAGE); });", + errors: [{ messageId: 'messageOnlyThrow' }], + }, + // A message variable does not pin the class for a later message check. + { + code: ` + it('refuses', async () => { + const expected = /no access/; + await expect(load(id)).rejects.toThrow(expected); + await expect(load(id)).rejects.toThrow('No access'); + }); + `, + errors: [ + { messageId: 'messageOnlyThrow', line: 4 }, + { messageId: 'messageOnlyThrow', line: 5 }, + ], + }, // Outside a test callback (a shared helper) it still reports. { code: 'export async function expectRefusal(p) { await expect(p).rejects.toThrow(); }', diff --git a/packages/eslint-plugin-code-quality/tests/rules/no-sleep-in-unit-tests.test.ts b/packages/eslint-plugin-code-quality/tests/rules/no-sleep-in-unit-tests.test.ts index 9f2491d..898c5c1 100644 --- a/packages/eslint-plugin-code-quality/tests/rules/no-sleep-in-unit-tests.test.ts +++ b/packages/eslint-plugin-code-quality/tests/rules/no-sleep-in-unit-tests.test.ts @@ -62,6 +62,8 @@ ruleTester.run('no-sleep-in-unit-tests', noSleepInUnitTestsRule, { code: "it('debounces', () => { setTimeout(onSave, 300); vi.advanceTimersByTime(300); expect(onSave).toHaveBeenCalled(); });", filename: TEST_FILE, }, + // A timer at the top level of the file has no enclosing function to be an executor. + { code: 'setTimeout(onSave, 300);', filename: TEST_FILE }, // Settly shape: a fake slow stream in a file that fakes timers, so the wait is virtual. { code: ` @@ -87,6 +89,134 @@ ruleTester.run('no-sleep-in-unit-tests', noSleepInUnitTestsRule, { code: "jest.useFakeTimers(); it('x', async () => { const p = new Promise((r) => setTimeout(r, 1000)); jest.advanceTimersByTime(1000); await p; });", filename: TEST_FILE, }, + // Fake timers installed for a suite cover its tests, nested suites included. + { + code: ` + describe('polling', () => { + beforeEach(() => { vi.useFakeTimers(); }); + afterEach(() => { vi.useRealTimers(); }); + it('polls', async () => { + const tick = new Promise((resolve) => setTimeout(resolve, 1_000)); + await vi.advanceTimersByTimeAsync(1_000); + await tick; + }); + describe('when offline', () => { + it('backs off', async () => { + const tick = new Promise((resolve) => setTimeout(resolve, 5_000)); + await vi.advanceTimersByTimeAsync(5_000); + await tick; + }); + }); + }); + describe('parsing', () => { it('parses', () => { expect(parse('1')).toBe(1); }); }); + `, + filename: TEST_FILE, + }, + // A test that installs fake timers itself covers the rest of its callback. + { + code: ` + it('advances', async () => { + vi.useFakeTimers(); + const tick = new Promise((resolve) => setTimeout(resolve, 1_000)); + await vi.advanceTimersByTimeAsync(1_000); + await tick; + vi.useRealTimers(); + }); + it('parses', () => { expect(parse('1')).toBe(1); }); + `, + filename: TEST_FILE, + }, + // A top-level sleep helper that is only called where the timers are faked. + { + code: ` + const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + describe('polling', () => { + beforeEach(() => { vi.useFakeTimers(); }); + it('polls', async () => { + const done = sleep(1_000); + await vi.advanceTimersByTimeAsync(1_000); + await done; + }); + }); + describe('parsing', () => { it('parses', () => { expect(parse('1')).toBe(1); }); }); + `, + filename: TEST_FILE, + }, + // A helper reached only through another helper, itself only called under fake timers. + { + code: ` + function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); } + async function slowReply(body: string) { await sleep(2_000); return body; } + it('times out', async () => { + vi.useFakeTimers(); + const reply = slowReply('late'); + await vi.advanceTimersByTimeAsync(2_000); + expect(await reply).toBe('late'); + vi.useRealTimers(); + }); + it('parses', () => { expect(parse('1')).toBe(1); }); + `, + filename: TEST_FILE, + }, + // A helper that calls itself, and two that call each other, are judged by their other callers. + { + code: ` + async function retry(times: number) { + await new Promise((resolve) => setTimeout(resolve, 100)); + if (times > 0) { await retry(times - 1); } + } + async function ping(n: number) { await new Promise((resolve) => setTimeout(resolve, 10)); if (n > 0) { await pong(n - 1); } } + async function pong(n: number) { await new Promise((resolve) => setTimeout(resolve, 10)); if (n > 0) { await ping(n - 1); } } + describe('backoff', () => { + beforeEach(() => { jest.useFakeTimers(); }); + it('retries', async () => { const done = retry(2); await jest.advanceTimersByTimeAsync(300); await done; }); + it('rallies', async () => { const done = ping(2); await jest.advanceTimersByTimeAsync(30); await done; }); + }); + it('parses', () => { expect(parse('1')).toBe(1); }); + `, + filename: TEST_FILE, + }, + // `describe.each` and `it.each` are a suite and a test like any other. + { + code: ` + describe.each(['get', 'post'])('%s', (method) => { + beforeAll(() => { jest.useFakeTimers(); }); + it.each([100, 200])('waits %i', async (ms) => { + const tick = new Promise((resolve) => setTimeout(resolve, ms)); + jest.advanceTimersByTime(ms); + await tick; + }); + }); + describe('parsing', () => { it('parses', () => { expect(parse('1')).toBe(1); }); }); + `, + filename: TEST_FILE, + }, + { + code: ` + it.each([100, 200])('waits %i', async (ms) => { + vi.useFakeTimers(); + const tick = new Promise((resolve) => setTimeout(resolve, ms)); + vi.advanceTimersByTime(ms); + await tick; + }); + it('parses', () => { expect(parse('1')).toBe(1); }); + `, + filename: TEST_FILE, + }, + // A top-level helper that installs the timers may run before any test: the whole file is exempt. + { + code: ` + function installClock() { vi.useFakeTimers(); } + it('advances', async () => { + installClock(); + const tick = new Promise((resolve) => setTimeout(resolve, 1_000)); + await vi.advanceTimersByTimeAsync(1_000); + await tick; + }); + it('settles', async () => { installClock(); await new Promise((resolve) => setTimeout(resolve, 50)); }); + `, + filename: TEST_FILE, + }, // Not a unit test file. { code: "it('waits', async () => { await new Promise((resolve) => setTimeout(resolve, 100)); });", @@ -197,6 +327,82 @@ ruleTester.run('no-sleep-in-unit-tests', noSleepInUnitTestsRule, { options: [{ fakeTimerMethods: ['installClock'] }], errors: [{ messageId: 'sleepInUnitTest' }], }, + // One test faking its timers does not cover a sibling test that sleeps for real. + { + code: ` + it('advances', async () => { + vi.useFakeTimers(); + await vi.advanceTimersByTimeAsync(100); + vi.useRealTimers(); + }); + it('waits', async () => { + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', data: { delay: '100' }, line: 8 }], + }, + // Fake timers installed for one suite do not cover a sibling suite. + { + code: ` + describe('polling', () => { + beforeEach(() => { vi.useFakeTimers(); }); + afterEach(() => { vi.useRealTimers(); }); + it('polls', async () => { await vi.advanceTimersByTimeAsync(1_000); }); + }); + describe('upload', () => { + it('settles', async () => { await new Promise((resolve) => setTimeout(resolve, 50)); }); + }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', data: { delay: '50' }, line: 8 }], + }, + // The same for a timers/promises sleep. + { + code: ` + import { setTimeout as delay } from 'node:timers/promises'; + it('advances', () => { jest.useFakeTimers(); jest.advanceTimersByTime(10); jest.useRealTimers(); }); + it('waits', async () => { await delay(200); }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', data: { delay: '200' }, line: 4 }], + }, + // A helper called under fake timers and from a real-timer test is reported where it sleeps. + { + code: ` + const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + describe('polling', () => { + beforeEach(() => { vi.useFakeTimers(); }); + it('polls', async () => { + const done = sleep(1_000); + await vi.advanceTimersByTimeAsync(1_000); + await done; + }); + }); + it('retries', async () => { await sleep(100); expect(calls).toBe(2); }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', data: { delay: 'ms' }, line: 2 }], + }, + // A helper nobody calls under fake timers: an unrelated test faking its clock does not cover it. + { + code: ` + export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + it('advances', () => { vi.useFakeTimers(); vi.advanceTimersByTime(10); vi.useRealTimers(); }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', line: 2 }], + }, + { + code: ` + function sleep(ms: number) { return new Promise((resolve) => setTimeout(resolve, ms)); } + async function slowReply(body: string) { await sleep(2_000); return body; } + it('advances', () => { vi.useFakeTimers(); vi.advanceTimersByTime(10); vi.useRealTimers(); }); + it('replies', async () => { expect(await slowReply('late')).toBe('late'); }); + `, + filename: TEST_FILE, + errors: [{ messageId: 'sleepInUnitTest', data: { delay: 'ms' }, line: 2 }], + }, // A custom suffix brings a file into scope. { code: "it('x', async () => { await new Promise((r) => setTimeout(r, 5)); });", diff --git a/packages/eslint-plugin-code-quality/tests/rules/no-vacuous-expect.test.ts b/packages/eslint-plugin-code-quality/tests/rules/no-vacuous-expect.test.ts index c8d7961..c09eb38 100644 --- a/packages/eslint-plugin-code-quality/tests/rules/no-vacuous-expect.test.ts +++ b/packages/eslint-plugin-code-quality/tests/rules/no-vacuous-expect.test.ts @@ -68,6 +68,21 @@ ruleTester.run('no-vacuous-expect', noVacuousExpectRule, { { code: "it('ships', () => { expect(ship.container).toBeVisible(); });" }, { code: "it('loads', () => { const { container } = fixtures; expect(container).not.toBeEmptyDOMElement(); });" }, { code: "it('packs', ({ container }) => { expect(container).not.toBeEmptyDOMElement(); });" }, + // A root from a call that is not a `render*` function, checked with a generic matcher, is + // not known to be a DOM node. + { + code: "it('inspects', async () => { const { container } = await docker.inspect(id); expect(container).not.toBeNull(); });", + }, + { + code: "it('inspects', async () => { const info = await docker.inspect(id); expect(info.container).not.toBeNull(); });", + }, + { + code: "it('inspects', async () => { expect((await docker.inspect(id)).container).not.toBeFalsy(); });", + }, + { + code: "it('mounts', () => { const { container } = setup(); expect(container).not.toBeNull(); });", + filename: 'src/Card.test.tsx', + }, // renderRoots: [] turns the render-root check off. { code: "it('renders', () => { const { container } = render(); expect(container).not.toBeEmptyDOMElement(); });", @@ -175,6 +190,34 @@ ruleTester.run('no-vacuous-expect', noVacuousExpectRule, { filename: 'src/Card.test.tsx', errors: [{ messageId: 'soleRenderRootExpect' }], }, + // A `render*` call proves the root, so a generic presence matcher on it is a smoke test. + { + code: "it('renders', () => { const { container } = render(); expect(container).not.toBeNull(); });", + filename: 'src/Card.test.tsx', + errors: [{ messageId: 'soleRenderRootExpect', data: { matcher: 'not.toBeNull' } }], + }, + // A root from any other call counts when the assertion itself is DOM-specific: a DOM-only + // matcher, or a DOM member read off the root. + { + code: "it('mounts', () => { const { container } = setup(); expect(container).toBeInTheDocument(); });", + filename: 'src/Card.test.tsx', + errors: [{ messageId: 'soleRenderRootExpect', data: { matcher: 'toBeInTheDocument' } }], + }, + { + code: "it('mounts', () => { const { container } = setup(); expect(container.firstChild).not.toBeNull(); });", + filename: 'src/Card.test.tsx', + errors: [{ messageId: 'soleRenderRootExpect', data: { matcher: 'not.toBeNull' } }], + }, + { + code: "it('mounts', () => { const view = setup(); expect(view.container).toBeVisible(); });", + filename: 'src/Card.test.tsx', + errors: [{ messageId: 'soleRenderRootExpect', data: { matcher: 'toBeVisible' } }], + }, + // Without that evidence a weak matcher on the same binding is still a sole weak expect. + { + code: "it('inspects', async () => { const { container } = await docker.inspect(id); expect(container).toBeTruthy(); });", + errors: [{ messageId: 'soleWeakExpect', data: { matcher: 'toBeTruthy' } }], + }, // A custom root name. { code: "it('mounts', () => { const { root } = mount(Card); expect(root).toBeInTheDocument(); });", diff --git a/packages/eslint-plugin-code-quality/tests/rules/typed-mock-over-double-cast.test.ts b/packages/eslint-plugin-code-quality/tests/rules/typed-mock-over-double-cast.test.ts index 68cec95..86d79bb 100644 --- a/packages/eslint-plugin-code-quality/tests/rules/typed-mock-over-double-cast.test.ts +++ b/packages/eslint-plugin-code-quality/tests/rules/typed-mock-over-double-cast.test.ts @@ -35,6 +35,18 @@ ruleTester.run('typed-mock-over-double-cast', typedMockOverDoubleCastRule, { { code: 'const repo = { find: sinon.stub() } as unknown as Repository;', }, + { code: 'const find = sinon.stub(); const repo = { find } as unknown as Repository;' }, + // A hoisted value that is not a mock: plain data, a parameter, an import. + { code: "const name = 'ada'; const input = { name } as unknown as CreateUserDto;" }, + { code: 'function build(get) { return { get } as unknown as ConfigService; }' }, + { + code: "import { get } from './doubles'; const config = { get } as unknown as ConfigService;", + }, + // A property key or a member name that matches a hoisted mock is not a reference to it. + { code: 'const fn = vi.fn(); const input = { fn: 1 } as unknown as CreateUserDto;' }, + { code: 'const fn = vi.fn(); const input = { a: other.fn } as unknown as CreateUserDto;' }, + // A name destructured out of a mock call is not the mock itself. + { code: 'const { fn } = vi.fn(); const input = { fn } as unknown as CreateUserDto;' }, ], invalid: [ // Settly shape: a NestJS ConfigService double. @@ -83,12 +95,65 @@ ruleTester.run('typed-mock-over-double-cast', typedMockOverDoubleCastRule, { filename: 'src/mail.service.spec.ts', errors: [{ messageId: 'doubleCastMock' }], }, + // A mock created first and placed in the object by name, shorthand or not. + { + code: ` + const fn = vi.fn(); + const svc = { fn } as unknown as Service; + `, + errors: [{ messageId: 'doubleCastMock', data: { target: 'Service' }, line: 3 }], + }, + { + code: 'const fn = jest.fn(); const svc = { run: fn } as unknown as MailService;', + errors: [{ messageId: 'doubleCastMock', data: { target: 'MailService' } }], + }, + // A hoisted chain, also through a cast on the initialiser. + { + code: "const getRawInput = jest.fn().mockResolvedValue({ foo: 'bar' }); const mw = { getRawInput } as unknown as MwOpts;", + errors: [{ messageId: 'doubleCastMock' }], + }, + { + code: 'const get = vi.fn() as Mock; const config = { get } as unknown as ConfigService;', + errors: [{ messageId: 'doubleCastMock' }], + }, + // A hoisted mock nested in an inner object. + { + code: 'const setHeader = jest.fn(); const opts = { ctx: { res: { setHeader } } } as unknown as OnErrorOptions;', + errors: [{ messageId: 'doubleCastMock' }], + }, + // A binding declared first and assigned its mock in a hook. + { + code: ` + let get: jest.Mock; + let config: ConfigService; + beforeEach(() => { + get = jest.fn(); + config = { get } as unknown as ConfigService; + }); + `, + errors: [{ messageId: 'doubleCastMock', line: 6 }], + }, + // A module-level mock used inside a function. + { + code: ` + const send = vi.fn(); + function makeMailer() { + return { send } as unknown as Mailer; + } + `, + errors: [{ messageId: 'doubleCastMock', line: 4 }], + }, // A custom factory list. { code: 'const repo = { find: sinon.stub() } as unknown as Repository;', options: [{ mockFactories: ['sinon.stub'] }], errors: [{ messageId: 'doubleCastMock' }], }, + { + code: 'const find = sinon.stub(); const repo = { find } as unknown as Repository;', + options: [{ mockFactories: ['sinon.stub'] }], + errors: [{ messageId: 'doubleCastMock' }], + }, // An allow glob for one type does not cover another. { code: 'const svc = { run: jest.fn() } as unknown as MailService;',