diff --git a/.fallowrc.json b/.fallowrc.json index 575941efea..8e91a99fed 100644 --- a/.fallowrc.json +++ b/.fallowrc.json @@ -627,6 +627,9 @@ "deadCodeBaseline": "fallow-baselines/dead-code.json", "healthBaseline": "fallow-baselines/health.json" }, + // CRAP needs measured coverage: estimated from imports it disagrees with measured scores on most + // findings. fallow-crap.json re-enables it for the CI Coverage job, which supplies the coverage map. + "health": { "maxCrap": 0 }, "includeEntryExports": true, "ignoreExportsUsedInFile": { "type": true, diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7691398f68..a092ecb9de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -298,6 +298,10 @@ jobs: uses: ./.github/actions/run-gate with: { gate: coverage-model } + - name: Test coverage-based CRAP report + uses: ./.github/actions/run-gate + with: { gate: coverage-crap-model } + - name: Run coverage id: run-coverage env: @@ -331,6 +335,29 @@ jobs: --base ${{ github.event.pull_request.base.sha }} + # Report-only: scores the functions this PR touches with the coverage map the coverage step + # wrote (fallow-crap.json), as a job summary and warning annotations. It never fails the job: + # a tooling error marks only this step. `!cancelled()` keeps it running after a failed + # changed-line gate. + - name: Report coverage-based CRAP + if: ${{ !cancelled() && steps.run-coverage.outcome == 'success' && github.event_name == 'pull_request' }} + continue-on-error: true + uses: ./.github/actions/run-gate + with: + gate: coverage-crap + args: | + --base + ${{ github.event.pull_request.base.sha }} + + # On main, the same report covers every function: the backlog of untested complex code. + - name: Report coverage-based CRAP for the whole repository + if: ${{ !cancelled() && steps.run-coverage.outcome == 'success' && github.event_name == 'push' }} + continue-on-error: true + uses: ./.github/actions/run-gate + with: + gate: coverage-crap + args: --all + integration: name: Integration Tests runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d3285c464e..5a8ddbe653 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -201,6 +201,9 @@ Useful direct entry points: - `pnpm test:integration` — Node and provider-backed integration suites - `pnpm perf --platform ios` or `pnpm perf --platform android` — device performance harness - `pnpm check:fallow --base origin/main` — changed-code quality gate +- `pnpm check:coverage-crap --base origin/main` — CRAP from measured coverage for the functions a + change touches (run `pnpm test:coverage` first; `--all` scans every function). Both report scores + at or above 30. `check:fallow` leaves CRAP off because it has no coverage. - `pnpm fallow:all` — full-tree audit, including grandfathered baseline findings - `pnpm fallow:baseline` — intentionally regenerate both reviewed Fallow baselines diff --git a/fallow-baselines/health.json b/fallow-baselines/health.json index 247f45bd3c..5ce01a8576 100644 --- a/fallow-baselines/health.json +++ b/fallow-baselines/health.json @@ -5,66 +5,19 @@ "count": 1 } }, - "src/__tests__/cli-config.test.ts": { - "crap_high": { - "count": 1 - }, - "crap_moderate": { - "count": 1 - } - }, - "src/__tests__/client-metro-companion.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/__tests__/runtime-public.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/commands/schema/cli-config.ts": { - "crap_moderate": { - "count": 1 - } - }, "src/commands/schema/option-schema.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 1 } }, "src/cli.ts": { "complexity_critical": { "count": 1 - }, - "crap_critical": { - "count": 1 } }, "src/cli/auth-session.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 1 - } - }, - "src/cli/commands/connection-runtime.ts": { - "crap_moderate": { - "count": 2 - } - }, - "src/cli/commands/connection.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/cli/commands/react-devtools.ts": { - "crap_moderate": { - "count": 1 } }, "src/cli/parser/args.ts": { @@ -73,46 +26,21 @@ }, "complexity_moderate": { "count": 1 - }, - "crap_critical": { - "count": 1 } }, "src/client/client-companion-tunnel-worker.ts": { "complexity_high": { "count": 1 - }, - "crap_moderate": { - "count": 3 - } - }, - "src/client/client-companion-tunnel.ts": { - "crap_high": { - "count": 2 - }, - "crap_moderate": { - "count": 1 } }, "src/core/dispatch.ts": { "complexity_critical": { "count": 1 - }, - "crap_critical": { - "count": 1 - } - }, - "src/daemon/__tests__/network-log.test.ts": { - "crap_moderate": { - "count": 1 } }, "src/daemon-client/daemon-client-lifecycle.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 } }, "src/daemon-client/daemon-client-metadata.ts": { @@ -120,11 +48,6 @@ "count": 1 } }, - "src/daemon-client/daemon-client-rpc.ts": { - "crap_moderate": { - "count": 1 - } - }, "src/daemon-client/daemon-client.ts": { "complexity_moderate": { "count": 1 @@ -133,91 +56,16 @@ "src/daemon/device/device-ready.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 - } - }, - "src/daemon/interaction/internal/__tests__/interaction-touch.test.ts": { - "crap_moderate": { - "count": 2 - } - }, - "src/daemon/interaction/internal/__tests__/interaction-touch-press.test.ts": { - "crap_moderate": { - "count": 3 - } - }, - "src/daemon/interaction/internal/__tests__/interaction-touch-press-admission.test.ts": { - "crap_moderate": { - "count": 2 - } - }, - "src/daemon/interaction/internal/__tests__/interaction-touch-response.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/handlers/__tests__/record-trace.test.ts": { - "crap_moderate": { - "count": 1 } }, "src/daemon/handlers/__tests__/replay-heal.test.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 - } - }, - "src/daemon/handlers/__tests__/session-appstate-input.test.ts": { - "crap_high": { - "count": 3 - }, - "crap_moderate": { - "count": 5 - } - }, - "src/daemon/handlers/__tests__/session-close-shutdown.test.ts": { - "crap_moderate": { - "count": 2 - } - }, - "src/daemon/session-lifecycle/internal/__tests__/session-devices-batch-runtime.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/session-observability/internal/__tests__/session-logs.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/session-lifecycle/internal/__tests__/session-open-runtime.test.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/__tests__/replay-suite/session-test-runner.test.ts": { - "crap_high": { - "count": 1 - }, - "crap_moderate": { - "count": 1 } }, "src/daemon/interaction/internal/find.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/interaction/internal/interaction-touch-reference-frame.ts": { - "crap_moderate": { - "count": 1 } }, "src/daemon/handlers/record-trace.ts": { @@ -230,37 +78,9 @@ "count": 1 } }, - "src/daemon/session-observability/internal/session-observability.ts": { - "crap_moderate": { - "count": 2 - } - }, - "src/daemon/session-lifecycle/internal/session-open-surface.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/session-lifecycle/internal/session-open.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/daemon/handlers/session-perf.ts": { - "crap_high": { - "count": 1 - } - }, - "src/daemon/session-runtime.ts": { - "crap_moderate": { - "count": 1 - } - }, "src/daemon/handlers/session-state.ts": { "complexity_critical": { "count": 2 - }, - "crap_critical": { - "count": 2 } }, "src/daemon/handlers/session-test-artifacts.ts": { @@ -273,11 +93,6 @@ "count": 1 } }, - "src/daemon/install-source-resolution.ts": { - "crap_moderate": { - "count": 1 - } - }, "src/daemon/recording-gestures.ts": { "complexity_moderate": { "count": 1 @@ -286,20 +101,11 @@ "src/daemon/runtime-hints.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 1 } }, "src/daemon/server/http-server.ts": { "complexity_critical": { "count": 1 - }, - "crap_critical": { - "count": 1 - }, - "crap_moderate": { - "count": 1 } }, "src/daemon/session-selector.ts": { @@ -312,53 +118,14 @@ "count": 1 } }, - "src/metro/client-metro.ts": { - "crap_high": { - "count": 1 - }, - "crap_moderate": { - "count": 2 - } - }, - "packages/platform-android/src/app-parsers.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/platform-android/src/manifest.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/platform-android/src/ui-hierarchy.ts": { - "crap_moderate": { - "count": 2 - } - }, - "packages/platform-apple/src/core/app-resolution.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/platform-apple/src/core/app-settings.ts": { - "crap_moderate": { - "count": 1 - } - }, "packages/platform-apple/src/core/perf.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 } }, "packages/platform-apple/src/core/screenshot-status-bar.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 2 } }, "packages/platform-apple/src/core/screenshot.ts": { @@ -366,22 +133,9 @@ "count": 1 } }, - "packages/platform-apple/src/os/macos/helper.ts": { - "crap_high": { - "count": 1 - } - }, "packages/provision-kit/src/boot-diagnostics.ts": { "complexity_critical": { "count": 1 - }, - "crap_critical": { - "count": 1 - } - }, - "packages/platform-linux/src/atspi-bridge.ts": { - "crap_moderate": { - "count": 1 } }, "packages/platform-linux/src/input-actions.ts": { @@ -389,32 +143,9 @@ "count": 1 } }, - "src/remote/daemon-artifacts.ts": { - "crap_moderate": { - "count": 2 - } - }, "src/remote/remote-config-core.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 - } - }, - "src/remote/upload-client.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/screenshot-diff/__tests__/screenshot-diff.test.ts": { - "crap_high": { - "count": 1 - } - }, - "src/screenshot-diff/screenshot-diff-non-text.ts": { - "crap_high": { - "count": 1 } }, "src/screenshot-diff/screenshot-diff.ts": { @@ -427,120 +158,29 @@ "count": 1 } }, - "packages/selectors/src/internal/match.ts": { - "crap_high": { - "count": 1 - } - }, "packages/selectors/src/internal/parse.ts": { "complexity_moderate": { "count": 1 - }, - "crap_moderate": { - "count": 2 - } - }, - "packages/selectors/src/internal/predicates.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/snapshot/mobile-snapshot-semantics.ts": { - "crap_moderate": { - "count": 2 - } - }, - "packages/capture-kit/src/snapshot/snapshot-lines.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/commands/output/snapshot.ts": { - "crap_moderate": { - "count": 1 - } - }, - "src/commands/output/diff.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/capture-kit/src/snapshot/scroll-edge-state.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/capture-kit/src/snapshot/snapshot-presentation/text-surface.ts": { - "crap_high": { - "count": 1 - } - }, - "src/cli/update-check.ts": { - "crap_moderate": { - "count": 1 } }, "test/integration/provider-scenarios/android-lifecycle.test.ts": { "complexity_critical": { "count": 1 - }, - "crap_critical": { - "count": 1 - }, - "crap_moderate": { - "count": 1 - } - }, - "test/integration/provider-scenarios/daemon-http-server.test.ts": { - "crap_high": { - "count": 1 - } - }, - "test/integration/provider-scenarios/ios-record-trace.test.ts": { - "crap_high": { - "count": 1 } }, "test/integration/provider-scenarios/macos-world.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 - } - }, - "test/integration/smoke-open-remote-config.test.ts": { - "crap_high": { - "count": 2 - } - }, - "test/integration/test-helpers.ts": { - "crap_moderate": { - "count": 1 - } - }, - "packages/platform-apple/src/runner/runner-startup-transport.ts": { - "crap_high": { - "count": 1 } }, "packages/maestro/test/conformance/conformance-normalize.ts": { "complexity_critical": { "count": 2 - }, - "crap_critical": { - "count": 2 } }, "src/daemon/server/http-rpc-methods.ts": { "complexity_high": { "count": 1 - }, - "crap_high": { - "count": 1 - }, - "crap_moderate": { - "count": 1 } } }, diff --git a/fallow-crap.json b/fallow-crap.json new file mode 100644 index 0000000000..6357e7e0c6 --- /dev/null +++ b/fallow-crap.json @@ -0,0 +1,10 @@ +{ + "$schema": "https://raw.githubusercontent.com/fallow-rs/fallow/main/schema.json", + "extends": ["./.fallowrc.json"], + // Coverage-based CRAP only: scripts/coverage-crap/run.ts passes the threshold (--max-crap), the + // Vitest coverage map and the PR diff. Cyclomatic and cognitive findings stay with check:fallow. + "rules": { + "complexity-cyclomatic": "off", + "complexity-cognitive": "off" + } +} diff --git a/package.json b/package.json index 3f39214c44..6513773423 100644 --- a/package.json +++ b/package.json @@ -153,6 +153,8 @@ "check:gate-manifest:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/gate/*.test.ts", "check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts", "check:coverage-changed:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts", + "check:coverage-crap": "node --experimental-strip-types scripts/coverage-crap/run.ts", + "check:coverage-crap:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-crap/model.test.ts scripts/coverage-crap/run.test.ts", "check:layering": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/layering/*.test.ts && node --experimental-strip-types scripts/layering/check.ts", "check:di-seams": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/di-seams/*.test.ts && node --experimental-strip-types scripts/di-seams/check.ts", "depgraph": "node --experimental-strip-types scripts/depgraph/build.ts", diff --git a/scripts/check-affected/checks.ts b/scripts/check-affected/checks.ts index 763df2ca61..d5a1dbf786 100644 --- a/scripts/check-affected/checks.ts +++ b/scripts/check-affected/checks.ts @@ -66,6 +66,7 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [ gate('unit', 'Unit + smoke suite', 'check:unit'), // CI owns the default coverage run; the scripts remain available for explicit local diagnosis. gate('coverage', 'Changed-line coverage', 'check:coverage-changed', false), + gate('coverage-crap', 'Coverage-based CRAP report', 'check:coverage-crap', false), gate('provider-integration', 'Provider-backed integration suite', 'test:integration:provider'), gate( 'integration-progress', @@ -104,6 +105,7 @@ export const CHECK_CATALOG: readonly CheckSpec[] = [ gate('tmpdir-leaks', 'Leaked test tmpdir detector', 'check:tmpdir-leaks'), gate('tmpdir-leaks-model', 'TMPDIR redirection model', 'check:tmpdir-leaks:test'), gate('coverage-model', 'Changed-line coverage model', 'check:coverage-changed:test'), + gate('coverage-crap-model', 'Coverage-based CRAP report model', 'check:coverage-crap:test'), gate('wire-compat-model', 'Wire-compat rules model', 'check:daemon-wire-compat:test'), gate('production-exports', 'Production-unused exports', 'check:production-exports'), gate('bundle-owner-files', 'Bundle owner-file manifest', 'check:bundle-owner-files'), diff --git a/scripts/check-affected/model.ts b/scripts/check-affected/model.ts index f1ed6994e2..e73e9466a6 100644 --- a/scripts/check-affected/model.ts +++ b/scripts/check-affected/model.ts @@ -50,6 +50,7 @@ export const ALL_CHECKS = [ 'unit', 'unit-ci', 'coverage', + 'coverage-crap', 'provider-integration', 'integration-progress', 'swift-runner-ios', @@ -66,6 +67,7 @@ export const ALL_CHECKS = [ 'tmpdir-leaks', 'tmpdir-leaks-model', 'coverage-model', + 'coverage-crap-model', 'wire-compat-model', 'production-exports', 'bundle-owner-files', diff --git a/scripts/coverage-changed/run.test.ts b/scripts/coverage-changed/run.test.ts index 4c2af3bb09..f9b58f85a8 100644 --- a/scripts/coverage-changed/run.test.ts +++ b/scripts/coverage-changed/run.test.ts @@ -96,6 +96,16 @@ test('fails when a changed source line is uncovered and names that line', () => assert.match(out, /`src\/feature\.ts`: 2/); }); +test('a changed line in a non-ASCII path is scored, not skipped by quoted diff headers', () => { + write('src/café.ts', 'export const covered = 1;\nexport const uncovered = 2;\n'); + git('add', '-A'); + git('commit', '-q', '-m', 'non-ascii path'); + writeLcov('SF:src/café.ts\nDA:1,3\nDA:2,0\nend_of_record\n'); + const { code, out } = capture(() => run(['--base', 'main'], repo)); + assert.equal(code, 1); + assert.match(out, /`src\/café\.ts`: 2/); +}); + test('fails when a changed workspace package line is uncovered and names that line', () => { write( 'packages/contracts/src/feature.ts', diff --git a/scripts/coverage-changed/run.ts b/scripts/coverage-changed/run.ts index 6a4bfd965f..a63f27e6f6 100644 --- a/scripts/coverage-changed/run.ts +++ b/scripts/coverage-changed/run.ts @@ -2,11 +2,11 @@ // `pnpm check:coverage-changed [--base ]`. // // Reuses the lcov report that `pnpm test:coverage` already produced (never runs -// coverage a second time), joins it with `git diff --unified=0 ...HEAD`, -// and fails when changed-line coverage is below the threshold. Renames are -// detected at RENAME_SIMILARITY regardless of the host's `diff.renames`, so a -// moved file owes only the hunks that differ from its source. A PR carrying -// the `coverage-waiver` label skips the failure but still prints every number. +// coverage a second time), joins it with the changed-lines diff against +// (`readChangedLinesDiff`, which pairs renames so a moved file owes only the +// hunks that differ from its source), and fails when changed-line coverage is +// below the threshold. A PR carrying the `coverage-waiver` label skips the +// failure but still prints every number. // The same markdown report goes to stdout and to the GitHub job summary; it // includes changed-branch coverage and the count of changed executable lines // excluded from coverage, both non-gating. @@ -16,6 +16,7 @@ import path from 'node:path'; import { pathToFileURL } from 'node:url'; import { runCmdSync } from '@agent-device/host-kit/command'; import { parseScriptArgs } from '../lib/cli-args.ts'; +import { readChangedLinesDiff } from '../lib/repo-git.ts'; import { computeChangedCoverage, parseLcov, @@ -25,8 +26,6 @@ import { const USAGE = 'Usage: pnpm check:coverage-changed [--base ]\n'; const LCOV_PATH = 'coverage/lcov.info'; -const RENAME_SIMILARITY = '90%'; -const GIT_DIFF_MAX_BUFFER_BYTES = 16 * 1024 * 1024; function fmtPct(pct: number | null): string { return pct === null ? 'n/a' : `${pct.toFixed(2)}%`; @@ -75,11 +74,7 @@ export function run(argv: readonly string[], cwd?: string): number { return 1; } - const diff = runCmdSync( - 'git', - ['diff', '--unified=0', '--no-color', `--find-renames=${RENAME_SIMILARITY}`, `${base}...HEAD`], - { cwd: root, maxBuffer: GIT_DIFF_MAX_BUFFER_BYTES }, - ).stdout; + const diff = readChangedLinesDiff(root, base); const result = computeChangedCoverage({ diffs: parseUnifiedDiff(diff), coverage: parseLcov(fs.readFileSync(lcovPath, 'utf8'), root), diff --git a/scripts/coverage-crap/fallow-health-report.fixture.json b/scripts/coverage-crap/fallow-health-report.fixture.json new file mode 100644 index 0000000000..e97f3449ba --- /dev/null +++ b/scripts/coverage-crap/fallow-health-report.fixture.json @@ -0,0 +1,57 @@ +{ + "kind": "health", + "schema_version": 11, + "version": "3.32.0", + "findings": [ + { + "path": "src/daemon/handlers/session-state.ts", + "name": "handleSessionStateCommands", + "line": 232, + "col": 7, + "cyclomatic": 34, + "cognitive": 44, + "line_count": 174, + "param_count": 1, + "exceeded": "all", + "severity": "critical", + "effective_severity": "error", + "crap": 34.1, + "coverage_pct": 95.83333333333334, + "coverage_tier": "high", + "coverage_source": "istanbul" + }, + { + "path": "src/daemon/handlers/session-state.ts", + "name": "handleAppStateCommand", + "line": 112, + "col": 0, + "cyclomatic": 30, + "cognitive": 36, + "line_count": 119, + "param_count": 1, + "exceeded": "all", + "severity": "high", + "effective_severity": "error", + "crap": 30.4, + "coverage_pct": 92.10526315789474, + "coverage_tier": "high", + "coverage_source": "istanbul" + }, + { + "path": "src/daemon/server/http-server.ts", + "name": "", + "line": 188, + "col": 18, + "cyclomatic": 45, + "cognitive": 40, + "line_count": 205, + "param_count": 0, + "exceeded": "all", + "severity": "critical", + "effective_severity": "error", + "crap": 482.4, + "coverage_tier": "partial", + "coverage_source": "estimated" + } + ] +} diff --git a/scripts/coverage-crap/model.test.ts b/scripts/coverage-crap/model.test.ts new file mode 100644 index 0000000000..25b3ba9856 --- /dev/null +++ b/scripts/coverage-crap/model.test.ts @@ -0,0 +1,138 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import { test } from 'node:test'; +import { + parseFallowJsonReport, + readCrapFindings, + renderCrapAnnotations, + renderCrapReport, + sanitizeIstanbulCoverage, +} from './model.ts'; + +const valid = { s: { 0: 3, 1: 0 }, f: { 0: 1 }, b: { 0: [2, 0] } }; + +test('sanitize keeps records whose counts are all non-negative integers', () => { + const { map, dropped } = sanitizeIstanbulCoverage({ '/repo/a.ts': valid }); + assert.deepEqual(map, { '/repo/a.ts': valid }); + assert.deepEqual(dropped, []); +}); + +test('sanitize drops a record with any impossible statement, function, or branch count', () => { + const { map, dropped } = sanitizeIstanbulCoverage({ + '/repo/ok.ts': valid, + '/repo/negative-branch.ts': { ...valid, b: { 0: [1237, -1237] } }, + '/repo/negative-statement.ts': { ...valid, s: { 0: -1 } }, + '/repo/fractional-function.ts': { ...valid, f: { 0: 0.5 } }, + '/repo/missing-count.ts': { ...valid, s: { 0: null } }, + }); + assert.deepEqual(Object.keys(map), ['/repo/ok.ts']); + assert.deepEqual(dropped, [ + '/repo/fractional-function.ts', + '/repo/missing-count.ts', + '/repo/negative-branch.ts', + '/repo/negative-statement.ts', + ]); +}); + +const finding = (overrides: Record) => ({ + path: 'src/a.ts', + name: 'run', + line: 10, + crap: 42, + cyclomatic: 6, + coverage_pct: 20, + coverage_source: 'istanbul', + ...overrides, +}); + +test('findings keep measured scores at or above the threshold, highest first', () => { + const findings = readCrapFindings( + { + findings: [ + finding({ name: 'low', crap: 29.9 }), + finding({ name: 'edge', crap: 30 }), + finding({ name: 'high', crap: 90, coverage_pct: undefined }), + finding({ name: 'estimated', crap: 95, coverage_source: 'estimated' }), + { path: 'src/b.ts', name: 'no-score', line: 1, cyclomatic: 12 }, + ], + }, + 30, + ); + assert.deepEqual( + findings.map((entry) => [entry.name, entry.crap, entry.coveragePct]), + [ + ['high', 90, null], + ['edge', 30, 20], + ], + ); +}); + +test('a report without a findings array is an error, not an empty verdict', () => { + assert.throws(() => readCrapFindings({ error: true }, 30), /no findings array/); +}); + +test('the report names touched functions and unscored files', () => { + const findings = readCrapFindings({ findings: [finding({})] }, 30); + const report = renderCrapReport({ + findings, + dropped: ['src/odd.ts'], + threshold: 30, + scope: 'touched', + }); + assert.match(report, /1 touched function\(s\) at or above 30/); + assert.match(report, /\| `src\/a\.ts:10` run \| 42\.0 \| 6 \| 20% \|/); + assert.match(report, /Unscored: 1 file\(s\).*`src\/odd\.ts`/); +}); + +test('an empty report says none and lists no table', () => { + const report = renderCrapReport({ findings: [], dropped: [], threshold: 30, scope: 'touched' }); + assert.match(report, /: no touched function/); + assert.doesNotMatch(report, /\| Function/); +}); + +test('a whole-repository report does not claim the functions were touched', () => { + const findings = readCrapFindings({ findings: [finding({})] }, 30); + const report = renderCrapReport({ findings, dropped: [], threshold: 30, scope: 'all' }); + assert.match(report, /: 1 function\(s\) at or above 30/); +}); + +test('annotations put each finding on its file and line', () => { + const findings = readCrapFindings({ findings: [finding({})] }, 30); + assert.equal( + renderCrapAnnotations(findings), + '::warning file=src/a.ts,line=10,title=CRAP 42.0::run has cyclomatic 6 at 20%25 coverage.\n', + ); +}); + +test('annotations escape a path or name that would end the workflow command', () => { + const findings = readCrapFindings( + { findings: [finding({ path: 'src/a,b:c.ts', name: 'run\n::error::forged 100%' })] }, + 30, + ); + assert.equal( + renderCrapAnnotations(findings), + '::warning file=src/a%2Cb%3Ac.ts,line=10,title=CRAP 42.0::run%0A::error::forged 100%25 has cyclomatic 6 at 20%25 coverage.\n', + ); +}); + +test('a captured fallow 3.32 report yields its measured findings past leading output', () => { + const report = fs.readFileSync( + new URL('./fallow-health-report.fixture.json', import.meta.url), + 'utf8', + ); + const findings = readCrapFindings( + parseFallowJsonReport(`loaded config: fallow-crap.json\n${report}`), + 30, + ); + assert.deepEqual( + findings.map((entry) => [entry.path, entry.name, entry.line, entry.crap]), + [ + ['src/daemon/handlers/session-state.ts', 'handleSessionStateCommands', 232, 34.1], + ['src/daemon/handlers/session-state.ts', 'handleAppStateCommand', 112, 30.4], + ], + ); +}); + +test('fallow output without a JSON report is an error', () => { + assert.throws(() => parseFallowJsonReport('Error: coverage map rejected'), /no JSON report/); +}); diff --git a/scripts/coverage-crap/model.ts b/scripts/coverage-crap/model.ts new file mode 100644 index 0000000000..351e16f045 --- /dev/null +++ b/scripts/coverage-crap/model.ts @@ -0,0 +1,154 @@ +// Pure pieces of the coverage-based CRAP report: which Istanbul file records fallow can trust, +// and how the touched functions it scores are rendered. run.ts owns the git, fallow and file I/O. + +type IstanbulCounters = Readonly<{ + s?: Readonly>; + f?: Readonly>; + b?: Readonly>; +}>; + +export type IstanbulCoverageMap = Readonly>; + +export type SanitizedCoverage = Readonly<{ + map: IstanbulCoverageMap; + /** Records dropped because a hit count is not a non-negative integer. */ + dropped: readonly string[]; +}>; + +function isHitCount(value: unknown): boolean { + return typeof value === 'number' && Number.isInteger(value) && value >= 0; +} + +function countsAreValid(record: IstanbulCounters): boolean { + const scalars = [...Object.values(record.s ?? {}), ...Object.values(record.f ?? {})]; + const branches = Object.values(record.b ?? {}).flatMap((counts) => + Array.isArray(counts) ? counts : [counts], + ); + return [...scalars, ...branches].every(isHitCount); +} + +/** + * Drops file records with a negative or non-integer hit count. Vitest's v8 provider can merge a + * module loaded both through its runner and natively into negative counts (vitest#11541), and + * fallow refuses the whole map on one. Zeroed counts from the same bug are indistinguishable from + * untested code and stay. A dropped file gets no CRAP score rather than a wrong one. + */ +export function sanitizeIstanbulCoverage(map: IstanbulCoverageMap): SanitizedCoverage { + const kept: Record = {}; + const dropped: string[] = []; + for (const [file, record] of Object.entries(map)) { + if (countsAreValid(record)) kept[file] = record; + else dropped.push(file); + } + return { map: kept, dropped: dropped.sort() }; +} + +export type CrapFinding = Readonly<{ + path: string; + name: string; + line: number; + crap: number; + cyclomatic: number; + coveragePct: number | null; +}>; + +function readFinding(value: unknown): CrapFinding | null { + if (typeof value !== 'object' || value === null) return null; + const record = value as Record; + const { path, name, line, crap, cyclomatic } = record; + // Outside the coverage map (tests, scripts, dropped records) fallow estimates from imports. + if (record.coverage_source !== 'istanbul') return null; + if (typeof path !== 'string' || typeof name !== 'string') return null; + if (typeof line !== 'number' || typeof crap !== 'number' || typeof cyclomatic !== 'number') { + return null; + } + const coveragePct = typeof record.coverage_pct === 'number' ? record.coverage_pct : null; + return { path, name, line, crap, cyclomatic, coveragePct }; +} + +/** The JSON report from fallow's stdout, past any line it prints before the report. */ +export function parseFallowJsonReport(stdout: string): unknown { + const start = stdout.indexOf('{'); + if (start === -1) throw new Error(`fallow printed no JSON report:\n${stdout.slice(0, 2000)}`); + return JSON.parse(stdout.slice(start)); +} + +/** The measured CRAP findings of one `fallow health --format json` report, highest score first. */ +export function readCrapFindings(report: unknown, threshold: number): CrapFinding[] { + const findings = + typeof report === 'object' && report !== null + ? (report as { findings?: unknown }).findings + : undefined; + if (!Array.isArray(findings)) { + throw new Error('fallow health report has no findings array'); + } + return findings + .map(readFinding) + .filter((finding): finding is CrapFinding => finding !== null && finding.crap >= threshold) + .sort((left, right) => right.crap - left.crap || left.path.localeCompare(right.path)); +} + +function formatCoverage(pct: number | null): string { + return pct === null ? 'n/a' : `${pct.toFixed(0)}%`; +} + +/** `touched` scores the functions a diff changes; `all` scores every function in the map. */ +export type CrapScope = 'touched' | 'all'; + +export function renderCrapReport(input: { + findings: readonly CrapFinding[]; + dropped: readonly string[]; + threshold: number; + scope: CrapScope; +}): string { + const { findings, dropped, threshold, scope } = input; + const subject = scope === 'touched' ? 'touched function(s)' : 'function(s)'; + const lines = [ + `## Coverage-based CRAP (report-only): ${findings.length === 0 ? 'no' : findings.length} ${subject} at or above ${threshold}`, + '', + ]; + if (findings.length > 0) { + lines.push('| Function | CRAP | Cyclomatic | Coverage |', '| --- | ---: | ---: | ---: |'); + for (const finding of findings) { + lines.push( + `| \`${finding.path}:${finding.line}\` ${finding.name} | ${finding.crap.toFixed(1)} | ${finding.cyclomatic} | ${formatCoverage(finding.coveragePct)} |`, + ); + } + lines.push( + '', + 'Add tests for the uncovered branches, or split the function. Code that only runs out of process (workers, browser-evaluated scripts, child processes) measures 0%.', + ); + } + if (dropped.length > 0) { + lines.push( + '', + `Unscored: ${dropped.length} file(s) with invalid coverage counts (vitest#11541): ${dropped.map((file) => `\`${file}\``).join(', ')}`, + ); + } + return `${lines.join('\n')}\n`; +} + +// Workflow-command escaping (actions/toolkit `command.ts`): data may not carry `%`, CR, or LF, and +// a property additionally may not carry `:` or `,`, or a path or name could end the command early. +function escapeCommandData(value: string): string { + return value.replaceAll('%', '%25').replaceAll('\r', '%0D').replaceAll('\n', '%0A'); +} + +function escapeCommandProperty(value: string): string { + return escapeCommandData(value).replaceAll(':', '%3A').replaceAll(',', '%2C'); +} + +/** GitHub Actions warning annotations, one per finding, so the score shows on the diff. */ +export function renderCrapAnnotations(findings: readonly CrapFinding[]): string { + return findings + .map((finding) => { + const properties = [ + `file=${escapeCommandProperty(finding.path)}`, + `line=${finding.line}`, + `title=${escapeCommandProperty(`CRAP ${finding.crap.toFixed(1)}`)}`, + ].join(','); + const message = `${finding.name} has cyclomatic ${finding.cyclomatic} at ${formatCoverage(finding.coveragePct)} coverage.`; + return `::warning ${properties}::${escapeCommandData(message)}\n`; + }) + .join(''); +} diff --git a/scripts/coverage-crap/run.test.ts b/scripts/coverage-crap/run.test.ts new file mode 100644 index 0000000000..25b347def7 --- /dev/null +++ b/scripts/coverage-crap/run.test.ts @@ -0,0 +1,31 @@ +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { afterEach, beforeEach, test } from 'node:test'; +import { run } from './run.ts'; + +let root: string; + +beforeEach(() => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'coverage-crap-')); +}); + +afterEach(() => { + fs.rmSync(root, { recursive: true, force: true }); +}); + +test('fails with the command to run when no coverage map exists', () => { + const chunks: string[] = []; + const originalErr = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: string | Uint8Array): boolean => { + chunks.push(String(chunk)); + return true; + }) as typeof process.stderr.write; + try { + assert.equal(run(['--base', 'main'], root), 1); + } finally { + process.stderr.write = originalErr; + } + assert.match(chunks.join(''), /no Istanbul coverage map .*Run `pnpm test:coverage` first/); +}); diff --git a/scripts/coverage-crap/run.ts b/scripts/coverage-crap/run.ts new file mode 100644 index 0000000000..ca7291c603 --- /dev/null +++ b/scripts/coverage-crap/run.ts @@ -0,0 +1,107 @@ +// Entry point for the coverage-based CRAP report: `pnpm check:coverage-crap [--base | --all]`. +// +// Reuses the Istanbul map `pnpm test:coverage` already wrote (never runs coverage a second time), +// drops records with impossible counts, and asks fallow to score the functions the diff against +// touches, or every function with --all. Report-only: it prints the functions at or above +// the threshold to stdout and the GitHub job summary (touched functions also as warning +// annotations), and exits 0 once it has a verdict. + +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { runCmdSync } from '@agent-device/host-kit/command'; +import { parseScriptArgs } from '../lib/cli-args.ts'; +import { readChangedLinesDiff } from '../lib/repo-git.ts'; +import { + parseFallowJsonReport, + readCrapFindings, + renderCrapAnnotations, + renderCrapReport, + sanitizeIstanbulCoverage, + type CrapScope, + type IstanbulCoverageMap, +} from './model.ts'; + +const USAGE = 'Usage: pnpm check:coverage-crap [--base | --all]\n'; +const COVERAGE_PATH = 'coverage/coverage-istanbul.json'; +const SANITIZED_PATH = 'coverage/fallow-crap-coverage.json'; +const DIFF_PATH = 'coverage/fallow-crap.diff'; +const FALLOW_CONFIG = 'fallow-crap.json'; +const CRAP_THRESHOLD = 30; +const FALLOW_MAX_BUFFER_BYTES = 64 * 1024 * 1024; + +export function run(argv: readonly string[], cwd?: string): number { + const values = parseScriptArgs(argv, USAGE, { + base: { type: 'string', default: 'origin/main' }, + all: { type: 'boolean', default: false }, + }); + const base = values.base ?? 'origin/main'; + const scope: CrapScope = values.all ? 'all' : 'touched'; + const root = cwd ?? runCmdSync('git', ['rev-parse', '--show-toplevel']).stdout.trim(); + const coveragePath = path.resolve(root, COVERAGE_PATH); + if (!fs.existsSync(coveragePath)) { + process.stderr.write( + `check:coverage-crap: no Istanbul coverage map at ${coveragePath}. Run \`pnpm test:coverage\` first.\n`, + ); + return 1; + } + + const { map, dropped } = sanitizeIstanbulCoverage( + JSON.parse(fs.readFileSync(coveragePath, 'utf8')) as IstanbulCoverageMap, + ); + fs.writeFileSync(path.resolve(root, SANITIZED_PATH), JSON.stringify(map)); + if (scope === 'touched') { + fs.writeFileSync(path.resolve(root, DIFF_PATH), readChangedLinesDiff(root, base)); + } + + const fallow = runCmdSync( + process.execPath, + [ + path.join(root, 'node_modules', 'fallow', 'bin', 'fallow'), + 'health', + '--config', + FALLOW_CONFIG, + '--complexity', + '--max-crap', + String(CRAP_THRESHOLD), + '--coverage', + SANITIZED_PATH, + ...(scope === 'touched' ? ['--diff-file', DIFF_PATH] : []), + '--report-only', + '--format', + 'json', + '--quiet', + ], + { cwd: root, allowFailure: true, maxBuffer: FALLOW_MAX_BUFFER_BYTES }, + ); + if (fallow.exitCode !== 0) { + process.stderr.write(`check:coverage-crap: fallow health failed:\n${fallow.stderr}\n`); + return 1; + } + const findings = readCrapFindings(parseFallowJsonReport(fallow.stdout), CRAP_THRESHOLD); + const report = renderCrapReport({ + findings, + dropped: dropped.map((file) => path.relative(root, file)), + threshold: CRAP_THRESHOLD, + scope, + }); + process.stdout.write(report); + const summaryPath = process.env.GITHUB_STEP_SUMMARY; + if (summaryPath) fs.appendFileSync(summaryPath, report); + if (scope === 'touched' && process.env.GITHUB_ACTIONS === 'true') { + process.stdout.write(renderCrapAnnotations(findings)); + } + return 0; +} + +// exitCode, not exit(): a large report written to a pipe must drain before the process ends. +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + try { + process.exitCode = run(process.argv.slice(2)); + } catch (error: unknown) { + process.stderr.write( + `check:coverage-crap: ${error instanceof Error ? error.message : String(error)}\n`, + ); + process.exitCode = 1; + } +} diff --git a/scripts/lib/repo-git.ts b/scripts/lib/repo-git.ts index f7ea03ebb5..d4ca6e4234 100644 --- a/scripts/lib/repo-git.ts +++ b/scripts/lib/repo-git.ts @@ -13,3 +13,28 @@ export function requireUnshallowHistory(git: RepoGit, subject: string): void { `${subject} needs full history and tags. Run \`git fetch --unshallow --tags\` first.`, ); } + +const CHANGED_LINES_RENAME_SIMILARITY = '90%'; +const CHANGED_LINES_MAX_BUFFER_BYTES = 16 * 1024 * 1024; + +/** + * The zero-context diff of HEAD against its merge base with `base`, which the changed-line gates + * score. Renames are paired at 90% regardless of the host's `diff.renames`, so a moved file owes + * only the hunks that differ from its source; paths stay unquoted (`core.quotePath=false`) so a + * non-ASCII file name matches its coverage record; the buffer fits a large stacked diff. + */ +export function readChangedLinesDiff(repoRoot: string, base: string): string { + return runCmdSync( + 'git', + [ + '-c', + 'core.quotePath=false', + 'diff', + '--unified=0', + '--no-color', + `--find-renames=${CHANGED_LINES_RENAME_SIMILARITY}`, + `${base}...HEAD`, + ], + { cwd: repoRoot, maxBuffer: CHANGED_LINES_MAX_BUFFER_BYTES }, + ).stdout; +} diff --git a/vitest.config.ts b/vitest.config.ts index 6ea6ec7b54..c08a37d680 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -257,7 +257,13 @@ export default defineConfig({ ].map((project) => ({ ...project, test: { ...project.test, ...TEST_ISOLATION } })), coverage: { provider: 'v8', - reporter: ['text', 'html', 'lcov', 'json-summary'], + reporter: [ + 'text', + 'html', + 'lcov', + 'json-summary', + ['json', { file: 'coverage-istanbul.json' }], + ], thresholds: { statements: 78, lines: 80,