Skip to content

Test: stop swallowing the parse error in the cloud_run pass-through CLI test - #709

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/cli-test-swallowed-parse-error
Open

Test: stop swallowing the parse error in the cloud_run pass-through CLI test#709
AmaadMartin wants to merge 1 commit into
mainfrom
fix/cli-test-swallowed-parse-error

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):

N/A

  1. Or, if no issue exists, describe the change:

Problem: In dev/test/cli/cli_test.ts, the test should pass args to deployToCloudRun including unknowns wrapped its exercise step in try { await parse(args); } catch (e) { console.log(e); }. A test file must not write to stdout. The catch also degrades the failure signal: it prints the real error and lets execution reach the next line, where mock.calls[0] is undefined, so the test dies on TypeError: Cannot read properties of undefined (reading '0').

Solution: I deleted the try/catch and call await parse(args) bare, like the other 25 parse call sites in the file. The local parse helper (lines 65-74) already absorbs the expected commander.exit, so anything it re-throws is a genuine failure that must fail the test at its own call site. The catch was unreachable on a passing run: cloud_run sets .allowUnknownOption() and .allowExcessArguments() so commander accepts this argv, and the action body catches its own errors while deployToCloudRun is module-mocked. The sibling clean-up in cli_deploy_agent_engine_test.ts is out of scope here because it is already up for review on fix/remove-debug-scaffolding-cli-deploy-agent-engine-test.

Collision check: I listed all open PRs on the fork and diffed the five that touch dev/test/cli/cli_test.ts (#694, #693, #587, #455, #447). None removes this console.log, and no hunk of theirs overlaps lines 303-307.

Testing Plan

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 reports 26 passed, with no stdout | block attributed to the file. npx vitest run --project unit:dev dev/test/cli/cli_deploy_agent_engine_test.ts reports 18 passed and still prints its own debug lines, which confirms I left that file alone.

No test is added. The change removes four lines of test scaffolding and adds no production code, so coverage cannot move: vitest.config.ts measures core/src, dev/src and integrations/src only.

Mutation proof. I made the parse helper reject for this test's argv with if (args.includes('--extra-arg=foo')) throw new Error('mutation probe'); and ran the suite twice.

Before the fix, the error goes to stdout and the reported failure is wrong:

stdout | ... > should pass args to deployToCloudRun including unknowns
Error: mutation probe

TypeError: Cannot read properties of undefined (reading '0')
 ❯ dev/test/cli/cli_test.ts:310:53

After the fix, the reported failure is the real error and stdout is empty:

Error: mutation probe
 ❯ parse dev/test/cli/cli_test.ts:66:49
 ❯ dev/test/cli/cli_test.ts:304:13

I then removed the probe and re-ran: 26 passed.

Manual End-to-End (E2E) Tests:

Not applicable. No user-visible behaviour changes and no production code is touched.

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.

…LI test

The exercise step wrapped `await parse(args)` in a try/catch whose only
body was `console.log(e)`. That violates the no-console rule for tests,
and it replaces an accurate failure with a misleading one: the caught
error goes to stdout and the test then dies on the next line with
`TypeError: Cannot read properties of undefined (reading '0')`.

Let the error propagate, like the other 25 parse call sites in the file.
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