diff --git a/.env.example b/.env.example index b231263faf..f2408d2954 100644 --- a/.env.example +++ b/.env.example @@ -68,32 +68,32 @@ GITTENSORY_REVIEW_ENRICHMENT=false # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta,callerImpact +# conflictMarker,debugLeftover,focusedTest,sizeSmell,floatingPromise,deepNesting,errorSwallow +# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# revertRecurrence,coverageDelta,callerImpact # # Profile defaults: # fast: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,secretLog,typosquat,iacMisconfig,nativeBuild # testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber,conflictMarker -# debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y,i18n,apiBreak -# deprecatedDep +# debugLeftover,focusedTest,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y +# i18n,apiBreak,deprecatedDep # balanced (default): dependency,dependencyDiff,lockfileDrift,secret,license,installScript # heavyDependency,hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight # typosquat,commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication # churnHotspot,blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch # commitHygiene,pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology -# todoMarker,magicNumber,conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting -# errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak -# deprecatedDep,revertRecurrence,coverageDelta,callerImpact +# todoMarker,magicNumber,conflictMarker,debugLeftover,focusedTest,sizeSmell,floatingPromise +# deepNesting,errorSwallow,unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint +# apiBreak,deprecatedDep,revertRecurrence,coverageDelta,callerImpact # deep: dependency,dependencyDiff,lockfileDrift,secret,license,installScript,heavyDependency # hardcodedUrl,actionPin,eol,redos,provenance,codeowners,secretLog,assetWeight,typosquat # commitSignature,iacMisconfig,nativeBuild,history,docCommentDrift,duplication,churnHotspot # blameLink,approvalIntegrity,ciCheckSignals,undocumentedExport,staleBranch,commitHygiene # pendingReviewRequests,testRatio,migrationSafety,looseRange,terminology,todoMarker,magicNumber -# conflictMarker,debugLeftover,sizeSmell,floatingPromise,deepNesting,errorSwallow,unsafeAny,a11y -# i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep,revertRecurrence -# coverageDelta,callerImpact +# conflictMarker,debugLeftover,focusedTest,sizeSmell,floatingPromise,deepNesting,errorSwallow +# unsafeAny,a11y,i18n,unusedExport,exhaustiveness,flakyTest,commitLint,apiBreak,deprecatedDep +# revertRecurrence,coverageDelta,callerImpact # END GENERATED REES ANALYZERS # Submitter-reputation spend control (internal-only): downgrades new/burst/low-rep diff --git a/apps/gittensory-ui/src/lib/rees-analyzers.ts b/apps/gittensory-ui/src/lib/rees-analyzers.ts index feca564a90..dad0f2822b 100644 --- a/apps/gittensory-ui/src/lib/rees-analyzers.ts +++ b/apps/gittensory-ui/src/lib/rees-analyzers.ts @@ -956,6 +956,28 @@ export const REES_ANALYZERS = [ "Distinct from the secrets-in-logs analyzer: this catches plain debug noise regardless of payload. String literals are stripped before matching.", }, }, + { + name: "focusedTest", + title: "Focused tests", + category: "quality", + cost: "local", + defaultEnabled: true, + profiles: ["fast", "balanced", "deep"], + requires: ["files"], + limits: { + maxFindings: 25, + maxLineChars: 2000, + }, + docs: { + summary: + "Flags a focused test a PR adds in a test file — `describe.only` / `it.only` / `test.only` — which silently skips every other test in that file.", + looksAt: "Added lines in changed test files.", + reports: "File and line of each focused-test call.", + network: "Pure local analyzer. No external network call.", + notes: + "The test-file counterpart of the debug-leftover analyzer. String literals are stripped before matching, so a `.only` inside a string is not flagged.", + }, + }, { name: "sizeSmell", title: "Size smells", diff --git a/review-enrichment/analyzer-metadata.json b/review-enrichment/analyzer-metadata.json index 6482e449c9..457ab1d0de 100644 --- a/review-enrichment/analyzer-metadata.json +++ b/review-enrichment/analyzer-metadata.json @@ -1079,6 +1079,32 @@ "notes": "Distinct from the secrets-in-logs analyzer: this catches plain debug noise regardless of payload. String literals are stripped before matching." } }, + { + "name": "focusedTest", + "title": "Focused tests", + "category": "quality", + "cost": "local", + "defaultEnabled": true, + "profiles": [ + "fast", + "balanced", + "deep" + ], + "requires": [ + "files" + ], + "limits": { + "maxFindings": 25, + "maxLineChars": 2000 + }, + "docs": { + "summary": "Flags a focused test a PR adds in a test file — `describe.only` / `it.only` / `test.only` — which silently skips every other test in that file.", + "looksAt": "Added lines in changed test files.", + "reports": "File and line of each focused-test call.", + "network": "Pure local analyzer. No external network call.", + "notes": "The test-file counterpart of the debug-leftover analyzer. String literals are stripped before matching, so a `.only` inside a string is not flagged." + } + }, { "name": "sizeSmell", "title": "Size smells", diff --git a/review-enrichment/src/analyzers/focused-test.ts b/review-enrichment/src/analyzers/focused-test.ts new file mode 100644 index 0000000000..09a29e542a --- /dev/null +++ b/review-enrichment/src/analyzers/focused-test.ts @@ -0,0 +1,86 @@ +// Focused-test analyzer (part of #1499 quality-signal family). Flags a focused test a PR adds in a test file — +// `describe.only` / `it.only` / `test.only` (and `context`/`suite`/`specify`) — which silently skips every OTHER +// test in that file, so CI can stay green while most of the suite no longer runs. It is the test-file counterpart +// of the debug-leftover analyzer: that one scans NON-test source, this one scans test files only. Pure compute, +// no network. String-literal content is stripped before matching so a `"it.only"` inside a string is not flagged. +// Line-cited via hunk headers, mirroring the sibling local analyzers. +import type { FocusedTestFinding, EnrichRequest } from "../types.js"; +import { codeOnly } from "./secret-log.js"; +import { isTestPath } from "./test-ratio.js"; + +const MAX_FINDINGS = 25; +const MAX_LINE_CHARS = 2000; + +// A `.only` chained onto a known test-block function — the cross-framework (Jest/Vitest/Mocha/Jasmine) way to +// focus one test/suite and skip the rest of the file. Restricted to those function names so an unrelated +// `stream.only(` never matches. +const FOCUSED_TEST_RE = + /\b(?:describe|context|suite|it|test|specify)\s*\.\s*only\s*\(/; + +/** Detect a focused-test (`.only`) call in one added line, or null. Pure. */ +export function detectFocusedTest(line: string): FocusedTestFinding["kind"] | null { + return FOCUSED_TEST_RE.test(codeOnly(line)) ? "only" : null; +} + +type ScanLimits = { + maxFindings?: number; + signal?: AbortSignal; +}; + +/** Scan one TEST file patch's added lines for focused-test calls, line-cited via hunk headers. Non-test files are + * skipped (a `.only` there is not a focused test). Pure. */ +export function scanPatchForFocusedTest( + path: string, + patch: string, + limits: ScanLimits = {}, +): FocusedTestFinding[] { + const maxFindings = limits.maxFindings ?? MAX_FINDINGS; + if (maxFindings <= 0 || !isTestPath(path)) return []; + const findings: FocusedTestFinding[] = []; + let newLine = 0; + let inHunk = false; + for (const line of patch.split("\n")) { + if (limits.signal?.aborted) throw new Error("analyzer_aborted"); + const hunk = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (hunk) { + newLine = Number(hunk[1]); + inHunk = true; + continue; + } + if (!inHunk) continue; + if (line.startsWith("+")) { + const body = line.slice(1); + if (body.length <= MAX_LINE_CHARS) { + const kind = detectFocusedTest(body); + if (kind) { + findings.push({ file: path, line: newLine, kind }); + if (findings.length >= maxFindings) return findings; + } + } + newLine++; + } else if (!line.startsWith("-") && !line.startsWith("\\")) { + newLine++; + } + } + return findings; +} + +/** Analyzer entrypoint: scan every changed test file's added lines for focused-test calls. */ +export async function scanFocusedTest( + req: EnrichRequest, + signal?: AbortSignal, +): Promise { + const findings: FocusedTestFinding[] = []; + for (const file of req.files ?? []) { + if (signal?.aborted) throw new Error("analyzer_aborted"); + if (!file.patch) continue; + for (const finding of scanPatchForFocusedTest(file.path, file.patch, { + maxFindings: MAX_FINDINGS - findings.length, + signal, + })) { + findings.push(finding); + if (findings.length >= MAX_FINDINGS) return findings; + } + } + return findings; +} diff --git a/review-enrichment/src/analyzers/registry.ts b/review-enrichment/src/analyzers/registry.ts index 152b6fd202..f446769c83 100644 --- a/review-enrichment/src/analyzers/registry.ts +++ b/review-enrichment/src/analyzers/registry.ts @@ -32,6 +32,7 @@ import { scanLooseRanges } from "./loose-range.js"; import { scanMagicNumbers } from "./magic-number.js"; import { scanConflictMarkers } from "./conflict-marker.js"; import { scanDebugLeftover } from "./debug-leftover.js"; +import { scanFocusedTest } from "./focused-test.js"; import { scanDeepNesting } from "./deep-nesting.js"; import { scanI18nRegression } from "./i18n-regression.js"; import { scanErrorSwallow } from "./error-swallow.js"; @@ -1028,6 +1029,35 @@ export const ANALYZER_DESCRIPTORS = [ }, run: (req, { signal }) => scanDebugLeftover(req, signal), }), + descriptor({ + name: "focusedTest", + title: "Focused tests", + category: "quality", + cost: "local", + defaultEnabled: true, + requires: ["files"], + limits: { maxFindings: 25, maxLineChars: 2000 }, + docs: { + summary: + "Flags a focused test a PR adds in a test file — `describe.only` / `it.only` / `test.only` — which silently skips every other test in that file.", + looksAt: "Added lines in changed test files.", + reports: "File and line of each focused-test call.", + network: "Pure local analyzer. No external network call.", + notes: + "The test-file counterpart of the debug-leftover analyzer. String literals are stripped before matching, so a `.only` inside a string is not flagged.", + }, + render: (findings, helpers) => { + if (!findings.length) return []; + const lines = ["### Focused tests (`.only` added by this PR — skips the rest of the file)"]; + for (const item of findings) { + lines.push( + `- ${helpers.safeCodeSpan(`${item.file}:${item.line}`)} — ${helpers.safeCodeSpan(item.kind)}`, + ); + } + return lines; + }, + run: (req, { signal }) => scanFocusedTest(req, signal), + }), descriptor({ name: "sizeSmell", title: "Size smells", diff --git a/review-enrichment/src/render.ts b/review-enrichment/src/render.ts index 5cd5c6c97d..aeee2e02bd 100644 --- a/review-enrichment/src/render.ts +++ b/review-enrichment/src/render.ts @@ -486,6 +486,7 @@ export function renderBrief( lines.push(...renderDescriptorSection("magicNumber", findings.magicNumber)); lines.push(...renderDescriptorSection("conflictMarker", findings.conflictMarker)); lines.push(...renderDescriptorSection("debugLeftover", findings.debugLeftover)); + lines.push(...renderDescriptorSection("focusedTest", findings.focusedTest)); lines.push(...renderDescriptorSection("sizeSmell", findings.sizeSmell)); lines.push(...renderDescriptorSection("floatingPromise", findings.floatingPromise)); lines.push(...renderDescriptorSection("deepNesting", findings.deepNesting)); diff --git a/review-enrichment/src/types.ts b/review-enrichment/src/types.ts index 5891e4ce13..f6469e887d 100644 --- a/review-enrichment/src/types.ts +++ b/review-enrichment/src/types.ts @@ -511,6 +511,12 @@ export interface DebugLeftoverFinding { kind: "debugger" | "console" | "print"; } +export interface FocusedTestFinding { + file: string; + line: number; + kind: "only"; +} + /** Maintainability size smell from patch structure (#2019, part of #1499). * Reports estimated file length or added function body span — never source content. */ export interface SizeSmellFinding { @@ -693,6 +699,7 @@ export interface BriefFindings { magicNumber?: MagicNumberFinding[]; conflictMarker?: ConflictMarkerFinding[]; debugLeftover?: DebugLeftoverFinding[]; + focusedTest?: FocusedTestFinding[]; sizeSmell?: SizeSmellFinding[]; floatingPromise?: FloatingPromiseFinding[]; deepNesting?: DeepNestingFinding[]; diff --git a/review-enrichment/test/analyzer-registry.test.ts b/review-enrichment/test/analyzer-registry.test.ts index 1042b62161..822edd39a5 100644 --- a/review-enrichment/test/analyzer-registry.test.ts +++ b/review-enrichment/test/analyzer-registry.test.ts @@ -48,6 +48,7 @@ const EXPECTED_ANALYZERS = [ "magicNumber", "conflictMarker", "debugLeftover", + "focusedTest", "sizeSmell", "floatingPromise", "deepNesting", diff --git a/review-enrichment/test/focused-test.test.ts b/review-enrichment/test/focused-test.test.ts new file mode 100644 index 0000000000..ec24446a7f --- /dev/null +++ b/review-enrichment/test/focused-test.test.ts @@ -0,0 +1,100 @@ +// Units for the focused-test analyzer (part of #1499). Own file (not enrichment.test.ts) so concurrent analyzer +// PRs don't collide. No network — pure, stateless per-line detection. Runs against the compiled dist/. +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { + detectFocusedTest, + scanFocusedTest, + scanPatchForFocusedTest, +} from "../dist/analyzers/focused-test.js"; +import { renderBrief } from "../dist/render.js"; + +const patchOf = (lines: string[]) => + `@@ -1,0 +1,${lines.length} @@\n${lines.map((l) => `+${l}`).join("\n")}`; + +test("detectFocusedTest: recognizes .only on every test-block function", () => { + assert.equal(detectFocusedTest("describe.only('a', () => {"), "only"); + assert.equal(detectFocusedTest(" it.only('does x', () => {"), "only"); + assert.equal(detectFocusedTest("test.only('y', () => {"), "only"); + assert.equal(detectFocusedTest("context.only('z', () => {"), "only"); + assert.equal(detectFocusedTest("suite.only('s', () => {"), "only"); + assert.equal(detectFocusedTest("specify.only('w', () => {"), "only"); + // whitespace tolerance around the dot and call paren + assert.equal(detectFocusedTest("it . only ( 'spaced', () => {"), "only"); +}); + +test("detectFocusedTest: ordinary tests and non-test .only calls are not flagged", () => { + assert.equal(detectFocusedTest("it('does x', () => {"), null); + assert.equal(detectFocusedTest("describe('suite', () => {"), null); + // `.only(` on something that is not a test-block function + assert.equal(detectFocusedTest("stream.only(handler)"), null); + // a test-fn name embedded in a longer identifier must not match + assert.equal(detectFocusedTest("submit.only(form)"), null); + // `.only` with no call paren is not a focused-test call + assert.equal(detectFocusedTest("const flag = it.only"), null); +}); + +test("detectFocusedTest: a .only inside a string literal is not flagged", () => { + assert.equal(detectFocusedTest('const s = "it.only(\'x\')";'), null); +}); + +test("scanPatchForFocusedTest: only scans test files", () => { + const patch = patchOf(["it.only('x', () => {});"]); + assert.deepEqual(scanPatchForFocusedTest("src/widget.test.ts", patch), [ + { file: "src/widget.test.ts", line: 1, kind: "only" }, + ]); + // a .only in a non-test source file is not a focused test → skipped + assert.deepEqual(scanPatchForFocusedTest("src/widget.ts", patch), []); +}); + +test("scanPatchForFocusedTest: cites the correct new-file line across context and removed lines", () => { + const patch = [ + "@@ -1,2 +1,4 @@", + " import { test } from 'node:test';", // context → line 1 + "+// setup", // added → line 2 + "-const old = 1;", // removed → no new line + "+it.only('focused', () => {});", // added → line 3 + "+test('other', () => {});", // added → line 4 + ].join("\n"); + assert.deepEqual(scanPatchForFocusedTest("test/foo.spec.ts", patch), [ + { file: "test/foo.spec.ts", line: 3, kind: "only" }, + ]); +}); + +test("scanPatchForFocusedTest: respects the maxFindings cap", () => { + const patch = patchOf([ + "it.only('a', () => {});", + "test.only('b', () => {});", + "describe.only('c', () => {});", + ]); + assert.equal(scanPatchForFocusedTest("a.test.ts", patch, { maxFindings: 2 }).length, 2); + assert.deepEqual(scanPatchForFocusedTest("a.test.ts", patch, { maxFindings: 0 }), []); +}); + +test("scanFocusedTest: scans every changed test file, skipping non-test files and files without a patch", async () => { + const findings = await scanFocusedTest({ + repoFullName: "o/r", + prNumber: 1, + files: [ + { path: "a.test.ts", patch: patchOf(["it.only('x', () => {});"]) }, + { path: "b.ts", patch: patchOf(["it.only('y', () => {});"]) }, // non-test → skipped + { path: "c.test.ts", patch: null }, // no patch → skipped + { path: "d.spec.ts", patch: patchOf(["describe.only('z', () => {});"]) }, + ], + }); + assert.deepEqual(findings, [ + { file: "a.test.ts", line: 1, kind: "only" }, + { file: "d.spec.ts", line: 1, kind: "only" }, + ]); +}); + +test("focusedTest renders a section only when there are findings", () => { + const { promptSection } = renderBrief({ + focusedTest: [{ file: "a.test.ts", line: 3, kind: "only" }], + }); + assert.match(promptSection, /Focused tests/); + assert.match(promptSection, /a\.test\.ts:3/); + + const { promptSection: empty } = renderBrief({ focusedTest: [] }); + assert.doesNotMatch(empty, /Focused tests/); +});