test(help): make help tests resilient to new builtins#226
Conversation
The four help scenarios under tests/scenarios/cmd/help/ pinned the full command list and total command count in their expected stdout. Every PR that added a builtin had to update all four files — same line, same alphabetical ordering — making merge conflicts the default outcome between any two builtin-adding PRs (e.g. #224). Switch the four scenarios to stdout_contains assertions covering the stable structural elements (header, sections, footer, a couple of known-stable commands). Move the "every registered builtin appears in help" inventory check into builtins/tests/help/help_test.go and drive it off builtins.Names() / Meta() so the registry itself is the source of truth — registering a builtin makes it expected, with no test edits.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a5973835
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Nice, good change!! Waiting for the Claude vs Codex iterations before approving |
|
@codex review this PR Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48a5973835
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — PR #226 (self-review)
This is a test-only refactor: no production code changes. The four tests/scenarios/cmd/help/*.yaml scenarios that previously hardcoded the full registry (causing systematic merge conflicts on every builtin addition, e.g. PR #224) move to stdout_contains substring checks for stable structural elements. The inventory check ("every registered builtin appears in help") moves into builtins/tests/help/help_test.go:TestHelpListsAllCommands, driven off builtins.Names() / builtins.Meta() so registering a builtin is what makes it expected.
Overall assessment: safe to merge. No P0/P1/P2 findings. Two P3 nits below — both about strengthening the inventory check so duplicate descriptions or a regression in the --all disabled-table body would not be silently masked.
Findings summary
| # | Priority | File | Finding |
|---|---|---|---|
| 1 |  |
builtins/tests/help/help_test.go:144 |
Inventory match on description-only is weakened by duplicate descriptions ([ and test share "evaluate conditional expression") |
| 2 |  |
tests/scenarios/cmd/help/restricted_all_flag.yaml:17 |
--all scenario asserts the Disabled commands: label is in table form but never asserts a description from the disabled table body actually rendered |
Positive observations
- The refactor solves the documented problem: a future builtin-adding PR no longer needs to touch any help test, eliminating the conflict pattern surfaced by #224.
- The
"Disabled commands: "(trailing space, compact form) vs"Disabled commands:\n"(newline, table form) split is a concise way to keep the meaningful semantic distinction betweenrestricted.yamlandrestricted_all_flag.yamleven after dropping the full-stdout match. - Existing rendering invariants (
TestHelpColumnsAligned,TestHelpListsSorted,TestHelpFieldOnlyOnNoFlagsCommands) keep covering format quality, so the YAML loosening does not lose coverage. skip_assert_against_bash: trueis preserved on all four scenarios — no new bash-comparison surface introduced.
|
Iteration 1 self-review result: 0 P0 / 0 P1 / 0 P2 / 2 P3. P0_P1_P2_COUNT = 0. Review: #226 (review) Both findings are nice-to-have hardening of the inventory check (description-only match weakened by |
Address review feedback (codex P2 + self-review P3) from the previous iteration: - TestHelpListsAllCommands: match the full rendered table row (`^name\s+description$`) instead of just the description text. Two builtins share the description "evaluate conditional expression" (`[` and `test`), so the previous description-only check would silently pass even if one row was dropped from the help output. - restricted_all_flag.yaml: add `"concatenate and print files"` to the contains-list. The label-only assertion (`"Disabled commands:\n"`) proved the table form was selected over the compact form, but did not prove the table body itself rendered. `cat` is always disabled under this scenario's allowed set, and its description does not appear elsewhere in the output, so the substring is stable.
|
@codex review this PR (iteration 2 — re-review after fixes) Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — PR #226 iteration 2 (self-review)
Re-reviewed after the iteration-1 commit 67c64925 which addressed all four threads from iteration 1 (codex P2 ×2 + self-review P3 ×2).
No new findings. The fixes are correct and minimal:
builtins/tests/help/help_test.go:148— the regex(?m)^name\s+description$is correctly multiline-anchored, usesregexp.QuoteMetaon bothname(important for[) and the description (defensive against future descriptions with metacharacters), and matches the actual rendered table-row format (name, ≥2 spaces of column padding, description). Catches the[/testshared-description hazard that codex called out.tests/scenarios/cmd/help/restricted_all_flag.yaml:18—"concatenate and print files"iscat's description; underallowed_commands: ["rshell:echo", "rshell:help"]catis always disabled, and the description does not appear elsewhere in the output (not in the allowed table, not in any feature blurb), so the substring genuinely proves the disabled-table body rendered without coupling the test to column-width math.
Overall assessment: safe to merge. 0 P0 / 0 P1 / 0 P2 / 0 P3.
Findings summary
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings. |
|
Iteration 2 self-review result: 0 P0 / 0 P1 / 0 P2 / 0 P3 — no new findings. P0_P1_P2_COUNT = 0. Iteration 1's regex-match and disabled-table-body fixes verified correct. #226 (review) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67c64925fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review this PR (iteration 3 — re-review after iteration 2 confirmed clean) Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — PR #226 iteration 3 (self-review)
HEAD unchanged since iteration 2 (67c64925). Re-reviewed the same diff: no new findings. The iter-1 fixes (regex inventory match in help_test.go, disabled-table-body assertion in restricted_all_flag.yaml) remain correct.
Overall assessment: safe to merge. 0 P0 / 0 P1 / 0 P2 / 0 P3.
|
Iteration 3 self-review result: 0 P0 / 0 P1 / 0 P2 / 0 P3 (HEAD unchanged since iter 2). P0_P1_P2_COUNT = 0. |
|
@codex review this PR (iteration 4) Severity format: P0 (red), P1 (orange), P2 (yellow), P3 (blue). Prefix each finding with its priority label and include a summary table. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — PR #226 iteration 4 (self-review)
HEAD unchanged since iteration 1's commit 67c64925. Re-reviewed: no new findings. 0 P0 / 0 P1 / 0 P2 / 0 P3.
All prior threads remain resolved; the iter-1 fixes (regex inventory match + disabled-table-body assertion) are correct.
|
Iteration 4 self-review result: 0 P0 / 0 P1 / 0 P2 / 0 P3 (HEAD unchanged). P0_P1_P2_COUNT = 0. |
|
@codex review this PR (iteration 5 — final round before clean-state confirmation) Severity format: P0/P1/P2/P3 with summary table. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Code Review — PR #226 iteration 5 (self-review)
HEAD unchanged since iteration 1's commit 67c64925. Final iteration of the review-fix loop. No new findings. 0 P0 / 0 P1 / 0 P2 / 0 P3.
All prior threads remain resolved; the iter-1 fixes (regex inventory match in builtins/tests/help/help_test.go + disabled-table-body assertion in tests/scenarios/cmd/help/restricted_all_flag.yaml) are correct and cover the issues codex raised at P2.
Overall assessment: safe to merge.
|
Iteration 5 self-review result: 0 P0 / 0 P1 / 0 P2 / 0 P3 (HEAD unchanged). P0_P1_P2_COUNT = 0. |
Review-Fix Loop Summary
Iteration log
Final state
Remaining issuesNone. Notes
|
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Address codex P2 feedback (#226 (comment)): the row regex `^name\s+description$` collapses to `^name\s+$` when a builtin's Description is empty, which silently matches the blank padded row that `printCommandTable` emits in that case. A builtin that forgets to set Description would still pass the inventory test. Add an explicit `require.NotEmpty(meta.Description)` before the regex so the repo rule "every builtin has a non-empty Description visible in help" is enforced loudly at test time.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb8ef8e897
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Summary
The four help scenarios under
tests/scenarios/cmd/help/(restricted.yaml,restricted_all_flag.yaml,unrestricted.yaml,unrestricted_all_flag.yaml) pinned the full command list and total command count in their expected stdout. Every PR that adds a builtin had to update all four files — same line, same alphabetical ordering — making merge conflicts the default outcome between any two builtin-adding PRs (e.g. thexargsPR #224 conflicting withdu#207).This PR makes the test setup resilient:
stdout_containsand only assert the stable structural elements: the header, theFeatures:/Not supported:/Commands:sections, a couple of known-stable command rows (echo,help), the disabled-commands label form (compact vs. table), and the footer hint.builtins/tests/help/help_test.go(TestHelpListsAllCommands), which now drives offbuiltins.Names()/builtins.Meta()— registering a builtin is what makes it expected, no test edits required.TestHelpColumnsAligned,TestHelpListsSorted,TestHelpFieldOnlyOnNoFlagsCommands) keep covering rendering quality.Net effect: a future PR adding a builtin no longer needs to touch any help test — just register the command in the registry.
Test plan
make fmtgo test ./builtins/tests/help/...go test ./tests/ -run TestShellScenarios -timeout 120sRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s(all four files setskip_assert_against_bash: true, unchanged)