-
Notifications
You must be signed in to change notification settings - Fork 45
Implement integration tests for provider template #767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a MistralAI LlmProviderTemplate manifest and a Gherkin-based integration test suite with step implementations for LLM provider template lifecycle operations (create, retrieve, update, delete, list). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@gateway/it/steps_llm.go`:
- Around line 57-115: The step handler for `the response should contain
oob-templates` currently returns nil on an empty body and uses a hard-coded
expectedCount; change it to treat an empty body as an error (return a
descriptive fmt.Errorf if body == "" or len(body) == 0) and remove the fixed
numeric check against response.Count — instead derive expected count from the
expectedIDs slice (e.g., compare response.Count to len(expectedIDs>) or drop the
Count assertion entirely and only assert that every ID in expectedIDs appears in
response.Templates). Update references in the function (`body`,
`response.Count`, `expectedIDs`) accordingly so the test fails on missing
response and is not brittle when new templates are added.
🧹 Nitpick comments (2)
gateway/gateway-controller/default-llm-provider-templates/mistral-template.yaml (1)
19-42: Template mapping looks good; remember to rebuild gateway images.Line 19-42: The token/model mappings look consistent; please rebuild gateway images before running the suite (
cd gateway && make build-local). As per coding guidelines, please rebuild after gateway-controller changes.gateway/it/steps_llm.go (1)
53-55: URL‑encode filter parameters to avoid malformed queries.Line 53-55: Building the query string via concatenation will break if
filterKeyorfilterValuecontains reserved characters. Please confirm whether the API expects encoded parameters; if yes, useurl.Values(orQueryEscape) to build the query safely.♻️ Proposed refactor
@@ -import ( - "encoding/json" - "fmt" - - "github.com/cucumber/godog" - "github.com/wso2/api-platform/gateway/it/steps" -) +import ( + "encoding/json" + "fmt" + "net/url" + + "github.com/cucumber/godog" + "github.com/wso2/api-platform/gateway/it/steps" +) @@ - ctx.Step(`^I list LLM provider templates with filter "([^"]*)" as "([^"]*)"$`, func(filterKey, filterValue string) error { - return httpSteps.SendGETToService("gateway-controller", "/llm-provider-templates?"+filterKey+"="+filterValue) - }) + ctx.Step(`^I list LLM provider templates with filter "([^"]*)" as "([^"]*)"$`, func(filterKey, filterValue string) error { + q := url.Values{} + q.Set(filterKey, filterValue) + return httpSteps.SendGETToService("gateway-controller", "/llm-provider-templates?"+q.Encode()) + })
Purpose
${subject}
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.