diff --git a/scripts/layering/check.ts b/scripts/layering/check.ts index ea33306b2c..785a7e5a48 100644 --- a/scripts/layering/check.ts +++ b/scripts/layering/check.ts @@ -63,6 +63,8 @@ // - Over WORKSPACE PACKAGE EXPORTS: no package but test-kit publishes test support (fixtures, // fakes, mocks, harnesses, `__tests__/`, `./testing/*`); the offender set is a merge-base // ratchet, and production code may not import those exports or test-kit (R83). +// - Over UNIT TESTS: each `*.test.ts` under `src/` and `packages/*/src/` is named for a module +// beside it; the unmatched set is a merge-base ratchet (R84). // - Over CONTRACTS PRODUCTION SOURCE: contracts owns vocabulary only — host, process, and timer // mechanics belong in capture-kit or an adapter (R18). // R6, R9, and the R10 R7 counts are ratchets with no written-down reference: each is the same @@ -143,13 +145,14 @@ import { packageExportSurfaceSummary, } from './package-export-surface.ts'; import { checkTestKitIsolation } from './test-kit-isolation.ts'; +import { checkTestColocation, testColocationSummary } from './test-colocation.ts'; import { checkDaemonLayers, DAEMON_ROOT } from './daemon-layers.ts'; import { checkSessionAuthorityOverlay, handlerOwnedOverlay } from './session-authority-overlay.ts'; import { DAEMON_SOURCE_ROOT, listTrackedPlatformZoneFiles, listTrackedProductionSources, - listTrackedPackageFiles, + listTrackedSourceTreeFiles, listTrackedSrcUtilsFiles, listTrackedTypeScriptFiles, } from './tracked-sources.ts'; @@ -446,7 +449,8 @@ function report( `no production daemon file reaches the root platform-runtime family or a hub that ` + `does (R76); the daemon process reaches no caller-side replay source module over ` + `static or dynamic edges (R82); ` + - `${packageExportSurfaceSummary(ratchets.testOnlyExports, reference.testOnlyExports)}; and ` + + `${packageExportSurfaceSummary(ratchets.testOnlyExports, reference.testOnlyExports)}; ` + + `${testColocationSummary(ratchets.unmatchedTests, reference.unmatchedTests)}; and ` + `the handler-owned SessionState/SessionStore ` + `authority overlay holds at or under the merge-base (R75, ` + `${handlerOwnedShapeFiles} shape / ${handlerOwnedAuthorityFiles} authority files); and ` + @@ -535,6 +539,7 @@ export const LAYERING_RULE_IDS = [ 'session-authority-overlay', 'daemon-layers', 'package-export-surface', + 'test-colocation', ] as const; export type LayeringRuleId = (typeof LAYERING_RULE_IDS)[number]; @@ -615,6 +620,8 @@ export const LAYERING_RULES: Readonly> = { ), ...checkTestKitIsolation(readTrackedPackageManifests(repoRoot), context.allTypeScriptSources), ], + 'test-colocation': (context) => + checkTestColocation(context.ratchets.unmatchedTests, context.reference.unmatchedTests), }; export function main(): number { @@ -629,7 +636,7 @@ export function main(): number { // Measured once and threaded: the rules and the success line must report the same numbers. const ratchets = measureRatchets(sources, edges, { manifests: readTrackedPackageManifests(repoRoot), - packageFiles: listTrackedPackageFiles(repoRoot), + files: listTrackedSourceTreeFiles(repoRoot), }); const reference = mergeBaseRatchets(repoRoot, parse); const context: LayeringContext = { diff --git a/scripts/layering/ratchet-reference.test.ts b/scripts/layering/ratchet-reference.test.ts index de8c37d57b..6163dae53f 100644 --- a/scripts/layering/ratchet-reference.test.ts +++ b/scripts/layering/ratchet-reference.test.ts @@ -2,7 +2,7 @@ import assert from 'node:assert/strict'; import { execFileSync } from 'node:child_process'; import { test } from 'node:test'; import { memoizedImportParser, resolveImportEdges } from './model.ts'; -import { measureRatchets, mergeBaseRatchets } from './ratchet-reference.ts'; +import { inHeadCoordinates, measureRatchets, mergeBaseRatchets } from './ratchet-reference.ts'; import { sessionStateWritePressure } from './session-state.ts'; import { listTrackedProductionSources } from './tracked-sources.ts'; import { readCommittedSources } from '../__tests__/committed-source-tree.ts'; @@ -52,6 +52,17 @@ test('sessionStateWritePressure counts written fields and (field, writer) claims }); }); +test('carried merge-base entries map to head paths and stay sorted', () => { + const moved = new Map([['src/z-old.test.ts', 'src/a-new.test.ts']]); + assert.deepEqual( + inHeadCoordinates( + ['src/b.test.ts', 'src/z-old.test.ts', 'src/c.test.ts'], + (file) => moved.get(file) ?? file, + ), + ['src/a-new.test.ts', 'src/b.test.ts', 'src/c.test.ts'], + ); +}); + test('measureRatchets reports all ratchets from one tree', () => { const sources = tree(); assert.deepEqual(measureRatchets(sources, resolveImportEdges(sources)), { @@ -60,6 +71,7 @@ test('measureRatchets reports all ratchets from one tree', () => { sessionState: { writerOwnedFields: 2, ownerFileClaims: 3 }, sessionAuthority: { shapeFiles: [], authorityFiles: [] }, testOnlyExports: [], + unmatchedTests: [], }); const withExports = measureRatchets(sources, resolveImportEdges(sources), { manifests: new Map([ @@ -68,7 +80,7 @@ test('measureRatchets reports all ratchets from one tree', () => { JSON.stringify({ name: '@agent-device/p', exports: { './x-fixtures': './src/x.ts' } }), ], ]), - packageFiles: [], + files: [], }); assert.deepEqual( withExports.testOnlyExports.map(({ id }) => id), diff --git a/scripts/layering/ratchet-reference.ts b/scripts/layering/ratchet-reference.ts index 9352887e13..9186cda417 100644 --- a/scripts/layering/ratchet-reference.ts +++ b/scripts/layering/ratchet-reference.ts @@ -28,6 +28,7 @@ import { type SessionAuthorityOverlay, } from './session-authority-overlay.ts'; import { sessionStateWritePressure, type SessionStateWritePressure } from './session-state.ts'; +import { measureUnmatchedTests } from './test-colocation.ts'; export type LayeringRatchets = Readonly<{ /** R6: distinct type-only spine inversions per `from -> to` zone pair. */ @@ -40,27 +41,41 @@ export type LayeringRatchets = Readonly<{ sessionAuthority: SessionAuthorityOverlay; /** R83: the test-only package exports published, whole tree, test-kit excluded. */ testOnlyExports: readonly TestOnlyExport[]; + /** R84: unit tests named for no module beside them, sorted. */ + unmatchedTests: readonly string[]; }>; -/** What R83 measures a tree from: its package manifests and its tracked package paths. */ -export type PackageExportSurfaceInput = Readonly<{ +/** What R83 and R84 measure a tree from: its package manifests and its tracked `src/` and `packages/` paths. */ +export type RepoTreeInput = Readonly<{ manifests: ReadonlyMap; - packageFiles: readonly string[]; + files: readonly string[]; }>; +/** + * Merge-base entries in head coordinates, sorted again: a rename can change lexical order, and + * `measureUnmatchedTests` promises sorted output. + */ +export function inHeadCoordinates( + entries: readonly string[], + toHead: (file: string) => string, +): string[] { + return entries.map(toHead).sort(); +} + export type MergeBaseRatchets = LayeringRatchets & Readonly<{ ref: string }>; export function measureRatchets( sources: ReadonlyMap, edges: readonly ResolvedImportEdge[], - exportSurface: PackageExportSurfaceInput = { manifests: new Map(), packageFiles: [] }, + tree: RepoTreeInput = { manifests: new Map(), files: [] }, ): LayeringRatchets { return { typeInversions: typeInversionCounts(edges), largestTypeCycle: largestTypeCycleMembers(edges), sessionState: sessionStateWritePressure(sources), sessionAuthority: measureSessionAuthorityOverlay(sources, edges), - testOnlyExports: measureTestOnlyExports(exportSurface.manifests, exportSurface.packageFiles), + testOnlyExports: measureTestOnlyExports(tree.manifests, tree.files), + unmatchedTests: measureUnmatchedTests(tree.files), }; } @@ -101,11 +116,12 @@ export function mergeBaseRatchets( : { ...edge, file, target, fromZone: targetDagZone(file), toZone: targetDagZone(target) }; }); const canonical = new Map([...sources].map(([file, text]) => [toHead(file), text])); + const base = measureRatchets(canonical, edges, { manifests, files }); return { ref, - ...measureRatchets(canonical, edges, { - manifests, - packageFiles: files.filter((file) => file.startsWith('packages/')), - }), + ...base, + // Base entries in head coordinates, like the edges above: a test moved or renamed since the + // merge-base is the same entry, not new. + unmatchedTests: inHeadCoordinates(base.unmatchedTests, toHead), }; } diff --git a/scripts/layering/test-colocation.test.ts b/scripts/layering/test-colocation.test.ts new file mode 100644 index 0000000000..fc11a046dc --- /dev/null +++ b/scripts/layering/test-colocation.test.ts @@ -0,0 +1,165 @@ +// R84 test-colocation: which tests count as sitting beside a module they are named for, and the +// merge-base ratchet over those that do not. + +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; +import { test } from 'node:test'; +import { + checkTestColocation, + measureUnmatchedTests, + TEST_COLOCATION_RULE, + testColocationSummary, +} from './test-colocation.ts'; + +function unmatched(...files: string[]) { + return measureUnmatchedTests(files); +} + +test('a test named for the module beside it, or for it plus an aspect, is matched', () => { + assert.deepEqual( + unmatched( + 'packages/p/src/store.ts', + 'packages/p/src/store.test.ts', + 'packages/p/src/store-recovery.test.ts', + 'src/commands/open.ts', + 'src/commands/open-flags.test.ts', + ), + [], + ); +}); + +test('a test named for no module in its directory is unmatched', () => { + assert.deepEqual( + unmatched( + 'packages/p/src/store.ts', + 'packages/p/src/other.test.ts', + 'packages/p/src/store2.test.ts', + 'packages/p/src/sto.test.ts', + ), + ['packages/p/src/other.test.ts', 'packages/p/src/sto.test.ts', 'packages/p/src/store2.test.ts'], + ); +}); + +test('a declaration module is named for its stem: foo.d.ts matches foo.test.ts', () => { + assert.deepEqual(unmatched('src/foo.d.ts', 'src/foo.test.ts', 'src/foo-shape.test.ts'), []); + assert.deepEqual(unmatched('src/foo.d.ts', 'src/foo.d.test.ts'), ['src/foo.d.test.ts']); +}); + +test('the aspect suffix is dash-separated: a dotted or glued name does not match', () => { + assert.deepEqual( + unmatched( + 'packages/p/src/store.ts', + 'packages/p/src/store.recovery.test.ts', + 'packages/p/src/storerecovery.test.ts', + ), + ['packages/p/src/store.recovery.test.ts', 'packages/p/src/storerecovery.test.ts'], + ); +}); + +test('a test in __tests__ is judged against the modules of its parent directory', () => { + assert.deepEqual( + unmatched( + 'packages/p/src/handlers/lease.ts', + 'packages/p/src/handlers/__tests__/lease.test.ts', + 'packages/p/src/handlers/__tests__/lease-expiry.test.ts', + 'src/session.ts', + 'src/__tests__/session.test.ts', + ), + [], + ); + assert.deepEqual( + unmatched( + 'packages/p/src/handlers/lease.ts', + 'packages/p/src/handlers/__tests__/session.test.ts', + 'packages/p/src/handlers/__tests__/support/lease.test.ts', + ), + [ + 'packages/p/src/handlers/__tests__/session.test.ts', + 'packages/p/src/handlers/__tests__/support/lease.test.ts', + ], + ); +}); + +test('modules inside __tests__ do not make a test matched', () => { + assert.deepEqual( + unmatched('packages/p/src/__tests__/helper.ts', 'packages/p/src/__tests__/helper.test.ts'), + ['packages/p/src/__tests__/helper.test.ts'], + ); +}); + +test('index is an ordinary module: a cross-module package test is index-.test.ts', () => { + assert.deepEqual( + unmatched( + 'packages/p/src/index.ts', + 'packages/p/src/index-surface.test.ts', + 'packages/p/src/index-conformance.test.ts', + ), + [], + ); + assert.deepEqual(unmatched('packages/p/src/index.ts', 'packages/p/src/surface.test.ts'), [ + 'packages/p/src/surface.test.ts', + ]); + // Without an index.ts beside it there is nothing for the name to point at. + assert.deepEqual(unmatched('packages/p/src/index-surface.test.ts'), [ + 'packages/p/src/index-surface.test.ts', + ]); +}); + +test('fixtures never count as the module a test is named for, and test-kit is not measured', () => { + assert.deepEqual(unmatched('packages/p/src/store.fixtures.ts', 'packages/p/src/store.test.ts'), [ + 'packages/p/src/store.test.ts', + ]); + assert.deepEqual(unmatched('packages/test-kit/src/other.test.ts'), []); +}); + +test('test/, scripts/ and package test trees are exempt: the rule reads src trees only', () => { + assert.deepEqual( + unmatched( + 'test/integration/smoke.test.ts', + 'test/repo-guards/swift-sources.test.ts', + 'scripts/layering/check-wiring.test.ts', + 'packages/p/test/conformance.test.ts', + 'packages/p/package.json', + ), + [], + ); +}); + +test('a daemon-composition style suite under src/__tests__ is unmatched', () => { + assert.deepEqual( + unmatched('src/__tests__/daemon-composition/handlers/claims.test.ts', 'src/session.ts'), + ['src/__tests__/daemon-composition/handlers/claims.test.ts'], + ); +}); + +test('growth against the merge-base fails; moving a test into place or an unchanged set passes', () => { + const base = ['src/__tests__/a.test.ts', 'src/__tests__/b.test.ts']; + const violations = checkTestColocation( + ['src/__tests__/b.test.ts', 'src/__tests__/c.test.ts'], + base, + ); + assert.equal(violations.length, 1); + assert.equal(violations[0]!.rule, TEST_COLOCATION_RULE); + assert.equal(violations[0]!.file, 'src/__tests__/c.test.ts'); + assert.match(violations[0]!.message, /index-\.test\.ts/); + assert.match(violations[0]!.message, /test\/repo-guards\//); + assert.deepEqual(checkTestColocation([], base), []); + assert.deepEqual(checkTestColocation(base, base), []); +}); + +test('the remedy locations R84 names are collected by a vitest project', () => { + // A test moved to test/repo-guards/ must run somewhere: no gate fails on a test no project collects. + const config = fs.readFileSync( + path.resolve(import.meta.dirname, '../../vitest.config.ts'), + 'utf8', + ); + assert.match(config, /'test\/repo-guards\/\*\.test\.ts'/); +}); + +test('the summary reports the remaining count against the merge-base', () => { + assert.match( + testColocationSummary(['a'], ['a', 'b']), + /1 unmatched test\(s\) remain against 2 at the merge-base \(R84\)/, + ); +}); diff --git a/scripts/layering/test-colocation.ts b/scripts/layering/test-colocation.ts new file mode 100644 index 0000000000..acfec08f80 --- /dev/null +++ b/scripts/layering/test-colocation.ts @@ -0,0 +1,100 @@ +// Catches: a unit test that sits beside no module it is named for, so a reader of a module cannot +// find its tests and a test cannot say which module it pins. Every `*.test.ts` under root `src/` +// and `packages//src/` sits in the directory of a module (or in that directory's +// `__tests__/`), and its name is that module's name or the module's name plus `-`: +// `session-store.test.ts` and `session-store-recovery.test.ts` for `session-store.ts`. +// The offender set only shrinks: it is measured at the merge-base with origin/main by the same +// function that measures the working tree (`ratchet-reference.ts`), so a new unmatched test +// fails, a move or rename into place needs no edit, and no baseline file exists to append to. +// Evidence: when the rule landed, a fifth of the unit tests matched no module (most under +// `src/__tests__/daemon-composition`, the rest across `src/__tests__`, the daemon's handler +// trees and platform-apple's runner), so "tests mirror source topology one-to-one" (AGENTS.md) +// was a sentence with no enforcement; the live count is in the check:layering output. +// Cost: one pass over the tracked file list per tree. +// Kill criterion: a maintainer decision that tests need not sit beside the modules they test. +// +// R84 test-colocation. Two conventions keep the rule free of exemptions: +// - Cross-module package tests (conformance drivers, package-surface tests) are named for the +// package entry: `index-.test.ts` beside `index.ts`. `index` is an ordinary module. +// - Repo-level guards (tests that assert on Swift sources, manifests, docs or scripts) live in +// `test/repo-guards/` and run in `unit-core`. +// `test/` (integration, e2e, wire-compat, repo guards) and `scripts/` are exempt: the rule reads +// only root `src/` and package `src/` trees, and not test-kit, whose sources are test support. A +// module is any production source file (`isProductionSourceFile`): fixtures never count as the +// module a test is named for. + +import path from 'node:path'; +import type { LayeringViolation } from './model.ts'; +import { isProductionSourceFile, SOURCE_TREE_ROOT } from './tracked-sources.ts'; + +export const TEST_COLOCATION_RULE = 'R84 test-colocation'; + +const TEST_KIT_TREE = 'packages/test-kit/'; + +function inScope(file: string): boolean { + return SOURCE_TREE_ROOT.test(file) && !file.startsWith(TEST_KIT_TREE); +} +const TEST_FILE = /\.test\.[cm]?[jt]sx?$/; + +/** The directory whose modules a test is judged against: its own, or the parent of `__tests__`. */ +function moduleDirectory(testFile: string): string { + const dir = path.posix.dirname(testFile); + return path.posix.basename(dir) === '__tests__' ? path.posix.dirname(dir) : dir; +} + +/** + * Every in-scope unit test, sorted, that is named for no module in its module directory. `files` + * is one tree's tracked paths. + */ +export function measureUnmatchedTests(files: readonly string[]): string[] { + const modulesByDirectory = new Map(); + for (const file of files) { + if (!inScope(file) || !isProductionSourceFile(file)) continue; + const dir = path.posix.dirname(file); + const modules = modulesByDirectory.get(dir) ?? []; + modules.push(path.posix.basename(file).replace(/(?:\.d)?\.ts$/, '')); + modulesByDirectory.set(dir, modules); + } + const unmatched: string[] = []; + for (const file of files) { + if (!inScope(file) || !TEST_FILE.test(file)) continue; + const name = path.posix.basename(file).replace(TEST_FILE, ''); + const modules = modulesByDirectory.get(moduleDirectory(file)) ?? []; + if (!modules.some((module) => name === module || name.startsWith(`${module}-`))) { + unmatched.push(file); + } + } + return unmatched.sort(); +} + +/** + * Growth fails: an unmatched test at head that the merge-base did not already hold. Base entries + * arrive in head coordinates, so a test moved or renamed since the merge-base is the same entry. + */ +export function checkTestColocation( + head: readonly string[], + base: readonly string[], +): LayeringViolation[] { + const held = new Set(base); + return head + .filter((file) => !held.has(file)) + .map((file) => ({ + rule: TEST_COLOCATION_RULE, + file, + line: 1, + message: + `${path.posix.basename(file)} is named for no module in ${moduleDirectory(file)}/. Name it ` + + `.test.ts or -.test.ts for a module there (index-.test.ts ` + + `for a cross-module package test, beside index.ts), move it beside its module, or, for a ` + + `repo-level guard on Swift sources, manifests, docs or scripts, move it to ` + + `test/repo-guards/.`, + })); +} + +/** The `R84` success-line fragment: how many unmatched tests remain, against the merge-base. */ +export function testColocationSummary(head: readonly string[], base: readonly string[]): string { + return ( + `every new unit test is named for a module beside it: ${head.length} unmatched test(s) ` + + `remain against ${base.length} at the merge-base (R84)` + ); +} diff --git a/scripts/layering/tracked-sources.ts b/scripts/layering/tracked-sources.ts index da5cd00a34..d206b5a6e1 100644 --- a/scripts/layering/tracked-sources.ts +++ b/scripts/layering/tracked-sources.ts @@ -75,7 +75,8 @@ function listTrackedFiles(repoRoot: string, pathspecs: readonly string[]): strin */ export const DAEMON_SOURCE_ROOT = 'packages/daemon/src/'; -const PRODUCTION_SOURCE_ROOT = /^(?:src|packages\/[^/]+\/src)\//; +/** Root `src/` and any package's `src/`: the trees whose files layering rules read. */ +export const SOURCE_TREE_ROOT = /^(?:src|packages\/[^/]+\/src)\//; /** * Test support, by path: `__tests__/` and package-local `test/` trees, `*.test.*` and @@ -92,12 +93,12 @@ const TEST_SUPPORT_PATH = * production sources, so tooling and vitest setup there may import test support. */ export function isProductionSourceFile(file: string): boolean { - return file.endsWith('.ts') && PRODUCTION_SOURCE_ROOT.test(file) && !TEST_SUPPORT_PATH.test(file); + return file.endsWith('.ts') && SOURCE_TREE_ROOT.test(file) && !TEST_SUPPORT_PATH.test(file); } -/** Every tracked file under `packages/`, repo-root-relative. */ -export function listTrackedPackageFiles(repoRoot: string): string[] { - return listTrackedFiles(repoRoot, ['packages']); +/** Every tracked file under root `src/` and `packages/`, repo-root-relative. */ +export function listTrackedSourceTreeFiles(repoRoot: string): string[] { + return listTrackedFiles(repoRoot, ['src', 'packages']); } /** The canonical layering scan input: tracked, production, repo-root-relative. */