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) }), +);