diff --git a/BENCHMARKS.md b/BENCHMARKS.md index b4a91d5..42c7538 100644 --- a/BENCHMARKS.md +++ b/BENCHMARKS.md @@ -320,11 +320,30 @@ the win scales with init cost times test count. Durations also become honest in this mode: they are measured inside the harness around `m.Run`, not around a process that spends most of its life initialising. -The follow-up that would turn the wash into a win everywhere is parsing the -binary covcounters format directly instead of shelling out to covdata per -window. It is deliberately not attempted here: the format is internal to the Go -toolchain and version-sensitive, and a maintenance trap should be walked into -deliberately or not at all. +The follow-up that turns the wash into a win everywhere is parsing the binary +covmeta/covcounters formats directly instead of shelling out to covdata per +window — and it is now built, with the maintenance trap fenced rather than +walked into blind. Both formats are version-stamped and unchanged since Go +1.20; the decoder refuses anything it does not recognise — future version, +foreign magic, meta-hash mismatch, truncation — and any refusal falls back to +the covdata subprocess path wholesale, so an incompatible future toolchain +costs time, never correctness. A parity test drives the real toolchain end to +end and requires the decoded blocks to equal the textfmt path's exactly. + +What the subprocess path paid per window was one spawn plus a re-read of the +package's whole-closure meta plus a text profile dominated by zero-count +lines the parser then discarded — a cost that scales with tests × closure +size, which is exactly the product that buries large workspaces. Measured on a +synthetic workspace of 90 packages where each of 30 test packages closes over +61 instrumented packages (600 tests, warm build caches, 4 cores): + +| Path | Wall | user+sys CPU | +|---|---|---| +| covdata subprocess per window | 13.1s | 41.3s | +| in-process decoding | **8.5s** | **25.7s** | + +At a 201-package closure the same shape gives 8.7s against 5.7s. The remaining +time is compile, link and test execution; the per-test conversion tax is gone. **The full 353-package index was not measured here, and the reason is worth recording.** The first attempt drove load average to 30 on a 4-core box and was diff --git a/internal/cli/index.go b/internal/cli/index.go index d2d06fa..8d74351 100644 --- a/internal/cli/index.go +++ b/internal/cli/index.go @@ -96,7 +96,7 @@ func newIndexCommand(opts *options) *cobra.Command { Jobs: jobs, Timeout: timeout, Packages: targets, - Progress: newProgress(cmd.ErrOrStderr(), quiet, opts.painter()), + Progress: newProgress(cmd.ErrOrStderr(), quiet, opts.errPainter()), Legacy: legacy, }) if err != nil { @@ -108,7 +108,8 @@ func newIndexCommand(opts *options) *cobra.Command { }, } - cmd.Flags().IntVar(&jobs, "jobs", 0, "packages to index concurrently (default: GOMAXPROCS)") + cmd.Flags().IntVar(&jobs, "jobs", 0, + "packages to index concurrently (default: half of GOMAXPROCS; each worker builds with its share of the cores)") cmd.Flags().DurationVar(&timeout, "timeout", 0, "per-test timeout (default: 60s)") cmd.Flags().StringSliceVar(&packages, "packages", nil, "limit indexing to these import paths") cmd.Flags().BoolVar(&quiet, "quiet", false, "suppress progress output") diff --git a/internal/cli/mutate.go b/internal/cli/mutate.go index 1566eb6..f5b4b24 100644 --- a/internal/cli/mutate.go +++ b/internal/cli/mutate.go @@ -146,7 +146,7 @@ func runMutate(cmd *cobra.Command, opts *options, flags *mutateFlags) error { mutants = mutate.Exclude(mutants, preflight.Failing) } - execOpts.Progress = newProgress(cmd.ErrOrStderr(), flags.quiet, opts.painter()) + execOpts.Progress = newProgress(cmd.ErrOrStderr(), flags.quiet, opts.errPainter()) results, err := mutate.Execute(ctx, mutants, execOpts) if err != nil { diff --git a/internal/cli/root.go b/internal/cli/root.go index 8fea0a4..5093074 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -72,14 +72,30 @@ type options struct { noColor bool } +func (o *options) colorEnabled() bool { + return !o.noColor && os.Getenv("NO_COLOR") == "" +} + +// useColor answers for stdout-bound output and useColorErr for stderr-bound: +// the two can disagree under redirection — `assay run 2>log` must not write +// escapes into the log because stdout is still a terminal, and `assay run +// >results` must not strip color from a summary still heading to one. func (o *options) useColor() bool { - return !o.noColor && os.Getenv("NO_COLOR") == "" && isTerminal(os.Stdout) + return o.colorEnabled() && isTerminal(os.Stdout) +} + +func (o *options) useColorErr() bool { + return o.colorEnabled() && isTerminal(os.Stderr) } func (o *options) painter() ui.Painter { return ui.New(o.useColor()) } +func (o *options) errPainter() ui.Painter { + return ui.New(o.useColorErr()) +} + func NewRootCommand() *cobra.Command { opts := &options{} diff --git a/internal/cli/run.go b/internal/cli/run.go index f34097d..977c0a7 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -27,7 +27,7 @@ func newRunCommand(opts *options) *cobra.Command { return err } - printer := report.NewPrinter(cmd.ErrOrStderr(), opts.useColor()) + printer := report.NewPrinter(cmd.ErrOrStderr(), opts.useColorErr()) if err := printer.Summary(summary, opts.verbose); err != nil { return err } diff --git a/internal/cli/verify.go b/internal/cli/verify.go index 05d24f0..1b46334 100644 --- a/internal/cli/verify.go +++ b/internal/cli/verify.go @@ -31,7 +31,7 @@ func newVerifyCommand(opts *options) *cobra.Command { return err } - printer := report.NewPrinter(cmd.ErrOrStderr(), opts.useColor()) + printer := report.NewPrinter(cmd.ErrOrStderr(), opts.useColorErr()) if err := printer.Summary(summary, opts.verbose); err != nil { return err } diff --git a/internal/cover/classify.go b/internal/cover/classify.go index 637bdc0..dac3c7c 100644 --- a/internal/cover/classify.go +++ b/internal/cover/classify.go @@ -315,7 +315,7 @@ func mergeBlocksOrEmpty(blocks []Block) []Block { return nil } - return mergeBlocks(blocks) + return MergeBlocks(blocks) } func splitLines(source []byte) []string { diff --git a/internal/cover/profile.go b/internal/cover/profile.go index a14844a..84bedeb 100644 --- a/internal/cover/profile.go +++ b/internal/cover/profile.go @@ -43,7 +43,7 @@ func ParseProfile(reader io.Reader) ([]FileBlocks, error) { if len(blocks) == 0 { continue } - out = append(out, FileBlocks{ProfileName: profile.FileName, Blocks: mergeBlocks(blocks)}) + out = append(out, FileBlocks{ProfileName: profile.FileName, Blocks: MergeBlocks(blocks)}) } sort.Slice(out, func(i, j int) bool { return out[i].ProfileName < out[j].ProfileName }) @@ -51,7 +51,12 @@ func ParseProfile(reader io.Reader) ([]FileBlocks, error) { return out, nil } -func mergeBlocks(blocks []Block) []Block { +// MergeBlocks sorts blocks and coalesces overlapping or line-adjacent ones, +// the same normalisation ParseProfile applies to text profiles. Callers that +// assemble blocks from another source (the binary covdata decoder) use it so +// both paths emit identical shapes. The input must be non-empty; it is +// modified in place. +func MergeBlocks(blocks []Block) []Block { sort.Slice(blocks, func(i, j int) bool { if blocks[i].StartLine != blocks[j].StartLine { return blocks[i].StartLine < blocks[j].StartLine diff --git a/internal/flake/flake_test.go b/internal/flake/flake_test.go index 678fdae..731c576 100644 --- a/internal/flake/flake_test.go +++ b/internal/flake/flake_test.go @@ -1,6 +1,7 @@ package flake import ( + "fmt" "os" "path/filepath" "sync" @@ -175,6 +176,33 @@ func TestCompactionKeepsTheNewestObservations(t *testing.T) { assert.Equal(t, int64(50), minBusy, "the newest observations are the ones kept") } +// TestCompactionKeepsTheNewestAppendsOnTimestampTies pins the tie order: a +// preflight stamps one clock reading across a whole batch, so a group over the +// cap can be all-ties — and compaction must keep the latest appends, not the +// order the file happened to serve them in. +func TestCompactionKeepsTheNewestAppendsOnTimestampTies(t *testing.T) { + j, err := OpenJournal(t.TempDir(), "/root") + require.NoError(t, err) + + var all []Observation + for i := range maxPerTest + 10 { + o := obs("p", "TestTied", "fp", VerdictPass, 7) + o.Detail = fmt.Sprintf("append-%03d", i) + all = append(all, o) + } + require.NoError(t, j.Record(all)) + + require.NoError(t, j.compactLocked()) + + got, err := j.Load() + require.NoError(t, err) + require.Len(t, got, maxPerTest) + for _, o := range got { + assert.NotEqual(t, "append-000", o.Detail, + "the oldest appends are the ones compaction must discard") + } +} + func TestParseVerboseOutputTakesTopLevelVerdictsOnly(t *testing.T) { output := []byte(`=== RUN TestAlpha --- PASS: TestAlpha (0.00s) diff --git a/internal/flake/journal.go b/internal/flake/journal.go index c0569b4..1601d9d 100644 --- a/internal/flake/journal.go +++ b/internal/flake/journal.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "slices" "sort" "github.com/zeebo/blake3" @@ -162,6 +163,11 @@ func (j *Journal) compactLocked() error { kept := make([]Observation, 0, len(observations)) for _, group := range grouped { + // The group arrives in append order, oldest first, and observations + // batched in one write share a timestamp. Reversing before the stable + // sort makes the newest appends win ties; without it a group trimmed + // at the cap kept its stalest evidence and discarded its newest. + slices.Reverse(group) sort.SliceStable(group, func(i, j int) bool { return group[i].Unix > group[j].Unix }) if len(group) > maxPerTest { group = group[:maxPerTest] diff --git a/internal/index/collect_test.go b/internal/index/collect_test.go index 6a2167e..12aba30 100644 --- a/internal/index/collect_test.go +++ b/internal/index/collect_test.go @@ -222,6 +222,43 @@ func TestFingerprintDiffersPerPackage(t *testing.T) { assert.Equal(t, calc, fp.For(testfixture.Module+"/calc"), "fingerprints must be stable") } +func TestFingerprintTracksWorkspaceRoot(t *testing.T) { + h := setup(t) + other := setup(t) + + require.NotEqual(t, h.root, other.root) + + same := index.NewFingerprinter(h.graph, h.digests, nil).For(testfixture.Module + "/calc") + moved := index.NewFingerprinter(other.graph, other.digests, nil).For(testfixture.Module + "/calc") + + assert.NotEqual(t, same, moved, + "records store absolute paths, so an identical tree at another root must not share records") +} + +// TestFingerprintScopesTestdataModuleFilesToTheirPackage pins the ownership +// rule for fixture module files: a go.mod under a package's testdata belongs +// to that package. Classified as unowned it was hashed into every package's +// fingerprint, and editing one fixture invalidated the entire index. +func TestFingerprintScopesTestdataModuleFilesToTheirPackage(t *testing.T) { + h := setup(t) + + before := index.NewFingerprinter(h.graph, h.digests, nil) + calcBefore := before.For(testfixture.Module + "/calc") + repoBefore := before.For(testfixture.Module + "/repo") + + digests := make(map[string]string, len(h.digests)+1) + for path, digest := range h.digests { + digests[path] = digest + } + digests["calc/testdata/proj/go.mod"] = "0000000000000000000000000000000000000001" + + after := index.NewFingerprinter(h.graph, digests, nil) + assert.NotEqual(t, calcBefore, after.For(testfixture.Module+"/calc"), + "the fixture belongs to calc and must invalidate it") + assert.Equal(t, repoBefore, after.For(testfixture.Module+"/repo"), + "a calc fixture must not invalidate unrelated packages") +} + func TestFingerprintTracksDependencies(t *testing.T) { h := setup(t) before := index.NewFingerprinter(h.graph, h.digests, nil).For(testfixture.Module + "/svc") diff --git a/internal/index/covdata.go b/internal/index/covdata.go new file mode 100644 index 0000000..baa3ecf --- /dev/null +++ b/internal/index/covdata.go @@ -0,0 +1,489 @@ +package index + +// In-process decoding of the Go toolchain's binary coverage files — the +// covmeta blob written once per package and the covcounters blob written per +// window. Shelling out to `go tool covdata textfmt` for every window meant one +// subprocess per test, each re-reading the package's whole-closure meta and +// serialising a text profile dominated by zero-count lines that the parser +// then discarded. On a large workspace that round-trip is the indexing wall +// clock. Decoding here reads the meta once per package and touches only the +// units a window actually executed. +// +// The formats are internal to the Go toolchain (src/internal/coverage/defs.go) +// and version-stamped: meta files carry MetaFileVersion 1, counter files +// CounterFileVersion 1, unchanged since Go 1.20. Every read is bounds-checked +// and every structural surprise — future version, foreign magic, hash +// mismatch, truncation — returns an error rather than a guess, and the caller +// falls back to the covdata subprocess path, which stays first-class. Being +// wrong here can cost a fallback; it must never cost a wrong record. + +import ( + "encoding/binary" + "errors" + "fmt" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/emoss08/assay/internal/cover" +) + +var ( + covMetaMagic = [4]byte{0x00, 'c', 'v', 'm'} + covCounterMagic = [4]byte{0x00, 'c', 'w', 'm'} +) + +const ( + covMetaVersion = 1 + covCounterVersion = 1 + + // Counter flavors: how function records encode their integers. + ctrRaw = 1 + ctrULeb128 = 2 + + // Counter granularities. Per-block is what -covermode emits; a per-func + // meta attributes one counter to every unit of the function. + granularityPerBlock = 1 + granularityPerFunc = 2 + + counterFilePrefix = "covcounters." + metaFilePrefix = "covmeta." +) + +// errCorrupt is the base of every structural complaint; callers only ever +// treat decoding as all-or-nothing, so one sentinel is enough. +var errCorrupt = errors.New("unexpected coverage data layout") + +// covMeta is one package's decoded meta-data file: the function tables for +// every instrumented package linked into the test binary, indexed by the +// package ID that counter records refer to. +type covMeta struct { + hash [16]byte + perFunc bool + packages []covMetaPackage +} + +type covMetaPackage struct { + funcs []covMetaFunc +} + +// covMetaFunc keeps only what window assembly consumes: the profile-style +// source name ("importpath/file.go", exactly what covdata textfmt prints) and +// each unit's line range, positionally matched to the function's counters. +type covMetaFunc struct { + srcFile string + units []covUnit +} + +type covUnit struct { + start, end uint32 +} + +// byteReader reads little-endian scalars from an in-memory file with sticky +// error state, so decode paths read straight through and check once. The +// stdlib's slicereader panics on truncated input; these files come from disk +// and truncation must surface as an error. +type byteReader struct { + b []byte + off int + err error +} + +func (r *byteReader) fail() { + if r.err == nil { + r.err = errCorrupt + } +} + +func (r *byteReader) bytes(n int) []byte { + if r.err != nil || n < 0 || r.off+n > len(r.b) { + r.fail() + + return nil + } + out := r.b[r.off : r.off+n] + r.off += n + + return out +} + +func (r *byteReader) u8() uint8 { + b := r.bytes(1) + if b == nil { + return 0 + } + + return b[0] +} + +func (r *byteReader) u32() uint32 { + b := r.bytes(4) + if b == nil { + return 0 + } + + return binary.LittleEndian.Uint32(b) +} + +func (r *byteReader) u32be() uint32 { + b := r.bytes(4) + if b == nil { + return 0 + } + + return binary.BigEndian.Uint32(b) +} + +func (r *byteReader) u64() uint64 { + b := r.bytes(8) + if b == nil { + return 0 + } + + return binary.LittleEndian.Uint64(b) +} + +func (r *byteReader) uleb() uint64 { + var value uint64 + var shift uint + for { + b := r.bytes(1) + if b == nil { + return 0 + } + value |= uint64(b[0]&0x7f) << shift + if b[0]&0x80 == 0 { + return value + } + shift += 7 + if shift > 63 { + r.fail() + + return 0 + } + } +} + +func (r *byteReader) seek(off int) { + if r.err != nil || off < 0 || off > len(r.b) { + r.fail() + + return + } + r.off = off +} + +// align4 skips padding up to the next 4-byte file offset, mirroring the +// preamble padding the counter writer inserts. +func (r *byteReader) align4() { + if rem := r.off % 4; rem != 0 { + r.bytes(4 - rem) + } +} + +// readStringTable decodes the uleb-prefixed string table both file kinds +// embed: entry count, then length-prefixed bytes per entry. +func (r *byteReader) readStringTable() []string { + count := r.uleb() + if r.err != nil || count > uint64(len(r.b)) { + r.fail() + + return nil + } + out := make([]string, 0, count) + for range count { + out = append(out, string(r.bytes(int(r.uleb())))) + } + + return out +} + +// readMetaDir locates and decodes the single covmeta file the harness wrote +// into the package's meta directory. +func readMetaDir(dir string) (*covMeta, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("read meta directory: %w", err) + } + + for _, entry := range entries { + if entry.IsDir() || !strings.HasPrefix(entry.Name(), metaFilePrefix) { + continue + } + payload, readErr := os.ReadFile(filepath.Join(dir, entry.Name())) + if readErr != nil { + return nil, fmt.Errorf("read meta file: %w", readErr) + } + + return decodeMeta(payload) + } + + return nil, fmt.Errorf("%w: no %s* file in %s", errCorrupt, metaFilePrefix, dir) +} + +func decodeMeta(payload []byte) (*covMeta, error) { + r := &byteReader{b: payload} + + var magic [4]byte + copy(magic[:], r.bytes(4)) + if magic != covMetaMagic { + return nil, fmt.Errorf("%w: not a coverage meta file", errCorrupt) + } + if version := r.u32(); version != covMetaVersion { + return nil, fmt.Errorf("%w: meta file version %d, can decode %d", + errCorrupt, version, covMetaVersion) + } + + totalLength := r.u64() + entries := r.u64() + if totalLength > uint64(len(payload)) || entries > uint64(len(payload)) { + return nil, errCorrupt + } + + meta := &covMeta{} + copy(meta.hash[:], r.bytes(16)) + + r.u32() // string table offset; the file-level table is not consulted + r.u32() // string table length + r.u8() // counter mode + meta.perFunc = r.u8() == granularityPerFunc + r.bytes(6) // header padding + + offsets := make([]uint64, entries) + for i := range offsets { + offsets[i] = r.u64() + } + lengths := make([]uint64, entries) + for i := range lengths { + lengths[i] = r.u64() + } + if r.err != nil { + return nil, r.err + } + + meta.packages = make([]covMetaPackage, entries) + for i := range meta.packages { + end := offsets[i] + lengths[i] + if offsets[i] > uint64(len(payload)) || end < offsets[i] || end > uint64(len(payload)) { + return nil, errCorrupt + } + pkg, err := decodeMetaPackage(payload[offsets[i]:end]) + if err != nil { + return nil, err + } + meta.packages[i] = pkg + } + + return meta, nil +} + +// decodeMetaPackage decodes one package's self-contained meta blob: symbol +// header, function offset table, package-local string table, then per-function +// unit lists. +func decodeMetaPackage(blob []byte) (covMetaPackage, error) { + const symbolHeaderSize = 44 // coverage.CovMetaHeaderSize + + r := &byteReader{b: blob} + if length := r.u32(); uint64(length) != uint64(len(blob)) { + return covMetaPackage{}, errCorrupt + } + r.u32() // package name index + r.u32() // package path index + r.u32() // module path index + r.bytes(16) + r.bytes(4) // one unused byte plus padding + r.u32() // file count; files resolve through the string table + numFuncs := r.u32() + if r.err != nil || r.off != symbolHeaderSize || uint64(numFuncs) > uint64(len(blob)) { + return covMetaPackage{}, errCorrupt + } + + funcOffsets := make([]uint32, numFuncs) + for i := range funcOffsets { + funcOffsets[i] = r.u32() + } + + strings := r.readStringTable() + if r.err != nil { + return covMetaPackage{}, r.err + } + + pkg := covMetaPackage{funcs: make([]covMetaFunc, numFuncs)} + for i, off := range funcOffsets { + r.seek(int(off)) + numUnits := r.uleb() + nameIdx := r.uleb() + fileIdx := r.uleb() + if r.err != nil || numUnits > uint64(len(blob)) || + nameIdx >= uint64(len(strings)) || fileIdx >= uint64(len(strings)) { + return covMetaPackage{}, errCorrupt + } + + fn := covMetaFunc{srcFile: strings[fileIdx], units: make([]covUnit, numUnits)} + for u := range fn.units { + stLine := r.uleb() + r.uleb() // start column + enLine := r.uleb() + r.uleb() // end column + r.uleb() // statement count + fn.units[u] = covUnit{start: uint32(stLine), end: uint32(enLine)} + } + if r.err != nil { + return covMetaPackage{}, r.err + } + pkg.funcs[i] = fn + } + + return pkg, nil +} + +// windowBlocks decodes every counter file in one window directory and returns +// the executed source blocks in the same shape ParseProfile produces from a +// text profile: per-file, count-filtered, merged, and sorted by profile name. +func (m *covMeta) windowBlocks(dir string) ([]cover.FileBlocks, error) { + entries, err := os.ReadDir(dir) + if err != nil { + return nil, fmt.Errorf("read counter directory: %w", err) + } + + perFile := make(map[string][]cover.Block) + found := false + for _, entry := range entries { + if entry.IsDir() || !strings.HasPrefix(entry.Name(), counterFilePrefix) { + continue + } + found = true + payload, readErr := os.ReadFile(filepath.Join(dir, entry.Name())) + if readErr != nil { + return nil, fmt.Errorf("read counter file: %w", readErr) + } + if decodeErr := m.accumulateCounters(payload, perFile); decodeErr != nil { + return nil, decodeErr + } + } + if !found { + return nil, fmt.Errorf("%w: no %s* file in %s", errCorrupt, counterFilePrefix, dir) + } + + out := make([]cover.FileBlocks, 0, len(perFile)) + for file, blocks := range perFile { + out = append(out, cover.FileBlocks{ProfileName: file, Blocks: cover.MergeBlocks(blocks)}) + } + sort.Slice(out, func(i, j int) bool { return out[i].ProfileName < out[j].ProfileName }) + + return out, nil +} + +// accumulateCounters walks one counter file's segments and records the line +// ranges of every unit with a nonzero counter. +func (m *covMeta) accumulateCounters(payload []byte, perFile map[string][]cover.Block) error { + const headerSize = 32 + const footerSize = 16 + + r := &byteReader{b: payload} + + var magic [4]byte + copy(magic[:], r.bytes(4)) + if magic != covCounterMagic { + return fmt.Errorf("%w: not a coverage counter file", errCorrupt) + } + if version := r.u32(); version != covCounterVersion { + return fmt.Errorf("%w: counter file version %d, can decode %d", + errCorrupt, version, covCounterVersion) + } + + var hash [16]byte + copy(hash[:], r.bytes(16)) + if hash != m.hash { + return fmt.Errorf("%w: counter file does not match the meta file", errCorrupt) + } + + flavor := r.u8() + bigEndian := r.u8() != 0 + r.bytes(6) // header padding + + var next func() uint32 + switch flavor { + case ctrULeb128: + next = func() uint32 { return uint32(r.uleb()) } + case ctrRaw: + if bigEndian { + next = r.u32be + } else { + next = r.u32 + } + default: + return fmt.Errorf("%w: unknown counter flavor %d", errCorrupt, flavor) + } + + if len(payload) < headerSize+footerSize { + return errCorrupt + } + footer := &byteReader{b: payload, off: len(payload) - footerSize} + copy(magic[:], footer.bytes(4)) + footer.bytes(4) + segments := footer.u32() + if magic != covCounterMagic || footer.err != nil { + return errCorrupt + } + + for segment := range segments { + if segment > 0 { + r.bytes(footerSize) // the previous segment's footer + } + + fcnEntries := r.u64() + strTabLen := r.u32() + argsLen := r.u32() + if r.err != nil || fcnEntries > uint64(len(payload)) { + return errCorrupt + } + r.bytes(int(strTabLen)) + r.bytes(int(argsLen)) + r.align4() + + for range fcnEntries { + numCounters := next() + pkgIdx := next() + funcIdx := next() + if r.err != nil { + return r.err + } + + // An index outside the meta is ignored rather than fatal — the + // same policy covdata itself applies to inconsistent payloads. + var fn *covMetaFunc + if int(pkgIdx) < len(m.packages) && + int(funcIdx) < len(m.packages[pkgIdx].funcs) { + fn = &m.packages[pkgIdx].funcs[funcIdx] + } + + for i := range int(numCounters) { + count := next() + if r.err != nil { + return r.err + } + if count == 0 || fn == nil { + continue + } + if m.perFunc { + // One counter stands for the whole function. + for _, unit := range fn.units { + perFile[fn.srcFile] = append(perFile[fn.srcFile], + cover.Block{StartLine: int(unit.start), EndLine: int(unit.end)}) + } + + continue + } + if i < len(fn.units) { + unit := fn.units[i] + perFile[fn.srcFile] = append(perFile[fn.srcFile], + cover.Block{StartLine: int(unit.start), EndLine: int(unit.end)}) + } + } + } + } + + return r.err +} diff --git a/internal/index/covdata_test.go b/internal/index/covdata_test.go new file mode 100644 index 0000000..807c6ca --- /dev/null +++ b/internal/index/covdata_test.go @@ -0,0 +1,181 @@ +package index + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/emoss08/assay/internal/overlay" +) + +// TestDecodeWindowsAgreesWithCovdataTextfmt is the contract for the in-process +// covdata decoder: for the same counter directory, decodeWindows and the +// `go tool covdata textfmt` subprocess path must produce identical blocks. It +// drives the real toolchain — a real instrumented build, a real harness run — +// so a Go release that changes the binary formats fails this test rather than +// silently corrupting records; decoding then rejects the version and every +// caller falls back to the subprocess path. +func TestDecodeWindowsAgreesWithCovdataTextfmt(t *testing.T) { + t.Parallel() + + root := t.TempDir() + writeCovdataFixture(t, root) + + workdir := t.TempDir() + injected := filepath.Join(root, "fixture", HarnessFileName) + harnessPath, err := overlay.WriteUnderBasename(workdir, "harness", injected, HarnessSource("fixture")) + require.NoError(t, err) + overlayPath, err := overlay.WriteFile(workdir, map[string]string{injected: harnessPath}) + require.NoError(t, err) + + ctx := context.Background() + opts := Options{Root: root} + + binary := filepath.Join(workdir, "harness.bin") + require.NoError(t, buildTestBinary( + ctx, opts, "example.com/covfixture/...", "example.com/covfixture/fixture", binary, overlayPath, 2)) + + dir := filepath.Join(root, "fixture") + names, err := listTests(ctx, opts, binary, dir) + require.NoError(t, err) + require.NotEmpty(t, names) + + listFile := filepath.Join(workdir, "tests.list") + require.NoError(t, os.WriteFile(listFile, []byte(strings.Join(names, "\n")+"\n"), 0o644)) + + counterDir := filepath.Join(workdir, "counters") + require.NoError(t, os.MkdirAll(filepath.Join(counterDir, "assay-final"), 0o755)) + + run := runHarness(ctx, opts, binary, dir, listFile, counterDir, names) + require.NoError(t, run.err) + require.Len(t, run.completed, len(names)) + + windows := make([]string, 0, len(names)+1) + windows = append(windows, initWindowName) + windows = append(windows, names...) + + decoded, err := decodeWindows(counterDir, windows) + require.NoError(t, err) + + converted, err := convertWindows(ctx, opts, counterDir, windows, 2) + require.NoError(t, err) + + require.Equal(t, converted, decoded, + "in-process covdata decoding must match `go tool covdata textfmt` exactly") + + // The decoder must have seen real coverage, not agreed on emptiness. + total := 0 + for _, blocks := range decoded { + total += len(blocks) + } + require.Positive(t, total) +} + +func TestDecodeMetaRejectsForeignPayloads(t *testing.T) { + t.Parallel() + + _, err := decodeMeta([]byte("not a meta file at all")) + require.Error(t, err) + + _, err = decodeMeta(nil) + require.Error(t, err) + + // A valid magic with a future version must refuse, not guess. + future := append([]byte{0x00, 'c', 'v', 'm'}, []byte{2, 0, 0, 0}...) + future = append(future, make([]byte, 64)...) + _, err = decodeMeta(future) + require.ErrorContains(t, err, "version") +} + +func TestAccumulateCountersRejectsForeignPayloads(t *testing.T) { + t.Parallel() + + meta := &covMeta{} + + require.Error(t, meta.accumulateCounters([]byte("junk"), nil)) + + // Valid magic and version but a mismatched meta hash must refuse: pairing + // counters with a meta they were not produced against fabricates coverage. + mismatched := append([]byte{0x00, 'c', 'w', 'm'}, []byte{1, 0, 0, 0}...) + mismatched = append(mismatched, []byte("0123456789abcdef")...) + mismatched = append(mismatched, make([]byte, 40)...) + require.Error(t, meta.accumulateCounters(mismatched, nil)) +} + +// writeCovdataFixture lays out a module whose coverage exercises the decoder's +// interesting axes: init-time execution, multiple files in one package, a +// dependency package pulled in through -coverpkg, per-test disjoint execution, +// and a skipped test with an empty window. +func writeCovdataFixture(t *testing.T, root string) { + t.Helper() + + files := map[string]string{ + "go.mod": "module example.com/covfixture\n\ngo 1.26\n", + + "dep/dep.go": `package dep + +func Double(v int) int { + return v * 2 +} +`, + "fixture/a.go": `package fixture + +import "example.com/covfixture/dep" + +var initialised = seed() + +func seed() int { + return dep.Double(21) +} + +func Left(v int) int { + if v > 0 { + return v + initialised + } + + return -v +} +`, + "fixture/b.go": `package fixture + +func Right(v int) int { + out := 0 + for i := 0; i < v; i++ { + out += i + } + + return out +} +`, + "fixture/fixture_test.go": `package fixture + +import "testing" + +func TestLeft(t *testing.T) { + if Left(1) != 43 { + t.Fatal("bad") + } +} + +func TestRight(t *testing.T) { + if Right(3) != 3 { + t.Fatal("bad") + } +} + +func TestSkipped(t *testing.T) { + t.Skip("nothing to see") +} +`, + } + + for name, content := range files { + path := filepath.Join(root, filepath.FromSlash(name)) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte(content), 0o644)) + } +} diff --git a/internal/index/fingerprint.go b/internal/index/fingerprint.go index 7e71b9b..c5b5ebd 100644 --- a/internal/index/fingerprint.go +++ b/internal/index/fingerprint.go @@ -25,10 +25,16 @@ func NewFingerprinter(g *graph.Graph, treeDigests map[string]string, tags []stri sorted := append([]string(nil), tags...) sort.Strings(sorted) + // The workspace root is part of the fingerprint because records intern + // absolute paths: a record built at /a/repo answers Knows() with /a/repo + // paths only. Without the root, a second checkout sharing the cache + // directory gets byte-identical fingerprints, hits records whose every + // path misses, and silently narrows to nothing. f := &Fingerprinter{ - graph: g, - byPkg: make(map[string][]string), - toolchain: cache.BuildIdentity() + "|" + runtime.Version() + "|" + strings.Join(sorted, ","), + graph: g, + byPkg: make(map[string][]string), + toolchain: cache.BuildIdentity() + "|" + runtime.Version() + "|" + + g.Root + "|" + strings.Join(sorted, ","), } paths := make([]string, 0, len(treeDigests)) @@ -44,7 +50,11 @@ func NewFingerprinter(g *graph.Graph, treeDigests map[string]string, tags []stri entry := path + " " + treeDigests[path] abs := filepath.Join(g.Root, filepath.FromSlash(path)) - if isModuleFile(path) { + // A module file under testdata is a fixture the build never reads; it + // attributes to its owning package below like any other fixture file. + // Classified as unowned it was hashed into every package's + // fingerprint, so editing one fixture invalidated the whole index. + if isModuleFile(path) && !underTestdataPath(path) { f.unowned = append(f.unowned, entry) continue @@ -66,6 +76,12 @@ func isRelevantPath(path string) bool { strings.Contains(path, "/testdata/") } +// underTestdataPath reports whether a slash-relative tree path has a testdata +// element, which the Go build system never looks inside. +func underTestdataPath(path string) bool { + return strings.HasPrefix(path, "testdata/") || strings.Contains(path, "/testdata/") +} + func isModuleFile(path string) bool { switch filepath.Base(path) { case "go.mod", "go.sum", "go.work", "go.work.sum": diff --git a/internal/index/windows.go b/internal/index/windows.go index cbdc9ae..5a13849 100644 --- a/internal/index/windows.go +++ b/internal/index/windows.go @@ -11,8 +11,15 @@ import ( "github.com/emoss08/assay/internal/cover" ) -// assembleWindows converts the completed counter windows to text profiles and -// builds the record from them. +// assembleWindows converts the completed counter windows to executed source +// blocks and builds the record from them. +// +// The fast path decodes the binary covmeta/covcounters files in this process: +// the meta is read once per package and each window costs one small counter +// file, instead of one `go tool covdata textfmt` subprocess per test each +// re-reading the meta and serialising a mostly-zero text profile. Any decoding +// surprise falls back to the subprocess path wholesale — the two produce +// identical blocks, so the fallback costs time, never correctness. // // The init window — everything the package executed before the first test — is // merged into every test's ranges, and merged *before* the empty-ranges check @@ -33,8 +40,12 @@ func assembleWindows( windows = append(windows, progress.name) } - if err := convertWindows(ctx, opts, counterDir, windows, buildParallelism); err != nil { - return nil, err + blocks, err := decodeWindows(counterDir, windows) + if err != nil { + blocks, err = convertWindows(ctx, opts, counterDir, windows, buildParallelism) + if err != nil { + return nil, err + } } record := &Record{Version: recordVersion, Package: importPath, IndexedAt: opts.Commit} @@ -47,17 +58,11 @@ func assembleWindows( } } - initRanges, unresolved, err := parseWindow(counterDir, initWindowName, build, resolve) - if err != nil { - return nil, err - } + initRanges, unresolved := blocksToRanges(blocks[0], build, resolve) degrade(unresolved) - for _, progress := range completed { - ranges, unresolvedTest, parseErr := parseWindow(counterDir, progress.name, build, resolve) - if parseErr != nil { - return nil, parseErr - } + for i, progress := range completed { + ranges, unresolvedTest := blocksToRanges(blocks[i+1], build, resolve) degrade(unresolvedTest) ranges = append(ranges, initRanges...) @@ -75,24 +80,54 @@ func assembleWindows( return record, nil } -// convertWindows runs `go tool covdata textfmt` for each window, pairing each -// counters-only window with the package's single meta directory — covdata -// matches them by hash across input directories. +// decodeWindows is the in-process path: one meta decode, then one counter-file +// decode per window. Windows are independent and the meta is read-only, so a +// package with thousands of tests still assembles quickly; the work per window +// is proportional to what the test executed, not to the closure. +func decodeWindows(counterDir string, windows []string) ([][]cover.FileBlocks, error) { + meta, err := readMetaDir(filepath.Join(counterDir, metaDirName)) + if err != nil { + return nil, err + } + + out := make([][]cover.FileBlocks, len(windows)) + for i, window := range windows { + blocks, decodeErr := meta.windowBlocks(filepath.Join(counterDir, window)) + if decodeErr != nil { + return nil, decodeErr + } + out[i] = blocks + } + + return out, nil +} + +// convertWindows is the subprocess fallback: `go tool covdata textfmt` for +// each window, pairing each counters-only window with the package's single +// meta directory — covdata matches them by hash across input directories. // // The pool is bounded by the caller's share of the cores, not by GOMAXPROCS: // this runs inside one of `jobs` package workers, and an inner bound that // ignores the outer one multiplies into cores²/4 concurrent covdata processes — // the exact trap the build path in this package already documents. -func convertWindows(ctx context.Context, opts Options, counterDir string, windows []string, buildParallelism int) error { +func convertWindows( + ctx context.Context, + opts Options, + counterDir string, + windows []string, + buildParallelism int, +) ([][]cover.FileBlocks, error) { workers := min(max(1, buildParallelism), len(windows)) + out := make([][]cover.FileBlocks, len(windows)) + converting := pool.New(). WithMaxGoroutines(workers). WithContext(ctx). WithFirstError(). WithCancelOnError() - for _, window := range windows { + for i, window := range windows { converting.Go(func(ctx context.Context) error { if err := ctx.Err(); err != nil { return err @@ -106,29 +141,50 @@ func convertWindows(ctx context.Context, opts Options, counterDir string, window return fmt.Errorf("convert coverage window %s: %w", window, err) } + blocks, err := parseWindowFile(output) + if err != nil { + return err + } + out[i] = blocks + return nil }) } - return converting.Wait() + if err := converting.Wait(); err != nil { + return nil, err + } + + return out, nil } -func parseWindow( - counterDir, window string, - build *builder, - resolve cover.Resolver, -) ([]Range, string, error) { - file, err := os.Open(filepath.Join(counterDir, window) + ".txt") +func parseWindowFile(path string) ([]cover.FileBlocks, error) { + file, err := os.Open(path) if err != nil { - return nil, "", fmt.Errorf("open converted window %s: %w", window, err) + return nil, fmt.Errorf("open converted window %s: %w", filepath.Base(path), err) } defer file.Close() blocks, err := cover.ParseProfile(file) if err != nil { - return nil, "", fmt.Errorf("parse coverage window %s: %w", window, err) + return nil, fmt.Errorf("parse coverage window %s: %w", filepath.Base(path), err) } + return blocks, nil +} + +// rebuilderFrom continues a record's file interning where assembly left it, so +// salvaged tests share file ids with the harness-collected ones. +func rebuilderFrom(record *Record) *builder { + build := newBuilder() + for _, file := range record.Files { + build.intern(file) + } + + return build +} + +func blocksToRanges(blocks []cover.FileBlocks, build *builder, resolve cover.Resolver) ([]Range, string) { var ranges []Range var unresolved string for _, fileBlocks := range blocks { @@ -150,16 +206,5 @@ func parseWindow( } } - return ranges, unresolved, nil -} - -// rebuilderFrom continues a record's file interning where assembly left it, so -// salvaged tests share file ids with the harness-collected ones. -func rebuilderFrom(record *Record) *builder { - build := newBuilder() - for _, file := range record.Files { - build.intern(file) - } - - return build + return ranges, unresolved } diff --git a/internal/mutate/execute.go b/internal/mutate/execute.go index 944523d..8e03137 100644 --- a/internal/mutate/execute.go +++ b/internal/mutate/execute.go @@ -143,8 +143,13 @@ func Execute(ctx context.Context, mutants []Mutant, opts ExecuteOptions) ([]Resu } ready, declined := prepareBatches(batches) + // Cleanup iterates its own copy: `ready` is nil-ed below once the harness + // has judged the batches, and a defer over the live variable then swept + // nothing, leaking one workdir — overlays plus every compiled test binary — + // per batch of every run. + prepared := ready defer func() { - for _, batch := range ready { + for _, batch := range prepared { batch.cleanup() } }() diff --git a/internal/mutate/execute_test.go b/internal/mutate/execute_test.go index 92e720a..d389902 100644 --- a/internal/mutate/execute_test.go +++ b/internal/mutate/execute_test.go @@ -312,6 +312,13 @@ func requireNoBuildFailures(t *testing.T, results []mutate.Result) { } func TestStrongSuiteKillsEveryMutant(t *testing.T) { + // The run doubles as the leak check for schemata workdirs: pointing TMPDIR + // at a fresh directory makes any assay-schemata-* directory that survives + // Execute visible. The harness path used to nil the batch list before the + // deferred cleanup ran, leaking overlays and compiled test binaries. + tmp := t.TempDir() + t.Setenv("TMPDIR", tmp) + c := newCorpus(t) results, score := c.run(t, "strong") @@ -320,6 +327,13 @@ func TestStrongSuiteKillsEveryMutant(t *testing.T) { assert.Zero(t, score.Survived, "a fully asserted suite should leave no survivors") assert.Positive(t, score.Killed) assert.InDelta(t, 100.0, score.MSI(), 0.001) + + entries, err := os.ReadDir(tmp) + require.NoError(t, err) + for _, entry := range entries { + assert.NotContains(t, entry.Name(), "assay-schemata-", + "Execute must remove every schemata workdir it prepared") + } } func TestEquivalentMutantSurvivesAThoroughSuite(t *testing.T) { diff --git a/internal/mutate/generate.go b/internal/mutate/generate.go index 6c9804b..94c9ec2 100644 --- a/internal/mutate/generate.go +++ b/internal/mutate/generate.go @@ -109,8 +109,8 @@ func Generate(ctx context.Context, opts GenerateOptions) ([]Mutant, error) { } var out []Mutant - for i, file := range pkg.Syntax { - path := syntaxPath(pkg, i) + for _, file := range pkg.Syntax { + path := syntaxPath(pkg, file) if path == "" || strings.HasSuffix(path, "_test.go") { continue } @@ -149,15 +149,19 @@ func parseWithComments(fset *token.FileSet, filename string, src []byte) (*ast.F return parser.ParseFile(fset, filename, src, parser.ParseComments|parser.AllErrors) } -func syntaxPath(pkg *packages.Package, i int) string { - if i < len(pkg.CompiledGoFiles) && !strings.Contains(pkg.CompiledGoFiles[i], "go-build") { - return pkg.CompiledGoFiles[i] - } - if i < len(pkg.GoFiles) { - return pkg.GoFiles[i] +// syntaxPath returns the on-disk source behind one parsed file, read from the +// tree's own position information rather than by indexing a parallel file +// list — GoFiles and Syntax disagree in length and order for any cgo package, +// and a positional fallback spliced mutations into whatever file happened to +// sit at that index. A cgo-generated file lives in the build cache and +// describes text that exists nowhere in the tree, so it yields no mutants. +func syntaxPath(pkg *packages.Package, file *ast.File) string { + path := pkg.Fset.Position(file.FileStart).Filename + if path == "" || strings.Contains(path, "go-build") { + return "" } - return "" + return path } func mutantsInFile( diff --git a/internal/selection/narrow.go b/internal/selection/narrow.go index d5fb623..59a04ef 100644 --- a/internal/selection/narrow.go +++ b/internal/selection/narrow.go @@ -3,6 +3,7 @@ package selection import ( "path/filepath" "sort" + "strings" "github.com/emoss08/assay/internal/cover" "github.com/emoss08/assay/internal/graph" @@ -159,6 +160,16 @@ func partitionChanges(changes []vcs.Change) ([]fileChange, []string) { if len(classification.Executable) == 0 { continue } + // A test file is executable and narrowable by classification, but it can + // never be attributed: coverage instruments only production code, so no + // index record knows a _test.go path, and treating the change as + // attributable would silently select nothing — the edited test itself + // would not run. The package runs in full instead. + if strings.HasSuffix(filepath.Base(change.Path), "_test.go") { + blocking = append(blocking, change.Path) + + continue + } baseLines := change.BaseLineNumbers() if len(baseLines) == 0 { diff --git a/internal/selection/narrow_test.go b/internal/selection/narrow_test.go index 647fc73..8da0127 100644 --- a/internal/selection/narrow_test.go +++ b/internal/selection/narrow_test.go @@ -193,6 +193,26 @@ func TestNarrowFallsBackOnNonGoFile(t *testing.T) { "coverage tracks Go lines only") } +func TestNarrowFallsBackOnTestFileChange(t *testing.T) { + h := newNarrowHarness(t) + + // Line 6 is the body of TestAdd — executable, narrowable, and invisible to + // the index, which never contains _test.go files. Before test files were + // forced onto the blocking path this selected nothing at all, silently + // skipping the very test that was edited. + got := h.narrow(t, []vcs.Change{{ + Path: filepath.Join(h.root, "calc", "calc_test.go"), + Status: "M", + Lines: []vcs.LineRange{{Start: 6, End: 6}}, + BaseLines: []vcs.LineRange{{Start: 6, End: 6}}, + }}) + + plan := planFor(t, got, testfixture.Module+"/calc") + assert.NotEmpty(t, plan.FullReason, + "a test-body edit cannot be attributed by the index; the package must run in full") + assert.Contains(t, got.Blocked, filepath.Join(h.root, "calc", "calc_test.go")) +} + func TestNarrowSurvivesIndexFromAnotherCommit(t *testing.T) { h := newNarrowHarness(t) changes := []vcs.Change{h.calcChange(testfixture.CalcAddBodyLine)} diff --git a/internal/selection/selection.go b/internal/selection/selection.go index e5871c6..800fb18 100644 --- a/internal/selection/selection.go +++ b/internal/selection/selection.go @@ -3,6 +3,7 @@ package selection import ( "path/filepath" "sort" + "strings" "github.com/emoss08/assay/internal/graph" "github.com/emoss08/assay/internal/vcs" @@ -45,7 +46,11 @@ func Select(opts Options) Result { for _, change := range opts.Changes { base := filepath.Base(change.Path) - if _, isTrigger := globalTriggerFiles[base]; isTrigger { + // A go.mod under a testdata directory is a fixture, not a module + // boundary — the build ignores testdata entirely — so it attributes to + // its owning package like any other fixture file instead of forcing a + // whole-workspace run. + if _, isTrigger := globalTriggerFiles[base]; isTrigger && !underTestdata(change.Path) { return All(g, "module definition changed: "+base) } @@ -90,6 +95,18 @@ func Select(opts Options) Result { } } +// underTestdata reports whether any element of the path is a testdata +// directory, which the Go build system never looks inside. +func underTestdata(path string) bool { + for _, segment := range strings.Split(filepath.ToSlash(path), "/") { + if segment == "testdata" { + return true + } + } + + return false +} + func All(g *graph.Graph, reason string) Result { packages := g.TestablePackages() reasons := make(map[string]Reason, len(packages)) diff --git a/internal/selection/selection_test.go b/internal/selection/selection_test.go index 3eb9457..019c3a7 100644 --- a/internal/selection/selection_test.go +++ b/internal/selection/selection_test.go @@ -98,6 +98,19 @@ func TestSelectFallsBackToAllOnModuleChange(t *testing.T) { } } +// TestSelectTreatsTestdataModuleFilesAsFixtures pins the trigger's scope: the +// build never reads a go.mod under testdata, so editing one is a fixture +// change for its owning package, not a workspace-wide module event. +func TestSelectTreatsTestdataModuleFilesAsFixtures(t *testing.T) { + g, root := loadFixture(t) + + res := selectFor(t, g, root, "svc/testdata/proj/go.mod") + + assert.False(t, res.SelectAll, "a fixture go.mod must not force a full run") + assert.Equal(t, []string{mod + "/app", mod + "/svc"}, res.Packages, + "the fixture attributes to svc like any other testdata file") +} + func TestSelectFallsBackToAllOnUnattributedGoFile(t *testing.T) { g, root := loadFixture(t) diff --git a/internal/vcs/churn.go b/internal/vcs/churn.go index 4257944..79dce45 100644 --- a/internal/vcs/churn.go +++ b/internal/vcs/churn.go @@ -3,6 +3,7 @@ package vcs import ( "context" "fmt" + "path/filepath" "strconv" "strings" "time" @@ -20,9 +21,9 @@ type ChurnStat struct { const churnMarker = "\x01" // Churn reports per-file change activity over the trailing window, keyed by -// slash-separated path relative to the repository root. Only production Go -// files count: churn in tests is not production exposure, and churn in -// non-Go files is not this tool's business. +// slash-separated path relative to root. Only production Go files count: +// churn in tests is not production exposure, and churn in non-Go files is not +// this tool's business. func Churn(ctx context.Context, root string, window time.Duration) (map[string]ChurnStat, error) { since := fmt.Sprintf("--since=%d.seconds.ago", int64(window.Seconds())) out, err := runGit(ctx, root, @@ -31,6 +32,15 @@ func Churn(ctx context.Context, root string, window time.Duration) (map[string]C return nil, fmt.Errorf("read churn: %w", err) } + // git prints numstat paths relative to the repository toplevel no matter + // where it runs, while callers key lookups by root-relative paths. When + // root is a subdirectory of the repo the two disagree for every file, so + // every lookup missed and risk called everything stable. + prefix, err := gitPathPrefix(ctx, root) + if err != nil { + return nil, err + } + churn := make(map[string]ChurnStat) var commitUnix int64 seenThisCommit := make(map[string]struct{}) @@ -52,6 +62,10 @@ func Churn(ctx context.Context, root string, window time.Duration) (map[string]C if !ok || !isProductionGoFile(path) { continue } + path, inRoot := strings.CutPrefix(path, prefix) + if !inRoot { + continue + } stat := churn[path] if _, counted := seenThisCommit[path]; !counted { @@ -68,6 +82,28 @@ func Churn(ctx context.Context, root string, window time.Duration) (map[string]C return churn, nil } +// gitPathPrefix returns root's toplevel-relative prefix, slash-separated and +// "/"-terminated, or "" when root is the repository toplevel itself. Symlinks +// are resolved before comparing because rev-parse resolves them too. +func gitPathPrefix(ctx context.Context, root string) (string, error) { + toplevel, err := RepoRoot(ctx, root) + if err != nil { + return "", err + } + + resolved := root + if r, symErr := filepath.EvalSymlinks(root); symErr == nil { + resolved = r + } + + rel, err := filepath.Rel(toplevel, resolved) + if err != nil || rel == "." || strings.HasPrefix(rel, "..") { + return "", nil + } + + return filepath.ToSlash(rel) + "/", nil +} + // parseNumstat reads one "addeddeletedpath" line. Binary files // report "-" for both counts and still count as a change. func parseNumstat(line string) (added, deleted int, path string, ok bool) { diff --git a/internal/vcs/churn_test.go b/internal/vcs/churn_test.go index 4fd497d..05faf58 100644 --- a/internal/vcs/churn_test.go +++ b/internal/vcs/churn_test.go @@ -79,6 +79,21 @@ func TestChurnCountsProductionGoFilesOnly(t *testing.T) { assert.NotContains(t, churn, "README.md") } +// TestChurnKeysPathsRelativeToTheGivenRoot pins the contract the risk report +// depends on: git prints numstat paths relative to the repository toplevel, +// but callers analyzing a subdirectory key their lookups by root-relative +// paths. A mismatch made every lookup miss and reported all churn as zero. +func TestChurnKeysPathsRelativeToTheGivenRoot(t *testing.T) { + root := churnRepo(t) + + churn, err := vcs.Churn(t.Context(), filepath.Join(root, "pkg"), 30*24*time.Hour) + require.NoError(t, err) + + assert.Equal(t, 3, churn["hot.go"].Commits, + "paths must be relative to the analysis root, not the repo toplevel") + assert.NotContains(t, churn, "pkg/hot.go") +} + // TestChurnHonoursTheWindow pins the time bound: commits older than the window // contribute nothing, and an empty history is a result, not an error. func TestChurnHonoursTheWindow(t *testing.T) { diff --git a/internal/watch/binaries.go b/internal/watch/binaries.go index aa2fd91..a453012 100644 --- a/internal/watch/binaries.go +++ b/internal/watch/binaries.go @@ -77,6 +77,15 @@ func (c *BinaryCache) Ensure(ctx context.Context, req BuildRequest) (string, boo output := filepath.Join(c.dir, binaryName(req.ImportPath)) staging := output + ".tmp" + // A build killed mid-write leaves a truncated staging file behind. The next + // build usually overwrites it, but a package that has since lost its test + // files produces no output at all — and the no-output check below would + // then find the stale leftover and promote it as fresh. Clear the slate + // before building so whatever sits at the staging path afterwards was put + // there by this build. + if err := os.Remove(staging); err != nil && !os.IsNotExist(err) { + return "", false, fmt.Errorf("clear stale staging binary for %s: %w", req.ImportPath, err) + } args := []string{"test", "-c", "-o", staging} if req.BuildParallelism > 0 { diff --git a/internal/watch/cycle.go b/internal/watch/cycle.go index d11dbeb..65ab755 100644 --- a/internal/watch/cycle.go +++ b/internal/watch/cycle.go @@ -262,6 +262,12 @@ func (p *Planner) refineTestEdit( } func runnableByTestRun(name string) bool { + // TestMain passes the prefix check but is the harness entry point, not a + // test: -test.run never selects it, so narrowing to it runs zero tests and + // prints a green cycle for an edit to the code that sets up all of them. + if name == "TestMain" { + return false + } for _, prefix := range [...]string{"Test", "Fuzz"} { if rest, ok := strings.CutPrefix(name, prefix); ok { if rest == "" || rest[0] < 'a' || rest[0] > 'z' { diff --git a/internal/watch/flake_test.go b/internal/watch/flake_test.go index 46646cf..333462c 100644 --- a/internal/watch/flake_test.go +++ b/internal/watch/flake_test.go @@ -21,7 +21,17 @@ func TestCycleObservationsAttributeEveryKnownVerdict(t *testing.T) { ImportPath: "example.com/m/red", Tests: []string{"TestBroken", "TestFine"}, Passed: false, - Output: []byte("--- FAIL: TestBroken (0.00s)\n"), + Output: []byte("--- FAIL: TestBroken (0.00s)\nFAIL\n"), + Fingerprint: fp, + }, + { + // A crashed run: the panic aborted the binary before TestNever ran, + // so no FAIL trailer was printed and no pass may be credited to it. + ImportPath: "example.com/m/crashed", + Tests: []string{"TestPanics", "TestNever"}, + Passed: false, + Output: []byte("--- FAIL: TestPanics (0.00s)\npanic: boom\n\n" + + "goroutine 1 [running]:\nmain.main()\n"), Fingerprint: fp, }, { @@ -44,11 +54,14 @@ func TestCycleObservationsAttributeEveryKnownVerdict(t *testing.T) { assert.Equal(t, "watch", o.Source) } - require.Len(t, observations, 3) + require.Len(t, observations, 4) assert.Equal(t, flake.VerdictFail, byKey["example.com/m/red.TestBroken"]) assert.Equal(t, flake.VerdictPass, byKey["example.com/m/red.TestFine"], "a test that ran beside the failure and did not fail is a pass worth recording") assert.Equal(t, flake.VerdictPass, byKey["example.com/m/green.TestOK"]) + assert.Equal(t, flake.VerdictFail, byKey["example.com/m/crashed.TestPanics"]) + assert.NotContains(t, byKey, "example.com/m/crashed.TestNever", + "a test the crash prevented from running was not observed and must not be journalled") } // TestStickyRerunsProduceFlakeEvidence pins the payoff of recording watch @@ -60,7 +73,7 @@ func TestStickyRerunsProduceFlakeEvidence(t *testing.T) { ImportPath: "example.com/m/p", Tests: []string{"TestJumpy"}, Passed: false, - Output: []byte("--- FAIL: TestJumpy (0.00s)\n"), + Output: []byte("--- FAIL: TestJumpy (0.00s)\nFAIL\n"), Fingerprint: fp, }}) rerunCycle := cycleObservations([]RunResult{{ diff --git a/internal/watch/loop.go b/internal/watch/loop.go index c14bdc8..ac83a1c 100644 --- a/internal/watch/loop.go +++ b/internal/watch/loop.go @@ -229,6 +229,14 @@ func cycleObservations(results []RunResult) []flake.Observation { failedSet[name] = struct{}{} observe(name, flake.VerdictFail) } + // result.Tests is the requested list, not the executed one. A run that + // died mid-list — a panic, a -test.timeout dump, an os.Exit — never + // reached the remaining tests, and crediting them passes would + // fabricate the exact conflicting evidence the journal exists to + // detect. Only a run that printed its FAIL trailer ran the whole list. + if !printedFailTrailer(result.Output) { + continue + } for _, name := range result.Tests { if _, isFailed := failedSet[name]; !isFailed { observe(name, flake.VerdictPass) @@ -585,6 +593,24 @@ func printFailure(out io.Writer, output []byte) { } } +// printedFailTrailer reports whether a failing test binary ran to completion: +// testing.M prints a bare FAIL as its final line only after the whole test +// list executed, while a crashed run ends in a stack trace or whatever the +// dying process last wrote. +func printedFailTrailer(output []byte) bool { + lines := strings.Split(string(output), "\n") + for i := len(lines) - 1; i >= 0; i-- { + line := strings.TrimSpace(lines[i]) + if line == "" { + continue + } + + return line == "FAIL" + } + + return false +} + func failedTestNames(output []byte) []string { seen := make(map[string]struct{}) for line := range strings.SplitSeq(string(output), "\n") { diff --git a/internal/watch/watch_test.go b/internal/watch/watch_test.go index 875a988..f71c482 100644 --- a/internal/watch/watch_test.go +++ b/internal/watch/watch_test.go @@ -163,6 +163,29 @@ func TestEditInsideOneTestRunsOnlyThatTest(t *testing.T) { assert.NotEmpty(t, cycle.Runs[0].Pattern) } +// TestEditInsideTestMainRunsThePackageInFull pins the refinement against the +// one Test-prefixed function -test.run can never select: narrowing to +// ^(TestMain)$ runs zero tests and prints a green cycle for an edit to the +// code that sets up all of them. +func TestEditInsideTestMainRunsThePackageInFull(t *testing.T) { + r := newRepo(t) + withMain := "package alpha\n\nimport (\n\t\"os\"\n\t\"testing\"\n)\n\n" + + "func TestMain(m *testing.M) {\n\tcode := m.Run()\n\tos.Exit(code)\n}\n\n" + + "func TestDouble(t *testing.T) {\n\tif Double(2) != 4 {\n\t\tt.Fatal(\"bad\")\n\t}\n}\n" + r.write("alpha/alpha_test.go", withMain) + r.git("add", ".") + r.git("commit", "-qm", "alpha gains a TestMain") + + r.write("alpha/alpha_test.go", strings.Replace(withMain, "code := m.Run()", "code := m.Run() + 0", 1)) + + cycle, err := planner(r).Plan(t.Context(), []string{r.path("alpha/alpha_test.go")}) + require.NoError(t, err) + + require.Len(t, cycle.Runs, 1) + assert.Empty(t, cycle.Runs[0].Tests, "an edit to TestMain must run the whole package") + assert.Empty(t, cycle.Runs[0].Pattern) +} + // TestEditOutsideTestBodiesRunsThePackageInFull pins the refinement's guard: a // change to a helper or import can affect any test in the package, so the // refinement must decline rather than guess. @@ -235,6 +258,34 @@ func TestBinaryCacheReusesUntilTheClosureChanges(t *testing.T) { assert.False(t, reused, "a changed dependency closure must rebuild") } +// TestBinaryCacheIgnoresStaleStagingLeftovers pins the promotion invariant: a +// truncated .tmp from a killed build must never be renamed into place as if a +// later build produced it. The trap needs a package whose successful build +// emits no binary — one with no test files — so the only thing at the staging +// path is the leftover. +func TestBinaryCacheIgnoresStaleStagingLeftovers(t *testing.T) { + r := newRepo(t) + r.write("quiet/quiet.go", "package quiet\n\nfunc Noop() {}\n") + + c, err := NewBinaryCache() + require.NoError(t, err) + t.Cleanup(c.Close) + + stale := filepath.Join(c.dir, binaryName(watchModule+"/quiet")+".tmp") + require.NoError(t, os.WriteFile(stale, []byte("truncated garbage"), 0o755)) + + path, reused, err := c.Ensure(t.Context(), BuildRequest{ + Root: r.root, + Env: append(os.Environ(), "GOWORK=off"), + ImportPath: watchModule + "/quiet", + Fingerprint: cache.Fingerprint{1}, + }) + require.NoError(t, err) + assert.False(t, reused) + assert.Empty(t, path, "a package with no test files has no binary, stale leftovers included") + assert.NoFileExists(t, filepath.Join(c.dir, binaryName(watchModule+"/quiet"))) +} + func TestWatcherBatchesDebouncedSaves(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package a\n"), 0o644))