Skip to content

Test: pin the usage-error contract for bare intermediate command groups - #705

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-group-bare-invocation-contract
Open

Test: pin the usage-error contract for bare intermediate command groups#705
AmaadMartin wants to merge 2 commits into
mainfrom
fix/cli-group-bare-invocation-contract

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

  2. Or, if no issue exists, describe the change:
    Problem: deploy and integration are intermediate groups that only namespace their children. Neither has an action handler, so commander prints the group help to stderr and exits 1. That answer is correct, but no test pins it and no comment records the decision, so a later refactor can attach a handler and flip it silently.

Solution: This change adds a comment and tests. No runtime behaviour changes. The adopted contract: a group named without a subcommand is a usage error (help to stderr, exit 1), and an explicit --help is a success (help to stdout, exit 0). Five new cases pin the stream, the CommanderError code and the exit code for deploy, integration, both unknown-subcommand forms, and deploy --help.

Why not print help to stdout and exit 0. An action handler on deploy disables commander's unknown-subcommand check, and DEPLOY_COMMAND already sets allowExcessArguments() for the gcloud pass-through. adk deploy cloud-run --project=x — a hyphen typo — would then print help and exit 0, reporting success for a deploy that never ran. I measured this, see mutation M1 below. The implicit help [command] subcommand also disappears, because _getHelpCommand() requires no action handler.

Collision check. I listed all open pull requests on this fork and diffed every candidate that touches dev/src/cli/cli.ts or dev/test/cli/cli_test.ts. No open pull request asserts anything about a bare group. Two overlap without colliding: #597 changes the root command (this change asserts nothing about bare adk, and its assertions avoid the program name, so it survives the ADK CLI -> adk rename), and #587 adds help-text assertions for deploy cloud_run. I did not stack, because stacking on one of two overlapping branches is arbitrary, and it would couple a test-only change to an unmerged runtime change.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

npx vitest run --project unit:dev dev/test/cli/cli_test.ts — 31 passed (26 existing, 5 new). No existing test was edited; the new describe block is appended at the end of the file.

Mutation proof. Coverage is not proof, so I ran each new test against mutated source and recorded the failure.

mutation in dev/src/cli/cli.ts cases that failed failure message
M1: .action(() => DEPLOY_COMMAND.outputHelp()) on deploy 1, 3 expected 'adk deploy' to throw; expected 'adk deploy bogus' to throw
M2: .action(() => CONFORMANCE_COMMAND.outputHelp()) on integration 2, 4 expected 'adk integration' to throw; expected 'commander.excessArguments' to be 'commander.unknownCommand'
M3: .helpOption(false) on deploy 1, 5 expected 'commander.help' to be 'commander.helpDisplayed'

M1 and M2 are option (b) applied to each group. They reproduce both regressions: deploy bogus stops throwing at all, and integration bogus degrades to excessArguments. All three mutations were reverted; the suite is green at the pushed commit.

One note on the test harness. Command.prototype.command() snapshots the parent exit callback and output config when the child is created, so the outer program.exitOverride() covers the root only. The new beforeEach walks program.commands and applies both to every child. Without that walk, parsing ['deploy'] calls the real process.exit(1) and kills the vitest worker.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Run npm run build, then run the built entrypoint. Measured output:

$ node dev/dist/esm/cli_entrypoint.js deploy       # exit=1  stdout=0B    stderr=328B
$ node dev/dist/esm/cli_entrypoint.js integration  # exit=1  stdout=0B    stderr=262B
$ node dev/dist/esm/cli_entrypoint.js deploy --help # exit=0 stdout=328B  stderr=0B
$ node dev/dist/esm/cli_entrypoint.js deploy bogus  # exit=1, "error: unknown command 'bogus'"

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 6, 2026 01:14
`deploy` and `integration` exist only to namespace their children. Their
missing action handler is deliberate, not an omission: commander then treats
a bare group as a usage error and keeps its unknown-subcommand check. State
that at the definition site so a later refactor does not attach a handler.
… groups

Nothing in the suite exercised `adk deploy` or `adk integration` without a
subcommand, so the stream and the exit code were incidental. Pin all five
rows of the contract: both groups write help to stderr and exit 1, both
reject an unknown subcommand, and an explicit --help writes to stdout and
exits 0.
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.

1 participant