Skip to content

Fix: await the conformance run and exit non-zero when a conformance test fails - #839

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/conformance-command-exit-code
Open

Fix: await the conformance run and exit non-zero when a conformance test fails#839
AmaadMartin wants to merge 3 commits into
mainfrom
fix/conformance-command-exit-code

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

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
  2. Or, if no issue exists, describe the change:
    Problem: adk integration conformance always exits 0. The action calls runIntegrationTests without await, and runIntegrationTests returns nothing, so no CI job can gate on the suite. A run that throws also escapes as an unhandled rejection instead of the logger.error line every other command in cli.ts prints.

Solution: runIntegrationTests now returns the number of failed tests, and the action awaits it. The action sets process.exitCode = 1 when a test failed or the run threw. I chose a bare count over a {passed, skipped, failed} object because the CLI reads exactly one fact; the other two fields would have no reader. I chose process.exitCode over process.exit(1) because the run writes its whole summary immediately before, and process.exit() discards stdout writes still pending on a pipe — the CI case this fix exists to serve.

Behaviour change: a caller that relied on this command exiting 0 after failures was relying on the bug.

Collision check: gh pr list --repo AmaadMartin/adk-js --state open --limit 200, then gh pr diff --name-only on every adjacent PR. Three PRs touch the same files, none implements this fix:

I branched from main rather than stacking, because the three overlaps are mutually exclusive bases and a stacked base gets no CI here.

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 dev/test/integration/run_integration_tests_result_test.ts
#  Test Files  2 passed (2)
#       Tests  34 passed (34)
npm run lint          # clean
npm run build         # clean
npx prettier --check <the four changed files>   # clean

npm run ts:check reports 287 pre-existing errors, all under core/test/** and tests/integration/**. None is in dev/, and my change adds none. Several open PRs (#736-#743) are fixing that backlog.

Proving each test can fail. I ran every new test against a mutated source and recorded the failure:

Mutation Test that failed Message
drop await in the action waits for the run expected true to be false
drop await in the action exit 1 on failure expected +0 to be 1
drop await in the action exit 1 on throw expected "error" to be called with arguments
delete if (failedCount > 0) {...} exit 1 on failure expected +0 to be 1
failedCount > 0 -> failedCount >= 0 leaves exit code alone expected 1 to be +0
delete the catch block exit 1 on throw promise rejected "Error: boom" instead of resolving
pass agents_dir as testsDir, forceRunAll: false passes parsed options expected "spy" to be called with arguments
return failedTests.length -> return 0 returns the failed count; ignores passed and skipped expected +0 to be 2; expected +0 to be 1
return failedTests.length -> + 1 returns 0 with no tests expected 1 to be +0

Integration tests: none. The command's only integration surface is a corpus of conformance YAML, and this repository ships none. A fixture tree is out of proportion to a five-line exit-code fix.

Manual End-to-End (E2E) Tests:
I built the CLI and ran it against synthetic corpora outside the repository. <agents> holds one root_agent.yaml.

npm ci && npm run build
CLI=dev/dist/esm/cli_entrypoint.js

# 1. one failing test
node $CLI integration conformance --agents_dir <agents> --tests_dir <fail>; echo "exit=$?"
#   0 tests passed, 0 tests skipped, 1 tests failed.
#   exit=1        (before this change: exit=0)
#   The full summary block still prints before the process exits.

# 2. one skipped test, nothing failed
node $CLI integration conformance --agents_dir <agents> --tests_dir <skip>; echo "exit=$?"
#   0 tests passed, 1 tests skipped, 0 tests failed.
#   exit=0

# 3. no tests at all
node $CLI integration conformance --agents_dir <agents> --tests_dir <empty>; echo "exit=$?"
#   exit=0

# 4. malformed spec.yaml, to exercise the catch
node $CLI integration conformance --agents_dir <agents> --tests_dir <broken>; echo "exit=$?"
#   [ADK CLI] Error running conformance tests: bad indentation of a mapping entry (2:6)
#   exit=1        (before this change: exit=1 via a raw YAMLException stack dump)

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 3 commits August 8, 2026 19:25
The CLI needs one fact from a conformance run: did anything fail? The
function accumulated the counts and threw them away, so no caller could
tell a clean run from a failed one.
The action dropped the runIntegrationTests promise, so the command exited
0 after a run in which every test failed, and a thrown error surfaced as
an unhandled rejection. It now awaits the run, reports an error through
the CLI logger, and sets process.exitCode.
Sibling suites in this directory are named <module>_test.ts.
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