From 0759c0a646f576951e61ecf5dbe7517811d692d6 Mon Sep 17 00:00:00 2001 From: Mark Ayers Date: Wed, 5 Aug 2026 15:00:08 -0700 Subject: [PATCH] Simplify pass + code-review fixes on today's changes Removed dice.Fixed (unused speculative scaffolding), collapsed shipgen's structureData aliases []string to a scalar, and merged golden/README-freshness test pairs to share one subprocess run each. Also fixed two worldgen -format json bugs found by review: nil TradeCodes rendering as null instead of [], and a mid-loop Fatalf silently dropping already-buffered stdout records. Made shipgen's core-cost test assertion non-tautological by pinning Fuel.Cost independently instead of reading it back off the struct under test. Co-Authored-By: Claude --- cmd/chargen/golden_test.go | 14 ++++++------ cmd/sectorgen/golden_test.go | 40 ++++++++++++++++++--------------- cmd/shipgen/golden_test.go | 30 ++++++++++++------------- cmd/worldgen/golden_test.go | 19 ++++++++-------- cmd/worldgen/main.go | 18 ++++++++++++++- internal/dice/dice.go | 14 ------------ internal/dice/dice_test.go | 28 ----------------------- internal/shipgen/design_test.go | 14 +++++++----- internal/shipgen/generate.go | 13 ++++------- internal/shipgen/shipgen.go | 18 +++++++-------- 10 files changed, 92 insertions(+), 116 deletions(-) diff --git a/cmd/chargen/golden_test.go b/cmd/chargen/golden_test.go index 3df8343..1054084 100644 --- a/cmd/chargen/golden_test.go +++ b/cmd/chargen/golden_test.go @@ -4,15 +4,15 @@ import "testing" // TestGoldenCareerScoutSeed7 pins chargen's rendered character sheet for a // seeded career run, byte for byte — see cmd/worldgen/golden_test.go for why. +// It also checks the same run against the README's sample block, sharing the +// one subprocess invocation rather than re-running the command a second time. // Run `go test ./cmd/chargen/... -update` to regenerate after a deliberate // rules change. func TestGoldenCareerScoutSeed7(t *testing.T) { - command.Run(t, "-career", "scout", "-seed", "7").AssertGolden(t, "careerScoutSeed7") -} - -// TestReadmeSampleUpToDate fails if the README's chargen sample ever diverges -// from a fresh run of the exact invocation it shows. -func TestReadmeSampleUpToDate(t *testing.T) { got := command.Run(t, "-career", "scout", "-seed", "7") - got.AssertReadmeUpToDate(t, "../../README.md", "$ go run ./cmd/chargen -career scout -seed 7") + + t.Run("golden", func(t *testing.T) { got.AssertGolden(t, "careerScoutSeed7") }) + t.Run("readme", func(t *testing.T) { + got.AssertReadmeUpToDate(t, "../../README.md", "$ go run ./cmd/chargen -career scout -seed 7") + }) } diff --git a/cmd/sectorgen/golden_test.go b/cmd/sectorgen/golden_test.go index 9785e20..2939f7c 100644 --- a/cmd/sectorgen/golden_test.go +++ b/cmd/sectorgen/golden_test.go @@ -8,25 +8,29 @@ import ( ) // TestGoldenSeed42 pins sectorgen's default subsector listing for a seeded -// run, byte for byte — see cmd/worldgen/golden_test.go for why. Run -// `go test ./cmd/sectorgen/... -update` to regenerate after a deliberate -// rules change. +// run, byte for byte — see cmd/worldgen/golden_test.go for why. It also +// checks the same run against the README's sample block, sharing the one +// subprocess invocation rather than re-running the command a second time. The +// README pipes the command through `head -3`, so only the first three lines +// are shown — comparing the whole stdout against that block would fail on +// line count alone, not content, so the readme check is a prefix comparison +// instead of full equality (the one command among the four goldens where +// that distinction matters). Run `go test ./cmd/sectorgen/... -update` to +// regenerate after a deliberate rules change. func TestGoldenSeed42(t *testing.T) { - command.Run(t, "-seed", "42").AssertGolden(t, "seed42") -} - -// TestReadmeSampleUpToDate fails if the README's sectorgen sample ever -// diverges from a fresh run of the exact invocation it shows. The README -// pipes the command through `head -3`, so only the first three lines are -// shown — comparing the whole stdout against that block would fail on line -// count alone, not content, so this checks a prefix instead of full equality -// (the one command among the four goldens where that distinction matters). -func TestReadmeSampleUpToDate(t *testing.T) { got := command.Run(t, "-seed", "42") - want := clitest.ReadmeBlock(t, "../../README.md", "$ go run ./cmd/sectorgen -seed 42 | head -3") - if !strings.HasPrefix(got.Stdout, want) { - t.Errorf("README sample is stale.\n--- README shows (head -3) ---\n%s--- actual output's first lines ---\n%.*s", - want, len(want), got.Stdout) - } + t.Run("golden", func(t *testing.T) { got.AssertGolden(t, "seed42") }) + t.Run("readme", func(t *testing.T) { + want := clitest.ReadmeBlock(t, "../../README.md", "$ go run ./cmd/sectorgen -seed 42 | head -3") + + if !strings.HasPrefix(got.Stdout, want) { + t.Errorf( + "README sample is stale.\n--- README shows (head -3) ---\n%s--- actual output's first lines ---\n%.*s", + want, + len(want), + got.Stdout, + ) + } + }) } diff --git a/cmd/shipgen/golden_test.go b/cmd/shipgen/golden_test.go index 2c75078..d034f31 100644 --- a/cmd/shipgen/golden_test.go +++ b/cmd/shipgen/golden_test.go @@ -15,21 +15,21 @@ func murphyReadmeArgs() []string { // TestGoldenReadmeDesign pins shipgen's rendered ship card for the README's // own design flags, byte for byte — see cmd/worldgen/golden_test.go for why. -// Run `go test ./cmd/shipgen/... -update` to regenerate after a deliberate -// rules change. +// It also checks the same run against the README's sample block, sharing the +// one subprocess invocation rather than re-running the command a second +// time; that readme check caught a real instance of drift on first write — +// the README was missing the Defenses/Fuel/problem lines the command +// actually prints. Run `go test ./cmd/shipgen/... -update` to regenerate +// after a deliberate rules change. func TestGoldenReadmeDesign(t *testing.T) { - command.Run(t, murphyReadmeArgs()...).AssertGolden(t, "readmeDesign") -} - -// TestReadmeSampleUpToDate fails if the README's shipgen sample ever -// diverges from a fresh run of the exact invocation it shows. It caught a -// real instance of this on first write: the README was missing the -// Defenses/Fuel/problem lines the command actually prints. -func TestReadmeSampleUpToDate(t *testing.T) { got := command.Run(t, murphyReadmeArgs()...) - got.AssertReadmeUpToDate( - t, - "../../README.md", - `$ go run ./cmd/shipgen -hull A -tl 12 -config S -maneuver A -jump A -weapon "beamlaser:T1:orbit" -defense blackglobe`, - ) + + t.Run("golden", func(t *testing.T) { got.AssertGolden(t, "readmeDesign") }) + t.Run("readme", func(t *testing.T) { + got.AssertReadmeUpToDate( + t, + "../../README.md", + `$ go run ./cmd/shipgen -hull A -tl 12 -config S -maneuver A -jump A -weapon "beamlaser:T1:orbit" -defense blackglobe`, + ) + }) } diff --git a/cmd/worldgen/golden_test.go b/cmd/worldgen/golden_test.go index 8f601a6..050adcc 100644 --- a/cmd/worldgen/golden_test.go +++ b/cmd/worldgen/golden_test.go @@ -10,19 +10,18 @@ import ( // TestGoldenN3Seed42 pins worldgen's rendered stdout for a seeded run, // byte for byte, closing the #321 hazard: nothing else in this package would // notice a reordered field or a shifted dice stream that changed every -// generated UWP without changing the exit code or the seed report. Run -// `go test ./cmd/worldgen/... -update` to regenerate after a deliberate +// generated UWP without changing the exit code or the seed report. It also +// checks the same run against the README's sample block, sharing the one +// subprocess invocation rather than re-running the command a second time. +// Run `go test ./cmd/worldgen/... -update` to regenerate after a deliberate // rules change — eyeball the diff before committing it. func TestGoldenN3Seed42(t *testing.T) { - command.Run(t, "-n", "3", "-seed", "42").AssertGolden(t, "n3seed42") -} - -// TestReadmeSampleUpToDate fails if the README's "Sample output" block for -// this command ever diverges from a fresh run of the exact invocation it -// shows. -func TestReadmeSampleUpToDate(t *testing.T) { got := command.Run(t, "-n", "3", "-seed", "42") - got.AssertReadmeUpToDate(t, "../../README.md", "$ go run ./cmd/worldgen -n 3 -seed 42") + + t.Run("golden", func(t *testing.T) { got.AssertGolden(t, "n3seed42") }) + t.Run("readme", func(t *testing.T) { + got.AssertReadmeUpToDate(t, "../../README.md", "$ go run ./cmd/worldgen -n 3 -seed 42") + }) } // TestGoldenN3Seed42JSON pins -format json's rendered output, byte for byte — diff --git a/cmd/worldgen/main.go b/cmd/worldgen/main.go index 17af39a..a851e31 100644 --- a/cmd/worldgen/main.go +++ b/cmd/worldgen/main.go @@ -74,6 +74,16 @@ func main() { // guard rather than bypassing it via string(p.Starport) directly. uwpStr := p.String() + // TradeClassifications returns a nil slice when no code matches — + // a real, valid outcome, not an edge case to special-case away — + // so it is coerced to an empty slice here to render "[]" rather + // than "null"; a code-less world otherwise breaks any JSON + // consumer that iterates tradeCodes without a null check. + jsonTC := tc + if jsonTC == nil { + jsonTC = []tradecode.Code{} + } + err := enc.Encode(jsonWorld{ UWP: uwpStr, Starport: uwpStr[:1], @@ -84,9 +94,15 @@ func main() { Government: p.Government, Law: p.Law, TechLevel: p.TechLevel, - TradeCodes: tc, + TradeCodes: jsonTC, }) if err != nil { + // Flush what already rendered before exiting: out is only + // otherwise flushed once, after the loop, so an unflushed + // buffer here would silently discard every earlier record + // this run already generated. + _ = out.Flush() + cli.Fatalf("writing world %d: %v", i, err) } diff --git a/internal/dice/dice.go b/internal/dice/dice.go index 9411128..7ad3eba 100644 --- a/internal/dice/dice.go +++ b/internal/dice/dice.go @@ -96,20 +96,6 @@ func NewScripted(faces ...int) *Roller { }) } -// Fixed returns a Roller where every die shows face, forever. It pins one -// variable in a property sweep — e.g. holding a die at 6 while another -// Roller varies the rest — where NewScripted's exact, finite script would be -// the wrong shape. -// -// It panics if face is not a real die face in 1..6. -func Fixed(face int) *Roller { - if face < 1 || face > 6 { - panic(fmt.Sprintf("dice: Fixed face %d is not a die face (want 1..6)", face)) - } - - return NewSource(func() int { return face }) -} - // Seed reports the seed the Roller was built from and whether it has one. A // Roller from NewSource or NewScripted draws from a supplied sequence rather // than a seeded generator, so it reports ok false. diff --git a/internal/dice/dice_test.go b/internal/dice/dice_test.go index 23cf351..1c5bb84 100644 --- a/internal/dice/dice_test.go +++ b/internal/dice/dice_test.go @@ -57,34 +57,6 @@ func TestScriptedPanicsWhenExhausted(t *testing.T) { r.Die() } -// TestFixedRejectsNonFaces mirrors TestScriptedRejectsNonFaces: Fixed -// validates its face eagerly, at construction, not at first roll. -func TestFixedRejectsNonFaces(t *testing.T) { - for _, bad := range []int{0, 7, -1} { - func() { - defer func() { - if recover() == nil { - t.Errorf("Fixed(%d) accepted a non-die face", bad) - } - }() - - Fixed(bad) - }() - } -} - -// TestFixedRepeatsForever distinguishes Fixed from NewScripted: unlike a -// script, it never exhausts. -func TestFixedRepeatsForever(t *testing.T) { - r := Fixed(6) - - for range 100 { - if got := r.Die(); got != 6 { - t.Fatalf("Die() = %d, want 6", got) - } - } -} - func TestDie(t *testing.T) { r := scripted(4) if got := r.Die(); got != 4 { diff --git a/internal/shipgen/design_test.go b/internal/shipgen/design_test.go index da4bc7a..7ddb94a 100644 --- a/internal/shipgen/design_test.go +++ b/internal/shipgen/design_test.go @@ -50,11 +50,15 @@ func TestDesignMurphy(t *testing.T) { t.Errorf("drive costs = %d/%d/%d, want 4,000,000/10,000,000/4,000,000", s.Maneuver.Cost, s.Jump.Cost, s.Power.Cost) } - // Core cost is the sum of the components just checked above, not a - // separately hand-added total — so it can't drift from them while still - // passing. (The book's MCr 70.3 full-ship figure additionally includes the - // deferred staterooms, turret, and sensors.) - if want := s.Hull.Cost + s.Maneuver.Cost + s.Jump.Cost + s.Power.Cost + s.Fuel.Cost; s.Cost != want { + // Core cost is the sum of the components just checked above. Fuel is + // pinned by its own formula (TestFuelMurphy: 22t x Cr500 + KCr100 scoop + + // MCr1 purifier), not read back off s.Fuel.Cost — reading it back would + // make this assertion tautological with respect to design.go's own + // ship.Cost formula and unable to catch a wrong fuel price. (The book's + // MCr 70.3 full-ship figure additionally includes the deferred + // staterooms, turret, and sensors.) + const fuelCost = 22*500 + 100_000 + 1_000_000 + if want := s.Hull.Cost + s.Maneuver.Cost + s.Jump.Cost + s.Power.Cost + fuelCost; s.Cost != want { t.Errorf("core cost = %d, want %d (Hull+Maneuver+Jump+Power+Fuel)", s.Cost, want) } } diff --git a/internal/shipgen/generate.go b/internal/shipgen/generate.go index a2435a4..3231097 100644 --- a/internal/shipgen/generate.go +++ b/internal/shipgen/generate.go @@ -27,21 +27,16 @@ func ConfigByLetter(letter string) (Config, bool) { // StructureByName returns the Structure for a name and whether it was found, // matching case-, space-, and hyphen-insensitively against the structure's CLI -// short name (see StructureNames), its display name, or any of its aliases — -// so "plate", "Frame-and-Plate", and "frame-plate" all resolve to FramePlate. +// short name (see StructureNames), its display name, or its alias — so +// "plate", "Frame-and-Plate", and "frame-plate" all resolve to FramePlate. func StructureByName(name string) (Structure, bool) { key := squash(name) for i, d := range structureData { - if squash(d.cliName) == key || squash(d.display) == key { + if squash(d.cliName) == key || squash(d.display) == key || + (d.alias != "" && squash(d.alias) == key) { return Structure(i), true } - - for _, alias := range d.aliases { - if squash(alias) == key { - return Structure(i), true - } - } } return 0, false diff --git a/internal/shipgen/shipgen.go b/internal/shipgen/shipgen.go index 439e141..b372f70 100644 --- a/internal/shipgen/shipgen.go +++ b/internal/shipgen/shipgen.go @@ -83,16 +83,16 @@ const ( // never be reached, since StructureByName squashes hyphens out of its input // before the lookup. var structureData = [...]struct { - display string // rendered form, e.g. in a ship card - cliName string // -structure flag's short form, e.g. "plate" - aliases []string // other accepted -structure spellings, squashed at lookup + display string // rendered form, e.g. in a ship card + cliName string // -structure flag's short form, e.g. "plate" + alias string // another accepted -structure spelling, squashed at lookup ("" if none) }{ - FramePlate: {"Frame-and-Plate", "plate", []string{"frameplate"}}, - Shell: {"Shell", "shell", nil}, - Polymer: {"Polymer", "polymer", nil}, - FeNi: {"FeNi", "feni", nil}, - Organic: {"Organic", "organic", nil}, - Charged: {"Charged", "charged", nil}, + FramePlate: {"Frame-and-Plate", "plate", "frameplate"}, + Shell: {"Shell", "shell", ""}, + Polymer: {"Polymer", "polymer", ""}, + FeNi: {"FeNi", "feni", ""}, + Organic: {"Organic", "organic", ""}, + Charged: {"Charged", "charged", ""}, } func (s Structure) String() string {