Test: fail webui_test fast when the repo has not been built (stacked on #359) - #559
Open
AmaadMartin wants to merge 1 commit into
Open
Conversation
The suite serves built output over HTTP: the CLI variant spawns dev/dist/esm/cli_entrypoint.js and the in-process variant mounts dev/dist/browser as the /dev-ui static root. On a partially built tree the failures describe the symptom, not the cause - 'expected 404 to be 200' when the adk-web bundle is absent, 'CLI exited prematurely with code 1' when the entrypoint is. Check both artifacts in an outer beforeAll and throw once, naming the missing paths and the build command.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no public issue.
Problem:
tests/integration/adk_web/webui_test.tsis the only test in therepo that is inherently a built-artifact test: it asserts that the compiled
ADK CLI and the packaged adk-web bundle serve the dev UI over HTTP. When the
tree is not (fully) built, it fails with messages that describe the symptom and
never name the cause. Both failure modes were reproduced locally on this branch:
Using ADK API servervariant —AdkApiServermountsexpress.static(path.join(__dirname, '../../browser'))(
dev/src/server/adk_api_server.ts), i.e.dev/dist/browser, whichensureBrowserAssets()indev/build.jspopulates by downloading thegoogle/adk-webv1.0.0adk-web-browser.ziprelease asset. With thatdirectory absent,
GET /dev-ui/falls through to Express's default 404handler and the suite reports
AssertionError: expected 404 to be 200.Run from ADK CLIvariant —AdkTsApiServerspawnsnode <repo>/dev/dist/esm/cli_entrypoint.js web …. With that file absent,nodeexits 1 andBaseTestServer.startProcessrejects withError: CLI exited prematurely with code 1.Note this is not a 60 s spawn-timeout burn, as one might assume:
spawnsucceeds and
nodeexits immediately, so the rejection is prompt (measured7.57 s wall for the whole vitest run, dominated by transform/collect). The DX
problem is the message, not the duration.
The
dist/esmpresent +dist/browsermissing state is not exotic:dev/build.jsmain()runs esbuild first andensureBrowserAssets()last, soan offline machine or a failed release fetch leaves exactly that state.
Solution: a precondition guard inside that one test file. An outer
beforeAllchecks the two artifacts withfs.existsSyncand, if either ismissing, throws once naming the build command and the missing absolute path(s):
beforeAllon the outerdescribe(rather than module scope) runs once, aheadof both nested
describe.eachsuites' ownbeforeAllhooks, so itshort-circuits both variants before the 20 s server-start hook and is reported
as a suite failure rather than a file-collection error. On a built repo the
guard costs two
existsSyncstats and behaviour is unchanged.Scope is deliberately one file, 24 added lines. Three things were considered
and not done:
vitest.config.tswas not touched. Theintegrationproject aliases only@google/adkand@google/adk-integrations;@google/adk-devtoolsresolvesthrough the workspace symlink to
dev/dist/esm/index.js. Adding an alias todev/srcwould relocate__dirnameinsideadk_api_server.tsand move theexpress.staticroot to the nonexistentdev/browser, breaking theUsing ADK API servervariant outright.tests/integration/test_api_server.ts. A sharedguard would also cover the
tests/integration/a2a/**suites andtests/cross_language/a2a/go_ts/; that is tracked as separate work, andpre-empting it here would create a conflicting diff.
a helper cluster, and extracting it would break the one-file scope.
Accepted limitation (stated plainly): when
dev/distis absent entirely,the static ESM import of
@google/adk-devtoolsat the top of the file fails toresolve before any module body or hook executes, so the guard cannot fire. That
case already produces a self-describing error naming
dev/dist/esm/index.js, sono machinery was added to chase it. The guard covers the partial-build states,
which are the ones that produce misleading messages. Converting the import to a
dynamic
await import()to beat this would change thedescribe.eachargumentshape for near-zero benefit.
CI impact: none.
.github/workflows/validation.yamlrunsnpm run buildbefore
npm run test:coverage, so the guard never fires on GitHub Actions. Thisis a pure local-developer-experience fix. Coverage thresholds are likewise
untouched: the coverage
includeglobs arecore/src/**,dev/src/**,integrations/src/**, so test files are not instrumented and no coverage numberexists for this code.
Collision check. Ran
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000(457 open PRs)and filtered for webui / dev-ui / build-guard work. One PR overlaps:
#359
Fix: name the webui_test hook timeouts and give the teardown an explicit budget, which edits the same file and inserts its constants at the sameanchor (between the
__dirnameshim and the outerdescribe). It does notimplement this guard, so this is an overlap, not a duplicate — this PR is
therefore stacked on
fix/webui-test-named-hook-timeoutsrather than branchedfrom
main, and the guard is written on top of #359'sSERVER_START_TIMEOUT/SERVER_STOP_TIMEOUTconstants. Merge #359 first. Noother open PR touches
tests/integration/adk_web/.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.
No new test file was added: the guard is test-side logic in an integration
suite that is itself the regression surface, and it lives outside the coverage
includeglobs, so no instrumented coverage number exists for it (relocating itinto
dev/src/**to become coverable would ship test scaffolding in a publishedpackage). It is verified instead by exercising every branch of it directly —
both missing-artifact paths and the all-present path — and by control runs
proving the messages it replaces.
Positive run on a fully built tree (
npm install && npm run build):Proof the guard can fail (mutation / negative runs). Each artifact was moved
away in turn and the suite re-run. Both branches of
REQUIRED_BUILD_ARTIFACTS.filter(...)and themissing.length > 0throw areexercised:
Note
tests 6ms/tests 4ms: the guard short-circuits before any serverstarts. The remaining wall time is vitest transform/collect.
Control runs — the same two broken states with the guard reverted
(
git stashof this diff), confirming the guard genuinely replaces themisleading output and that these tests are not passing for free:
Artifacts were restored after each case and the suite re-run green
(
2 passed);git statusshows only the single intended file modified.Local validation of the exact pushed commit (this PR is stacked, so
validation.yaml—pull_request: branches: [main]— does not trigger andno CI test job runs on it; it will run once rebased onto
mainafter #359merges):
npm run ts:checkis already red onmainin this fork (302 pre-existingerrors, e.g.
BASE_AGENT_SIGNATURE_SYMBOLmismatches incore/test/a2a/); itreports 308 on this branch's base both with and without this diff, and zero
of them come from
webui_test.ts. This change introduces no new type errors.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
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.