test(e2e): cover scenario combinations#18
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new E2E test file containing two integration tests: one verifying service, plugin configuration, and route behavior together; another validating upstream round-robin traffic distribution across multiple backend nodes. Includes helper utilities for test server creation, URL parsing, and license validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant CLI
participant Gateway
participant Upstream[Service/Upstream]
participant Plugin[Plugin System]
Test->>CLI: Provision service, plugin config, route
Test->>Gateway: Poll GET /scenario-combo
Gateway->>Plugin: Apply plugin transformations
Plugin->>Upstream: Forward to upstream URL
Upstream-->>Plugin: Response + data
Plugin-->>Gateway: Add X-Scenario-Coverage header
Gateway-->>Test: HTTP 200 + JSON response
Test->>Test: Assert upstream URL and plugin header
Test->>CLI: Cleanup delete route/service/config
sequenceDiagram
participant Test
participant CLI
participant Gateway
participant LoadBalancer[Round-robin Upstream]
participant BackendA[Backend A]
participant BackendB[Backend B]
Test->>CLI: Provision upstream (round-robin) with 2 nodes
Test->>CLI: Provision route to upstream
Test->>Gateway: Poll GET /scenario-multi-node
Gateway->>LoadBalancer: Route request via round-robin
LoadBalancer->>BackendA: Forward request
BackendA-->>Gateway: Response with backend-a identifier
Test->>Test: Record backend-a observed
Test->>Gateway: Poll GET /scenario-multi-node again
Gateway->>LoadBalancer: Route request via round-robin
LoadBalancer->>BackendB: Forward request
BackendB-->>Gateway: Response with backend-b identifier
Test->>Test: Assert both backends reached
Test->>CLI: Cleanup delete route/upstream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/scenario_coverage_test.go`:
- Around line 40-49: The service JSON currently hardcodes the upstream node
address when building serviceJSON; change it to use the existing httpbinURL
variable instead of "127.0.0.1:8080" so test environment overrides (HTTPBIN_URL)
are respected. Locate the serviceJSON construction that references serviceID and
replace the upstream node entry with the httpbinURL value (ensure it remains a
string in the formatted JSON). No other behavior should change—only swap the
hardcoded address for httpbinURL.
- Around line 109-115: The assertions in the test are too strict: instead of
asserting result["url"] equals fmt.Sprintf("%s/get", httpbinURL), update the
check to accept gateway-hosted URLs by asserting the URL string ends with or
contains "/get" (use strings.HasSuffix or strings.Contains on the result["url"]
value after converting to string); and for headers, make the extraction of
headers flexible by handling headers :=
result["headers"].(map[string]interface{}) then reading v :=
headers["X-Scenario-Coverage"] and accepting either a string or an array
([]interface{})—normalize to a []string or check membership so that
"service-route-plugin-config" is accepted whether the header decodes as a single
string or as an array; adjust the require/assert calls accordingly in
scenario_coverage_test.go around the json.Unmarshal/result/headers checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1a10776-b08e-4031-a8c2-9dd74c568fa2
📒 Files selected for processing (1)
test/e2e/scenario_coverage_test.go
There was a problem hiding this comment.
Pull request overview
Adds new end-to-end scenario tests to increase coverage for “combination” gateway configuration cases and multi-node upstream real-traffic behavior, with conditional skipping in license-gated environments.
Changes:
- Add an E2E test covering
service + route + plugin-configworking together through the gateway. - Add an E2E test validating real traffic reaches multiple upstream nodes (round-robin).
- Add a helper to skip these scenarios when resource creation fails due to a license gate.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert.Equal(t, fmt.Sprintf("%s/get", httpbinURL), result["url"]) | ||
|
|
There was a problem hiding this comment.
The test assumes httpbin’s JSON field url will be HTTPBIN_URL + "/get", but httpbin typically builds this value from the incoming Host header. APISIX commonly preserves the client Host header by default, so this may end up being the gateway host/port (e.g. 127.0.0.1:9080) and make the assertion fail even when the proxying works. Consider asserting on the parsed URL path (equals /get) and/or that it ends with /get, rather than matching the full host.
| assert.Equal(t, fmt.Sprintf("%s/get", httpbinURL), result["url"]) | |
| returnedURL, ok := result["url"].(string) | |
| require.True(t, ok, "httpbin response should expose url as a string") | |
| assert.True(t, strings.HasSuffix(returnedURL, "/get"), "httpbin response url should end with /get, got %q", returnedURL) |
| seen := map[string]bool{} | ||
| for i := 0; i < 12; i++ { | ||
| resp, err := http.Get(gatewayURL + "/scenario-multi-node") | ||
| require.NoError(t, err, "gateway request should succeed") | ||
|
|
||
| payload, readErr := io.ReadAll(resp.Body) | ||
| resp.Body.Close() | ||
| require.NoError(t, readErr) | ||
|
|
||
| if resp.StatusCode == http.StatusOK { | ||
| seen[strings.TrimSpace(string(payload))] = true | ||
| if seen["backend-a"] && seen["backend-b"] { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| time.Sleep(200 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
The retry budget here is only ~2.4s (12 * 200ms). Other e2e tests in this package typically allow ~5s (10 * 500ms) for config propagation. This shorter window can introduce flakes in slower environments; consider using the same retry duration pattern (or a time-based deadline) before asserting that both backends were seen.
| func hostPort(t *testing.T, rawURL string) string { | ||
| t.Helper() | ||
|
|
||
| host := strings.TrimPrefix(rawURL, "http://") | ||
| _, _, err := net.SplitHostPort(host) | ||
| require.NoError(t, err) | ||
| return host |
There was a problem hiding this comment.
hostPort currently strips only the http:// prefix and then relies on net.SplitHostPort. This is a bit brittle (e.g., if the URL format changes, or if an https:// server is used later). Parsing via net/url and using u.Host would make the helper more robust and self-documenting.
Summary
Validation
Refs #14
Summary by CodeRabbit