fix(internal/harness): size the report columns to the results - #358
Merged
Conversation
Report laid its table out with printf field widths chosen by eye -- %-40s for the spec and %-20s for the outcome. Nearly every path in testdata is longer than 40 characters (168 of 177 tracked paths), so the outcome was not a column at all: it landed wherever the path happened to end. The trailing %-20s also padded every line with spaces nothing followed. Both widths are now measured from the results being rendered, in runes, which is the unit fmt's %-*s pads in. A line whose Detail is empty stops at its outcome rather than padding out to a column with nothing to its right, which is also what sizes the outcome column: only rows carrying a Detail have a neighbour to line up against.
Two claims Report's own documentation makes that no test held it to, each confirmed by planting the defect and watching the new assertion redden. Sizing the outcome column from every result rather than only the ones carrying a Detail passed the whole suite. It is an observable difference — a line showing no detail is the longest outcome the harness has, and widening the column for it pads every detail out past an empty column, the padding this report exists to stop emitting. Report copies its input so the caller's slice order survives, and TestReport_IsStableAndSorted read only the rendered string, so sorting in place passed it. It now checks the caller's slice as well.
Two claims in Report's own documentation that the code did not keep. "A stable multi-line summary" was sorted with sort.Slice, which is not stable, so nothing ordered two results carrying one spec name and the same sweep could print its findings differently from one run to the next. It sorts stably now, for the reason irverify already does. The test that pins it scatters the duplicates through enough distinct keys to make the sort partition around them: an unstable sort leaves a short slice to an insertion pass and short-circuits one whose keys are all equal, so a two-result case would have passed either way. "One aligned line per spec" is not what a Detail carrying newlines renders, and the round-trip oracle's carries several — it prints both encodings. The comment now says one line per spec plus one for each newline a Detail holds, and that the columns line up on the line each result begins.
The comment justifying the stable sort claimed an unstable one would render a sweep differently from one run to the next. It would not: sort.Slice is deterministic for a given input, identical across repeated sorts and across processes. What it gives is an order the API does not specify and the caller did not choose — no less a reason for a report that promises a stable summary, but not the reason that was written down.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
harness.Reportlaid its table out with printf field widths chosen by eye:r.Specis a spec path, and nearly every spec the sweep renders is longer than40 characters, so the outcome was not a column at all — it landed wherever the
path happened to end:
The trailing
%-20salso padded every line with spaces the report had no usefor, since most lines have no
Detailto follow them.Both widths are now measured from the results being rendered, the way the CLI's
command column is derived rather than declared. Counting is in runes, which is
the unit
fmt's%-*spads in — a byte count would over-pad a non-ASCII path.A line whose
Detailis empty stops at its outcome instead of padding out to acolumn with nothing to its right; that is also what sizes the outcome column,
since only rows carrying a
Detailhave a neighbour to line up against.After:
Test plan
Three new tests in
internal/harness, each confirmed to redden against thedefect it names rather than accepted because it went green:
TestReport_ColumnsAreSizedToTheResults— the outcome and detail columnsstart at the same offset on a long-path line and a short-path line, and the
spec column is exactly the longest spec wide. Restoring
%-40s %-20s %sfails it on both offsets.
TestReport_LinesAreNotPaddedPastTheirLastColumn— no line carries paddingafter its last column. Restoring the old format fails it.
TestReport_WidthsAreCountedInRunes— an eight-rune, eleven-byte path ispadded to eight. Swapping
utf8.RuneCountInStringforlenfails it withexpected: 9, actual: 12.TestReport_OutcomeColumnIsSizedOnlyByLinesThatUseIt— the outcome column ismeasured against the results that carry a
Detailand no others. Sizing itfrom every result passed the rest of the suite while padding each detail out
past a column standing empty on the line that set its width.
TestReport_IsStableAndSortednow also checks the caller's slice, which thecopy exists to protect; reading only the rendered string let an in-place sort
pass.
TestReport_ResultsNamedAlikeKeepTheirGivenOrder— two results carrying onespec name render in the order they were given.
Reportdocumented a "stable"summary and sorted with
sort.Slice, which is not; it usessort.SliceStablenow, for the reason
irverifyalready does. The case scatters its duplicatesthrough enough distinct keys that the sort must partition around them, since an
unstable sort leaves a short slice to an insertion pass and short-circuits one
whose keys are all equal — a two-result case would pass either way.
Report's doc comment also claimed "one aligned line per spec", which is notwhat a
Detailholding newlines renders, and the round-trip oracle's holdsseveral: it prints both encodings. It now says one line per spec plus one for
each newline a
Detailcarries, and that the columns line up on the line eachresult begins.
columnStartfails when a column is absent, so two missing columns cannotcompare equal and assert nothing.
mainis merged in, and the full gate is green on the merged branch:gofmt,go vet ./...,golangci-lint run(0 issues),go build ./...,./scripts/check-coverage.sh.Closes #309