Skip to content

fix: run bedrock-kb store test in node only#2966

Merged
opieter-aws merged 1 commit into
strands-agents:mainfrom
opieter-aws:opieter-aws/fix-kb-store-browser-test
Jun 25, 2026
Merged

fix: run bedrock-kb store test in node only#2966
opieter-aws merged 1 commit into
strands-agents:mainfrom
opieter-aws:opieter-aws/fix-kb-store-browser-test

Conversation

@opieter-aws

@opieter-aws opieter-aws commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

bedrock-knowledge-base-store.test.ts mocks the AWS SDK clients (@aws-sdk/client-bedrock-agent-runtime, @aws-sdk/client-bedrock-agent, @aws-sdk/client-s3) with vi.mock and, in four of its cases, asserts on the mocked constructor — e.g. expect(vi.mocked(BedrockAgentRuntimeClient)).toHaveBeenCalledWith({}). Under the unit-browser (chromium) project, the AWS-SDK module mock isn't applied the way it is in Node, so the imported BedrockAgentRuntimeClient is the real class rather than a vi.fn(), and the assertion throws TypeError: [Function BedrockAgentRuntimeClient] is not a spy or a call to a spy!.

Because the file is named *.test.ts, vitest runs it in both the unit-node and unit-browser projects (see vitest.config.ts). The constructor-spy assertions only make sense in Node, where the SDK mock takes effect — exercising Node AWS SDK client construction has no meaning in a browser environment. This is purely a test-environment mismatch; the store itself is unaffected.

The fix renames the file to bedrock-knowledge-base-store.test.node.ts, which the config scopes to unit-node only (the unit-browser project includes *.test.ts / *.test.browser.ts, not *.test.node.ts). This matches the repository convention for Node-only tests documented in strands-ts/docs/TESTING.md.

Related Issues

None.

Documentation PR

Not applicable.

Type of Change

Bug fix

Testing

Ran the affected suites locally after the rename:

  • npx vitest run --project unit-node …/bedrock-knowledge-base-store.test.node.ts70/70 passed.

  • npx vitest list --project unit-browser → the file is no longer collected by the browser project (so the chromium failure can't recur).

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@opieter-aws opieter-aws marked this pull request as ready for review June 25, 2026 18:47
@github-actions github-actions Bot added typescript Pull requests that update typescript code area-community Related to community and contributor health bug Something isn't working strands-running labels Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment (lean approve)

Clean, minimal, correctly-scoped fix. The 100% rename moves the AWS-SDK-mocking suite into the *.test.node.ts bucket that vitest.config.ts runs in unit-node only, matching the convention in strands-ts/docs/TESTING.md. I verified the diff is content-free and that nothing else in the repo (including site/ sourceLinks) references the old filename.

Review notes
  • Correctness: Rename resolves the browser constructor-spy TypeError; config scoping confirmed. ✅
  • References: No stale references to the old path anywhere. ✅
  • Scope/API: Tiny, focused, test-only — no public API impact. ✅
  • One open question (non-blocking): A sibling suite (bedrock.test.ts) mocks the same SDK and keeps browser coverage via an async importOriginal factory. Worth confirming whether browser coverage of this store should be preserved that way, or whether node-only is intentional — see the inline comment.

Nice, well-written PR description that made the root cause easy to follow.

@opieter-aws opieter-aws merged commit 188d474 into strands-agents:main Jun 25, 2026
35 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-community Related to community and contributor health bug Something isn't working size/xs typescript Pull requests that update typescript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants