chore: add e2e & integ tests for web-search#1604
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1604-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.20.2.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Looks good to merge.
The new tests follow the established patterns (matching add-remove-gateway.test.ts and the existing e2e lifecycle suites), use real temp-dir projects rather than fs mocks, exercise both happy-path and validation-error branches, and include telemetry assertions on the success paths for add.gateway-target, remove.gateway-target, add.web-search, and remove.web-search. The e2e suite correctly pins AWS_REGION=us-east-1 (web-search is only available there) and restores it in afterAll, and uses describe.sequential + a short-circuit step() so a failure in an earlier step doesn't waste time on later ones.
A couple of small things I noticed but don't think need to block the merge:
integ-tests/add-remove-web-search.test.tsshares one module-levelcreateTelemetryHelper()across both top-leveldescribeblocks (and both calltelemetry.destroy()in their ownafterAll). It happens to work because the helper's destroy is idempotent (rmSyncwithforce: true) and the asserted command names don't overlap between the two blocks, but it'd be a touch more robust to give each describe its own helper — same asadd-remove-gateway.test.tswould need to do if it grew a second block.- In
e2e-tests/web-search-lifecycle.test.ts, ifcanRunis false thebeforeAllearly-returns without ever touchingAWS_REGION, but theafterAllstill runsif (originalRegion === undefined) delete process.env.AWS_REGION— which would clobber a realAWS_REGIONset on the host. Since each test file runs in its own isolated vitest worker this has no practical effect, but it's slightly cleaner to guard the restore oncanRunas well.
Neither of these warrant a change before merge.
Coverage Report
|
Description
Related Issue
Closes #
Documentation PR
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.