From f458278e889338a39a54a067147041e893c2ae67 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 18 Oct 2026 22:46:00 +0200 Subject: [PATCH] ci(lint): a grown warning count names the file and line that grew it lint:warnings failed with the total and the files holding the most of the rule, which are old ones the baseline allows; the file that tipped the count was never named, so finding it meant linting by hand. When a rule grows, the ratchet now lints the files that differ from the base twice, as they are and as they were there, and lists every one holding more of the rule, line and message. The base is TREK_LINT_BASE (the CI fetches the pushed-over commit on a push and the target branch on a pull request) or origin/dev; without it the report stays as it was. --- .github/workflows/test.yml | 20 +++++ client/scripts/lib/eslint-warnings.mjs | 83 ++++++++++++++++++- .../tests/unit/scripts/eslint-ratchet.test.ts | 41 ++++++++- scripts/lib/eslint-warnings.mjs | 83 ++++++++++++++++++- scripts/lib/eslint-warnings.test.mjs | 25 +++++- server/scripts/eslint-ratchet.mjs | 6 +- 6 files changed, 244 insertions(+), 14 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1002c96ca..3d540db1f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -257,6 +257,15 @@ jobs: # migration that changes a column regenerates it in the same change. run: cd server && npm run check:db-types + - name: Fetch the base the warnings are compared with + # lint:warnings names the file behind a grown count by linting it as + # it is on the base: the pushed-over commit on a push, the target + # branch on a pull request. A miss only drops that hint. + run: >- + git fetch --no-tags --depth=1 origin + "+${{ github.event_name == 'push' && github.event.before || format('refs/heads/{0}', github.base_ref || 'dev') }}:refs/remotes/lint-base" + || true + - name: Lint # Errors fail, and the warnings per rule may not pass # scripts/eslint-baseline.json (src, tests and eslint-disable'd @@ -264,6 +273,7 @@ jobs: # program, which outgrows Node's default ~4 GB heap on the runner. env: NODE_OPTIONS: --max-old-space-size=8192 + TREK_LINT_BASE: refs/remotes/lint-base run: cd server && npm run lint:warnings - name: Formatting check @@ -395,6 +405,15 @@ jobs: # one included, has to be clean (scripts/strict-lint.mjs). run: cd client && npm run lint:strict + - name: Fetch the base the warnings are compared with + # lint:warnings names the file behind a grown count by linting it as + # it is on the base: the pushed-over commit on a push, the target + # branch on a pull request. A miss only drops that hint. + run: >- + git fetch --no-tags --depth=1 origin + "+${{ github.event_name == 'push' && github.event.before || format('refs/heads/{0}', github.base_ref || 'dev') }}:refs/remotes/lint-base" + || true + - name: Lint # Fails on any ESLint error, like lint:check did, and on any rule # whose warning count rises above scripts/eslint-baseline.json. @@ -402,6 +421,7 @@ jobs: # whole TypeScript program, so it gets the heap the server lint gets. env: NODE_OPTIONS: --max-old-space-size=8192 + TREK_LINT_BASE: refs/remotes/lint-base run: cd client && npm run lint:warnings - name: Page pattern check diff --git a/client/scripts/lib/eslint-warnings.mjs b/client/scripts/lib/eslint-warnings.mjs index 7a5617df0..d1fade0c8 100644 --- a/client/scripts/lib/eslint-warnings.mjs +++ b/client/scripts/lib/eslint-warnings.mjs @@ -2,6 +2,7 @@ * The tally and the comparison behind lint:warnings (scripts/eslint-ratchet.mjs). */ import { ESLint } from 'eslint'; +import { execFileSync } from 'node:child_process'; import { join } from 'node:path'; import ts from 'typescript'; import { @@ -137,6 +138,78 @@ export async function lintClient(root) { })); } +/** The ref new warnings are measured against: TREK_LINT_BASE (the CI sets it), else origin/dev. */ +const baseRef = () => process.env.TREK_LINT_BASE || 'origin/dev'; + +/** Git in the workspace: its output, or null when it fails (no repository, no such ref). */ +function gitIn(root, args) { + try { + return execFileSync('git', args, { + cwd: root, + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'ignore'], + maxBuffer: 1 << 28, + }); + } catch { + return null; + } +} + +/** The messages of one rule in one area of a single file's lint result. */ +function ofRule(results, rule, area) { + const result = results[0]; + if (!result) return []; + const pool = + area === 'suppressed' ? (result.suppressedMessages ?? []) : result.messages.filter((m) => m.severity === 1); + return pool.filter((m) => ruleOf(m) === rule); +} + +/** + * Where a grown rule's new warnings sit: every file among `keys` (the files + * holding the rule now) that differs from `ref` and holds more of the rule + * than it did there, each of its warnings of the rule as `file:line message`. + * The count alone names no file, and the files holding the most are old ones; + * this names the file that tipped it. Null when `ref` is not at hand (a clone + * without it), and the report then keeps to the files holding the most. + */ +export async function newWarningsOf({ root, rule, area, keys, lintText, ref = baseRef(), git = gitIn }) { + const changed = git(root, ['diff', '--name-only', '--relative', ref, '--', '.']); + if (changed === null) return null; + const differs = new Set(changed.split('\n').filter(Boolean)); + const found = await Promise.all( + keys + .filter((key) => differs.has(key)) + .map(async (key) => { + const path = join(root, key); + const now = ofRule(await lintText(readText(path), path), rule, area); + const before = git(root, ['show', `${ref}:./${key}`]); + const was = before === null ? [] : ofRule(await lintText(before, path), rule, area); + return now.length > was.length ? now.map((m) => `${key}:${m.line ?? 0} ${m.message}`) : []; + }) + ); + return found.flat(); +} + +/** Prints, under the grown counts, the files and lines behind each one (see newWarningsOf). */ +async function reportNew(grown, { root, files, lintText, error }) { + if (!grown.length || !lintText) return; + const found = await Promise.all( + grown.map(({ area, rule }) => newWarningsOf({ root, rule, area, keys: Object.keys(files[area][rule]), lintText })) + ); + grown.forEach(({ area, rule }, i) => { + const lines = found[i]; + if (!lines?.length) return; + error(`New ${rule} warning(s) in ${area}, against ${baseRef()}:`); + for (const line of lines) error(` ${line}`); + }); +} + +/** Lints one file's text with the client's own config, for newWarningsOf. */ +export function lintTextClient(root) { + const eslint = new ESLint({ cwd: root }); + return (text, filePath) => eslint.lintText(text, { filePath }); +} + const sum = (map) => Object.values(map).reduce((a, b) => a + b, 0); /** @@ -148,6 +221,7 @@ export async function check({ baselinePath = join(root, 'scripts/eslint-baseline.json'), update = false, lint = lintClient, + lintText = lint === lintClient ? lintTextClient : null, log = console.log, error = console.error, }) { @@ -160,13 +234,13 @@ export async function check({ } for (const line of errors) error(`FAIL ${line}`); - let grown = 0; + const grown = []; let stale = 0; for (const area of AREAS) { for (const [rule, n] of Object.entries(counts[area])) { const allowed = baseline[area][rule] ?? 0; if (n <= allowed) continue; - grown++; + grown.push({ area, rule }); const top = Object.entries(files[area][rule]) .sort(([, a], [, b]) => b - a) .slice(0, 5) @@ -182,7 +256,8 @@ export async function check({ }); stale += fallen.length; } - if (grown) { + await reportNew(grown, { root, files, lintText: lintText && lintText(root), error }); + if (grown.length) { error( 'Fix the new warning rather than silencing it: an eslint-disable comment is counted under "suppressed". ' + 'Run npx eslint to see each one.' @@ -191,5 +266,5 @@ export async function check({ log( `eslint: ${errors.length} error(s); warnings ${AREAS.map((area) => `${area} ${sum(counts[area])}/${sum(baseline[area])}`).join(', ')} (now/baseline)` ); - return errors.length || grown || stale ? 1 : 0; + return errors.length || grown.length || stale ? 1 : 0; } diff --git a/client/tests/unit/scripts/eslint-ratchet.test.ts b/client/tests/unit/scripts/eslint-ratchet.test.ts index aa9006a56..e8bce02dd 100644 --- a/client/tests/unit/scripts/eslint-ratchet.test.ts +++ b/client/tests/unit/scripts/eslint-ratchet.test.ts @@ -1,7 +1,15 @@ -import { readFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, expect, it } from 'vitest'; -import { areaCounts, check, inlineConfigsOf, lintClient, tally } from '../../../scripts/lib/eslint-warnings.mjs'; +import { + areaCounts, + check, + inlineConfigsOf, + lintClient, + newWarningsOf, + tally, +} from '../../../scripts/lib/eslint-warnings.mjs'; import { RatchetError } from '../../../scripts/lib/ratchet.mjs'; import { ratchetTree, type RatchetTree } from '../../helpers/ratchetFixture'; @@ -183,4 +191,33 @@ describe('lint:warnings', () => { expect(byKey['b.js'].messages.map((m) => m.ruleId)).toEqual(['no-empty']); expect(byKey['b.js'].inlineConfigs).toEqual([]); }); + + it('names the file and line behind a grown count, measured against the base, and nothing without one', async () => { + const root = mkdtempSync(join(tmpdir(), 'lint-new-')); + try { + mkdirSync(join(root, 'src')); + writeFileSync(join(root, 'src/new.ts'), 'two'); + writeFileSync(join(root, 'src/old.ts'), 'one'); + // new.ts held none of the rule on the base, old.ts already held its one + const git = (_root: string, args: string[]) => + args[0] === 'diff' ? 'src/new.ts\nsrc/old.ts\n' : args[1]?.endsWith('old.ts') ? 'one' : 'none'; + const counts: Record = { two: 2, one: 1, none: 0 }; + const lintText = async (text: string) => [ + { + messages: Array.from({ length: counts[text] ?? 0 }, (_, i) => ({ + ruleId: 'no-empty', + severity: 1, + message: `w${i}`, + line: i + 1, + })), + }, + ]; + const keys = ['src/new.ts', 'src/old.ts', 'src/untouched.ts']; + const lines = await newWarningsOf({ root, rule: 'no-empty', area: 'src', keys, lintText, ref: 'base', git }); + expect(lines).toEqual(['src/new.ts:1 w0', 'src/new.ts:2 w1']); + expect(await newWarningsOf({ root, rule: 'no-empty', area: 'src', keys, lintText, git: () => null })).toBeNull(); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); }); diff --git a/scripts/lib/eslint-warnings.mjs b/scripts/lib/eslint-warnings.mjs index a60b62970..6fffe159f 100644 --- a/scripts/lib/eslint-warnings.mjs +++ b/scripts/lib/eslint-warnings.mjs @@ -6,6 +6,7 @@ * own major and has to lint with the one its eslint.config.mjs was written * for. This file never imports eslint, so it resolves no version of its own. */ +import { execFileSync } from 'node:child_process'; import { join } from 'node:path'; import ts from 'typescript'; import { @@ -145,6 +146,77 @@ export function lintWith(ESLint) { }; } +/** The ref new warnings are measured against: TREK_LINT_BASE (the CI sets it), else origin/dev. */ +const baseRef = () => process.env.TREK_LINT_BASE || 'origin/dev'; + +/** Git in the workspace: its output, or null when it fails (no repository, no such ref). */ +function gitIn(root, args) { + try { + return execFileSync('git', args, { cwd: root, encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], maxBuffer: 1 << 28 }); + } catch { + return null; + } +} + +/** The messages of one rule in one area of a single file's lint result. */ +function ofRule(results, rule, area) { + const result = results[0]; + if (!result) return []; + const pool = + area === 'suppressed' ? (result.suppressedMessages ?? []) : result.messages.filter((m) => m.severity === 1); + return pool.filter((m) => ruleOf(m) === rule); +} + +/** + * Where a grown rule's new warnings sit: every file among `keys` (the files + * holding the rule now) that differs from `ref` and holds more of the rule + * than it did there, each of its warnings of the rule as `file:line message`. + * The count alone names no file, and the files holding the most are old ones; + * this names the file that tipped it. Null when `ref` is not at hand (a clone + * without it), and the report then keeps to the files holding the most. + */ +export async function newWarningsOf({ root, rule, area, keys, lintText, ref = baseRef(), git = gitIn }) { + const changed = git(root, ['diff', '--name-only', '--relative', ref, '--', '.']); + if (changed === null) return null; + const differs = new Set(changed.split('\n').filter(Boolean)); + const found = await Promise.all( + keys + .filter((key) => differs.has(key)) + .map(async (key) => { + const path = join(root, key); + const now = ofRule(await lintText(readText(path), path), rule, area); + const before = git(root, ['show', `${ref}:./${key}`]); + const was = before === null ? [] : ofRule(await lintText(before, path), rule, area); + return now.length > was.length ? now.map((m) => `${key}:${m.line ?? 0} ${m.message}`) : []; + }) + ); + return found.flat(); +} + +/** Prints, under the grown counts, the files and lines behind each one (see newWarningsOf). */ +async function reportNew(grown, { root, files, lintText, error }) { + if (!grown.length || !lintText) return; + const found = await Promise.all( + grown.map(({ area, rule }) => + newWarningsOf({ root, rule, area, keys: Object.keys(files[area][rule]), lintText }) + ) + ); + grown.forEach(({ area, rule }, i) => { + const lines = found[i]; + if (!lines?.length) return; + error(`New ${rule} warning(s) in ${area}, against ${baseRef()}:`); + for (const line of lines) error(` ${line}`); + }); +} + +/** A one-file lint for newWarningsOf, with the ESLint the workspace imports (see lintWith). */ +export function lintTextWith(ESLint) { + return (root) => { + const eslint = new ESLint({ cwd: root }); + return (text, filePath) => eslint.lintText(text, { filePath }); + }; +} + const sum = (map) => Object.values(map).reduce((a, b) => a + b, 0); /** @@ -156,6 +228,8 @@ export async function check({ baselinePath = join(root, 'scripts/eslint-baseline.json'), update = false, lint, + /** lintTextWith(ESLint): names the files behind a grown count; left out, only the counts are told. */ + lintText = null, log = console.log, error = console.error, }) { @@ -169,13 +243,13 @@ export async function check({ } for (const line of errors) error(`FAIL ${line}`); - let grown = 0; + const grown = []; let stale = 0; for (const area of AREAS) { for (const [rule, n] of Object.entries(counts[area])) { const allowed = baseline[area][rule] ?? 0; if (n <= allowed) continue; - grown++; + grown.push({ area, rule }); const top = Object.entries(files[area][rule]) .sort(([, a], [, b]) => b - a) .slice(0, 5) @@ -191,7 +265,8 @@ export async function check({ }); stale += fallen.length; } - if (grown) { + await reportNew(grown, { root, files, lintText: lintText && lintText(root), error }); + if (grown.length) { error( 'Fix the new warning rather than silencing it: an eslint-disable comment is counted under "suppressed". ' + 'Run npx eslint to see each one.' @@ -200,5 +275,5 @@ export async function check({ log( `eslint: ${errors.length} error(s); warnings ${AREAS.map((area) => `${area} ${sum(counts[area])}/${sum(baseline[area])}`).join(', ')} (now/baseline)` ); - return errors.length || grown || stale ? 1 : 0; + return errors.length || grown.length || stale ? 1 : 0; } diff --git a/scripts/lib/eslint-warnings.test.mjs b/scripts/lib/eslint-warnings.test.mjs index dcdc555f6..9067da28e 100644 --- a/scripts/lib/eslint-warnings.test.mjs +++ b/scripts/lib/eslint-warnings.test.mjs @@ -1,9 +1,10 @@ import { ESLint } from 'eslint'; import assert from 'node:assert/strict'; -import { readFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, describe, it } from 'node:test'; -import { areaCounts, check, inlineConfigsOf, lintWith, tally } from './eslint-warnings.mjs'; +import { areaCounts, check, inlineConfigsOf, lintWith, newWarningsOf, tally } from './eslint-warnings.mjs'; import { RatchetError } from './ratchet.mjs'; import { ratchetTree } from './tree-fixture.mjs'; @@ -92,6 +93,26 @@ describe('scripts/lib/eslint-warnings', () => { assert.match(out, /Fix the new warning rather than silencing it/); }); + it('names the file and line behind a grown count, measured against the base, and nothing without one', async () => { + const root = mkdtempSync(join(tmpdir(), 'lint-new-')); + try { + mkdirSync(join(root, 'src')); + writeFileSync(join(root, 'src/new.ts'), 'two'); + writeFileSync(join(root, 'src/old.ts'), 'one'); + // new.ts held none of the rule on the base, old.ts already held its one + const git = (_root, args) => (args[0] === 'diff' ? 'src/new.ts\nsrc/old.ts\n' : args[1].endsWith('old.ts') ? 'one' : 'none'); + const lintText = async (text) => [ + { messages: Array.from({ length: { two: 2, one: 1, none: 0 }[text] }, (_, i) => ({ ...warn('no-empty', `w${i}`), line: i + 1 })) }, + ]; + const keys = ['src/new.ts', 'src/old.ts', 'src/untouched.ts']; + const lines = await newWarningsOf({ root, rule: 'no-empty', area: 'src', keys, lintText, ref: 'base', git }); + assert.deepEqual(lines, ['src/new.ts:1 w0', 'src/new.ts:2 w1']); + assert.equal(await newWarningsOf({ root, rule: 'no-empty', area: 'src', keys, lintText, git: () => null }), null); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); + it('counts a message an eslint-disable comment silenced', async () => { assert.equal(await run(empty, (root) => [result(root, 'src/a.ts', [], [warn('no-console')])]), 1); assert.match(tree.error.join('\n'), /suppressed: 1 no-console warning/); diff --git a/server/scripts/eslint-ratchet.mjs b/server/scripts/eslint-ratchet.mjs index d3b74c85f..91081a169 100644 --- a/server/scripts/eslint-ratchet.mjs +++ b/server/scripts/eslint-ratchet.mjs @@ -23,11 +23,13 @@ * ESLint comes from this workspace, so the server lints with the major its * config was written for. */ -import { check, lintWith } from '../../scripts/lib/eslint-warnings.mjs'; +import { check, lintTextWith, lintWith } from '../../scripts/lib/eslint-warnings.mjs'; import { runCli } from '../../scripts/lib/ratchet.mjs'; import { ESLint } from 'eslint'; import { fileURLToPath } from 'node:url'; const root = fileURLToPath(new URL('..', import.meta.url)); -await runCli('lint:warnings', (args) => check({ root, update: args.includes('--update'), lint: lintWith(ESLint) })); +await runCli('lint:warnings', (args) => + check({ root, update: args.includes('--update'), lint: lintWith(ESLint), lintText: lintTextWith(ESLint) }), +);