Skip to content

test(cmd/morphic): pin the rendered help text with goldens - #207

Open
fuad-daoud wants to merge 1 commit into
fix/cli-help-exit-codefrom
test/cli-help-goldens
Open

test(cmd/morphic): pin the rendered help text with goldens#207
fuad-daoud wants to merge 1 commit into
fix/cli-help-exit-codefrom
test/cli-help-goldens

Conversation

@fuad-daoud

@fuad-daoud fuad-daoud commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds golden files for the rendered root help and compile help, so wording changes show up as a reviewable diff rather than slipping by unnoticed. Regenerate with:

go test ./cmd/morphic -run TestHelp_MatchesGolden -update

That -update is ir/irtest's, not a second declaration in this package. Two flag.Bool("update", ...) calls do not shadow each other — both register on the same command-line FlagSet, so the test binary panics at init (flag redefined: update) the moment anything pulls irtest into these tests. Reusing the existing flag makes the collision unrepresentable.

The README gains the help forms and states that help exits 0. It also gets compile's real synopsis back alongside the root one: with only morphic <command> [flags] above it, the flag table read as though -o and --fail-on were root flags.

Test plan

  • TestHelp_MatchesGolden compares both help texts against the committed goldens; deliberately not parallel because -update writes files.
  • Checked the three things that usually go wrong with goldens, by executing rather than reading:
    • golden deleted → red, with read golden testdata/root-help.txt (run with -update to create)
    • -update → recreates both files, md5sum -c clean against the committed bytes
    • wording changed in writeRootHelp → red, golden mismatch for testdata/root-help.txt
  • Confirmed the -update collision is gone: a test file importing irtest alongside these now runs, where before it panicked at init.
  • gofmt -l, go vet ./..., golangci-lint run (0 issues), go build ./..., ./scripts/check-coverage.sh — 4232/4232 statements.

@fuad-daoud
fuad-daoud force-pushed the test/cli-help-goldens branch from 680137c to b99e2be Compare July 31, 2026 08:15
@fuad-daoud
fuad-daoud changed the base branch from test/cli-usage-error-table to fix/cli-help-exit-code July 31, 2026 08:15
@fuad-daoud
fuad-daoud requested a review from OmarAlJarrah August 1, 2026 18:47
@OmarAlJarrah
OmarAlJarrah force-pushed the test/cli-help-goldens branch from b99e2be to 1ebb5a3 Compare August 2, 2026 01:23

@OmarAlJarrah OmarAlJarrah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The golden mechanism holds up. I checked the three things that usually go wrong with goldens rather than reading it:

  • golden removed: red, with the message that tells you to run -update
  • -update: recreates both files byte for byte
  • wording changed in writeRootHelp: red

Gate clean on the merged state: golangci-lint 0 issues, coverage 4234/4234.

Also worth noting this lands the README update that #205's Breaking section already claims. That claim is still wrong on #205 read on its own, since the file isn't in that PR.

Last thing: the five follow-ups in the description are good catches, but a squashed PR body isn't a tracker. None of them are open issues right now, so they'll be gone once this merges. Worth filing the ones you mean to keep, particularly the flag.ErrHelp handling being compile-specific, since that one gets more expensive with every subcommand added.

Comment thread cmd/morphic/help_test.go Outdated
}
}

var updateHelp = flag.Bool("update", false, "rewrite help golden files instead of comparing")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't define a second -update. ir/irtest already owns one, and this isn't a latent hazard, it's a live landmine: import ir/irtest anywhere in this package's tests and the binary panics at init before a single test runs.

morphic.test flag redefined: update
panic: ... flag redefined: update

Reuse the existing one and the collision can't happen:

Suggested change
var updateHelp = flag.Bool("update", false, "rewrite help golden files instead of comparing")

Then if *updateHelp { becomes if irtest.Update() {, with "github.com/dexpace/morphic/ir/irtest" in the imports. I ran it: -update still rewrites both goldens byte for byte, the package tests pass, and archtest is happy with the import.

That also clears one of the five follow-ups in the description.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — it uses irtest.Update() now.

Reproduced the panic first (a probe test importing irtestflag redefined: update before any test ran), then confirmed it's gone: the same probe now runs clean. -update still rewrites both goldens byte for byte, checked with md5sum -c against the committed files, and archtest is green (it only audits non-test files, so the import is outside its allowlist by design).

Comment thread cmd/morphic/help_test.go
Comment on lines +156 to +157
{"root", nil, "root-help.txt"},
{"compile", []string{"help", "compile"}, "compile-help.txt"},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The misuse text is the third thing that renders and the only one not pinned here. writeCommandUsage and the root help that misuse prints to stderr can both drift silently. Worth a third row, capturing stderr:

{"compile misuse", []string{"compile"}, "compile-usage.txt"},

Needs the helper to take stderr for that case, so only do it if you think the text is worth pinning. The two you have are the ones that matter most.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this one until the rest of the stack settles — it needs the helper to take stderr, and I want to decide it alongside the flagSetprintFlags question on #204, which touches the same rendering. Leaning yes: the misuse text is the only one of the three renders that nothing pins.

Comment thread README.md
```
usage:
morphic compile <spec-file> [-o out.json] [--fail-on error|warning] [--skip-validate]
morphic <command> [flags]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the compile synopsis leaves the flag table sitting under morphic <command> [flags], so -o and --fail-on now read as root flags. They aren't. The old line was wrong in form but it did tell you whose flags these were.

Add compile's real synopsis back, in the form the CLI actually prints, so this can't drift again:

Suggested change
morphic <command> [flags]
morphic <command> [flags]
morphic compile <spec-file> [flags]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added compile's synopsis back in the form the CLI prints, alongside the root one. Also put a line above the table saying the flags are compile's, since the table itself gives no clue.

Comment thread README.md Outdated
Comment on lines +108 to +109
`morphic`, `morphic help`, and `morphic -h`/`--help` print the command list. `morphic help compile`
and `morphic compile --help` print a command's flags. Help always prints to stdout and exits `0`.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-help works too, same as -h and --help. Either list it or say "a help flag" rather than naming two of the three.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — it now says a help flag and lists all three.

Golden files make the rendered help reviewable as a diff, so wording
changes are deliberate rather than incidental. The -update flag comes
from ir/irtest rather than a second declaration here, which would panic
the test binary at init the moment anything else imported irtest.

Document the help forms and their exit code in the README, and replace
the compile synopsis the CLI never prints with the one it does — the
flag table below it is compile's, not the root command's.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fuad-daoud
fuad-daoud force-pushed the test/cli-help-goldens branch from 1ebb5a3 to cdcc016 Compare August 2, 2026 05:53
@fuad-daoud fuad-daoud changed the title test(cmd/morphic): pin help text with goldens and document the forms test(cmd/morphic): pin the rendered help text with goldens Aug 2, 2026
@fuad-daoud

Copy link
Copy Markdown
Collaborator Author

Filed the surviving follow-ups so they outlive the squash — you were right that a PR body isn't a tracker:

The other two are closed by the stack rather than filed: the -update collision is fixed here, and TestCommands_TableIsWellFormed on #204 covers the malformed-entry case (blank name, duplicate name — both verified by planting the defect).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants