Skip to content

test(dotAI): add BedrockModelProviderStrategy unit tests#36023

Open
sfreudenthaler wants to merge 1 commit into
dot-ai-langchain-amazon-bedrockfrom
dot-ai-bedrock-strategy-tests
Open

test(dotAI): add BedrockModelProviderStrategy unit tests#36023
sfreudenthaler wants to merge 1 commit into
dot-ai-langchain-amazon-bedrockfrom
dot-ai-bedrock-strategy-tests

Conversation

@sfreudenthaler
Copy link
Copy Markdown
Member

@sfreudenthaler sfreudenthaler commented Jun 5, 2026

Stacked PR 1/3 into #35242 (dot-ai-langchain-amazon-bedrock) — merge or close independently; 2/3 (timeout/maxRetries) and 3/3 (docs) build on this. Offered as optional help, @ihoffmann-dot — feel free to dismiss.

Proposed Changes

  • New BedrockModelProviderStrategyTest (18 tests) mirroring the Azure/Vertex test conventions, locking in current behavior before further changes:
    • credential selection: static keys vs default chain; exactly-one-key set → IllegalArgumentException
    • region/model validation messages
    • chat + streaming happy paths; temperature/maxTokens param-mapping branches
    • embedding family routing: cohere.* / amazon.titan-* (case-insensitive), unsupported family error, embeddingInputType passthrough
    • image → UnsupportedOperationException

Checklist

  • Tests: BedrockModelProviderStrategyTest 18/18, LangChain4jModelFactoryTest 38/38 (no regression)
  • No translations needed
  • No security implications (test-only)

🤖 Generated with Claude Code

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 5, 2026

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • This PR fixes #123 or Fixes: #123

  • This PR closes #123 or Closes: #123

  • This PR resolves #123 or Resolves: #123

  • Other supported keywords: fix, fixed, close, closed, resolve, resolved
    Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jun 5, 2026

Claude finished @sfreudenthaler's task in 1m 13s —— View job


Code Review

Tests are clean and well-matched to the implementation. A few items worth noting:


1. assertTrue(x.equals(y)) instead of assertEquals — minor but produces an unhelpful failure message (expected true but was false rather than expected <bedrock> but was <...>).

BedrockModelProviderStrategyTest.java:38

// current
assertTrue("bedrock".equals(strategy.providerName()));
// better
assertEquals("bedrock", strategy.providerName());

2. validate() checks region before model, but test_buildChatModel_missingRegion_throws doesn't expose this ordering dependency.

The test config at line 113 omits region but supplies both keys — that's fine. However, if someone later inverts the validate order (model before region), the region test would still pass because region is the only missing field. The tests are sound as written; just noting the ordering assumption is implicit.


3. No validation path tests for buildEmbeddingModel.

buildEmbeddingModel calls validate(config, modelType) identically to the chat methods, but there are no tests for missing region/model on the embedding path. The routing (cohere.*, amazon.titan-*) is tested well; the validation gate before it is not.


4. Half-key error not tested on the streaming path.

buildStreamingChatModel goes through the same credentials() check, but test_buildChatModel_onlyKeyId_throwsBothOrNeither / test_buildChatModel_onlySecret_throwsBothOrNeither only exercise the sync chat path. Low risk since it's the same private method, but worth noting the gap.


5. test_buildEmbeddingModel_unsupportedFamily_throws uses a chat model ID (openai.gpt-oss-120b-1:0).

The string check at line 287–288 ("cohere.", "amazon.titan-") is correct against the implementation's error message. No bug, but it's slightly odd that the embedding unsupported-family test uses a model ID from the chat tests — a cleaner name like "mistral.mistral-7b" would make the intent clearer.


Summary: Clean tests overall — no bugs, no unsafe assumptions, good coverage of the main branches. Items 3 and 4 are the only real gaps; the rest are style nits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant