Add integration tests for llm entities in the platform API#1273
Add integration tests for llm entities in the platform API#1273
Conversation
WalkthroughAdds comprehensive mock repository implementations and unit test cases for LLMProviderService and LLMProxyService, expanding test coverage for provider/proxy creation, duplicate conflict handling, upstream/authentication preservation during updates, and list retrieval operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/service/llm_test.go (1)
441-441:⚠️ Potential issue | 🟡 MinorMissing coverage for 2-part version normalization.
TestGenerateLLMProviderDeploymentYAML_NormalizesPolicyVersionToMajoronly exercises 3-part semver inputs ("0.1.0"→"v0"and"v10.2.3"→"v10"). This test implicitly relies on"0.1"normalizing to"v0"at line 441, but that 2-part case is never explicitly covered by the normalization test. Consider adding a"0.1"→"v0"case toTestGenerateLLMProviderDeploymentYAML_NormalizesPolicyVersionToMajorto make the contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/llm_test.go` at line 441, The test TestGenerateLLMProviderDeploymentYAML_NormalizesPolicyVersionToMajor currently only asserts 3-part semver normalization and implicitly relies on a 2-part case; update that test to include an explicit 2-part input like "0.1" and assert it normalizes to "v0" (same style as existing cases), updating the test's input table/array and expected output; locate the test and the call sites to findPolicy in the generated YAML assertions (e.g., guardrailPolicy := findPolicy(...)) and add an entry and corresponding expectation for "0.1" → "v0" so the 2-part normalization is covered.
🧹 Nitpick comments (1)
platform-api/src/internal/service/llm_test.go (1)
1016-1019:mockLLMProxyRepo.Createis missingcreateCalledtracking.Unlike
mockLLMProviderRepo.Createwhich sets bothm.createCalled = trueandm.created = p, the proxy mock only capturesm.created. This inconsistency makes it impossible to assert thatCreatewas not called (analogous to howTestLLMProviderServiceCreateRejectsMultipleModelProvidersForNativeTemplateassertsproviderRepo.createCalledat line 1071).♻️ Proposed fix
+type mockLLMProxyRepo struct { + repository.LLMProxyRepository + existsResult bool + countResult int + countByProviderValue int + listByProviderItems []*model.LLMProxy + lastListProviderUUID string + getByIDFunc func(proxyID, orgUUID string) (*model.LLMProxy, error) + createCalled bool + created *model.LLMProxy + updated *model.LLMProxy +} func (m *mockLLMProxyRepo) Create(p *model.LLMProxy) error { + m.createCalled = true m.created = p return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/llm_test.go` around lines 1016 - 1019, The mockLLMProxyRepo.Create implementation should mirror mockLLMProviderRepo.Create by setting a boolean flag when invoked so tests can assert whether Create was called; add a createCalled bool field on mockLLMProxyRepo and set m.createCalled = true inside the Create(p *model.LLMProxy) method (in addition to m.created = p) so tests like TestLLMProviderServiceCreateRejectsMultipleModelProvidersForNativeTemplate can assert on proxyRepo.createCalled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/internal/service/llm_test.go`:
- Around line 1202-1215: The test constructs the service with a nil projectRepo
which makes it fragile to internal validation order; update
TestLLMProxyServiceCreateReturnsConflictForDuplicateHandle to pass a no-op
mockProjectRepo (e.g., a mock implementing the projectRepo interface whose
relevant methods return nil, nil or false,nil) into NewLLMProxyService instead
of nil so the test no longer depends on Create's internal check ordering (refer
to NewLLMProxyService,
TestLLMProxyServiceCreateReturnsConflictForDuplicateHandle and
mockProjectRepo/mockProjectRepo methods).
- Around line 1333-1335: Remove the redundant stringPtr helper and replace its
uses with the existing strPtr helper: delete the stringPtr function definition
in llm_test.go and update every call site that currently uses stringPtr(...) to
use strPtr(...); verify other tests (llm_test.go and deployment_test.go in
package service) compile and no remaining references to stringPtr exist.
---
Outside diff comments:
In `@platform-api/src/internal/service/llm_test.go`:
- Line 441: The test
TestGenerateLLMProviderDeploymentYAML_NormalizesPolicyVersionToMajor currently
only asserts 3-part semver normalization and implicitly relies on a 2-part case;
update that test to include an explicit 2-part input like "0.1" and assert it
normalizes to "v0" (same style as existing cases), updating the test's input
table/array and expected output; locate the test and the call sites to
findPolicy in the generated YAML assertions (e.g., guardrailPolicy :=
findPolicy(...)) and add an entry and corresponding expectation for "0.1" → "v0"
so the 2-part normalization is covered.
---
Nitpick comments:
In `@platform-api/src/internal/service/llm_test.go`:
- Around line 1016-1019: The mockLLMProxyRepo.Create implementation should
mirror mockLLMProviderRepo.Create by setting a boolean flag when invoked so
tests can assert whether Create was called; add a createCalled bool field on
mockLLMProxyRepo and set m.createCalled = true inside the Create(p
*model.LLMProxy) method (in addition to m.created = p) so tests like
TestLLMProviderServiceCreateRejectsMultipleModelProvidersForNativeTemplate can
assert on proxyRepo.createCalled.
| func TestLLMProxyServiceCreateReturnsConflictForDuplicateHandle(t *testing.T) { | ||
| proxyRepo := &mockLLMProxyRepo{existsResult: true} | ||
| providerRepo := &mockLLMProviderRepo{ | ||
| getByIDFunc: func(providerID, orgUUID string) (*model.LLMProvider, error) { | ||
| return &model.LLMProvider{UUID: "provider-uuid", ID: providerID}, nil | ||
| }, | ||
| } | ||
| service := NewLLMProxyService(proxyRepo, providerRepo, nil) | ||
|
|
||
| _, err := service.Create("org-1", "alice", validProxyRequest("provider-1", "project-1")) | ||
| if err != constants.ErrLLMProxyExists { | ||
| t.Fatalf("expected ErrLLMProxyExists, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nil projectRepo creates a fragile dependency on internal check ordering.
NewLLMProxyService(proxyRepo, providerRepo, nil) passes nil for projectRepo. The test will panic if the service's Create implementation validates project membership before the proxy-uniqueness check. Consider passing a no-op mockProjectRepo (e.g. one returning nil, nil) to eliminate the implicit ordering dependency.
🛡️ Proposed fix
- service := NewLLMProxyService(proxyRepo, providerRepo, nil)
+ projectRepo := &mockProjectRepo{project: nil}
+ service := NewLLMProxyService(proxyRepo, providerRepo, projectRepo)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/internal/service/llm_test.go` around lines 1202 - 1215, The
test constructs the service with a nil projectRepo which makes it fragile to
internal validation order; update
TestLLMProxyServiceCreateReturnsConflictForDuplicateHandle to pass a no-op
mockProjectRepo (e.g., a mock implementing the projectRepo interface whose
relevant methods return nil, nil or false,nil) into NewLLMProxyService instead
of nil so the test no longer depends on Create's internal check ordering (refer
to NewLLMProxyService,
TestLLMProxyServiceCreateReturnsConflictForDuplicateHandle and
mockProjectRepo/mockProjectRepo methods).
| func stringPtr(s string) *string { | ||
| return &s | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n 'func str[A-Za-z]*[Pp]tr\b|func stringPtr\b' --type go platform-api/src/internal/service/Repository: wso2/api-platform
Length of output: 236
🏁 Script executed:
rg -n 'strPtr\|stringPtr' platform-api/src/internal/service/llm_test.go | head -50Repository: wso2/api-platform
Length of output: 43
🏁 Script executed:
# Check if strPtr is defined anywhere in llm_test.go
rg 'func strPtr' platform-api/src/internal/service/llm_test.go
# Check specific lines mentioned in the comment to see what's actually there
sed -n '255p; 367p; 479p; 531p; 644p; 786p' platform-api/src/internal/service/llm_test.goRepository: wso2/api-platform
Length of output: 260
🏁 Script executed:
# Search for strPtr definition in the entire llm_test.go file
rg -B2 -A2 'func.*strPtr' platform-api/src/internal/service/llm_test.go
# Also check if there's an import or helper package that defines it
rg 'strPtr' platform-api/src/internal/service/llm_test.go -B1 -A1 | head -30Repository: wso2/api-platform
Length of output: 621
🏁 Script executed:
# Check package declaration for both test files
head -10 platform-api/src/internal/service/llm_test.go | grep package
head -10 platform-api/src/internal/service/deployment_test.go | grep packageRepository: wso2/api-platform
Length of output: 76
Remove stringPtr and use the existing strPtr helper instead.
Both llm_test.go and deployment_test.go are in the same package service, making the helpers accessible across test files. The strPtr function is already defined in deployment_test.go and is used throughout llm_test.go (lines 255, 367, 479, 531, 644, 786). The new stringPtr helper is identical and redundant. Delete the new helper and replace its usage with strPtr.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/internal/service/llm_test.go` around lines 1333 - 1335,
Remove the redundant stringPtr helper and replace its uses with the existing
strPtr helper: delete the stringPtr function definition in llm_test.go and
update every call site that currently uses stringPtr(...) to use strPtr(...);
verify other tests (llm_test.go and deployment_test.go in package service)
compile and no remaining references to stringPtr exist.
2 participants
Purpose
Summary by CodeRabbit