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

.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.

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<unknown>) {
 ```
 
 ```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
 

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', {

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(<TurnstileField form={form} />);
   expect(container).not.toBeEmptyDOMElement();
@@ -84,6 +94,11 @@ it('renders the card', () => {
   const { container } = render(<Card title="Q3" />);
   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(<Card title="Q3" />);
   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.
 

packages/eslint-plugin-code-quality/docs/rules/typed-mock-over-double-cast.md

@@ -52,13 +52,42 @@ const host: jest.Mocked<Pick<ArgumentsHost, 'switchToHttp'>> = {
 };
 ```
 
+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<Pick<Mailer, 'send'>> = { send };
+
+let get: jest.Mock;
+beforeEach(() => {
+  get = jest.fn().mockReturnValue('smtp://localhost');
+  config = { get } satisfies Partial<ConfigService>;
+});
+```
+
 ## What it does not flag
 
 - A single cast (`as jest.Mocked<Pick<T, 'get'>>`): TypeScript still checks that the two types
   overlap.
 - 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

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<RuleOptions, MessageId
       }
     }
 
+    /**
+     * The initialiser of the variable `identifier` names, when the variable is
+     * declared once with one and never written again. Null for a binding with
+     * no value to read: an import, a parameter, a class, a reassigned `let`.
+     */
+    function constantInit(identifier: TSESTree.Identifier): TSESTree.Expression | null {
+      const scope = context.sourceCode.getScope(identifier);
+      const variable = ASTUtils.findVariable(scope, identifier);
+      if (variable?.defs.length !== 1) {
+        return null;
+      }
+      const declarator = variable.defs[0]?.node;
+      if (
+        declarator?.type !== AST_NODE_TYPES.VariableDeclarator ||
+        declarator.id.type !== AST_NODE_TYPES.Identifier ||
+        variable.references.some((reference) => 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.<matcher>(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<RuleOptions, MessageId
       if (argument === undefined) {
         return 'bareThrow';
       }
-      if (isMessageArgument(argument)) {
+      if (isMessageCheck(argument)) {
         return allowMessageOnly ? null : 'messageOnlyThrow';
       }
       if (argument.type === AST_NODE_TYPES.NewExpression && !trustErrorInstances) {