test: assert e2e resources through cli reads#26
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (8)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates E2E coverage to validate created APISIX resources via CLI get/list reads (instead of exercising proxy-chain traffic assertions) and hardens the API client types to handle APISIX upstream node formats returned by the real Admin API.
Changes:
- Refactor E2E “scenario coverage” and upstream health scenarios to assert resources via
a6 ... getJSON reads. - Add
api.UpstreamNodeswith a custom JSON unmarshaler to support upstream nodes returned as either an object map or an array of host/port/weight entries. - Update Route/Service/Upstream/StreamRoute API types to use
UpstreamNodes, with new unit tests covering both node encodings.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/upstream_ginkgo_test.go | Reworks health-check test to assert persisted config via upstream get JSON output. |
| test/e2e/scenario_coverage_ginkgo_test.go | Replaces real gateway traffic assertions with CLI read-based resource assertions. |
| pkg/api/types_upstream_nodes.go | Introduces UpstreamNodes and custom JSON unmarshaling for object/array node formats. |
| pkg/api/types_upstream_nodes_test.go | Adds unit tests for unmarshaling both upstream node representations. |
| pkg/api/types_upstream.go | Switches Upstream.Nodes to UpstreamNodes. |
| pkg/api/types_route.go | Switches inline route upstream Nodes to UpstreamNodes. |
| pkg/api/types_service.go | Switches inline service upstream Nodes to UpstreamNodes. |
| pkg/api/types_stream_route.go | Switches inline stream route upstream Nodes to UpstreamNodes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (n *UpstreamNodes) UnmarshalJSON(data []byte) error { | ||
| if string(data) == "null" { | ||
| *n = nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
UnmarshalJSON compares string(data) == "null" without trimming whitespace. JSON values passed to UnmarshalJSON may include leading/trailing spaces/newlines, so inputs like " null " will currently fall through and error. Consider bytes.TrimSpace(data) (and similarly trimming node.Weight) before comparing against null/"null" to make decoding robust.
| var weight interface{} = 1 | ||
| if len(node.Weight) > 0 && string(node.Weight) != "null" { | ||
| if err := json.Unmarshal(node.Weight, &weight); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| nodes[key] = weight |
There was a problem hiding this comment.
In the array-form parsing, the default weight is set as an int (var weight interface{} = 1), while encoding/json unmarshals numeric values into float64 when targeting interface{}. This can produce mixed numeric types in the same UpstreamNodes map (e.g., float64(2) vs int(1)), which is easy for callers to mishandle. Consider defaulting to float64(1) so numeric weights have a consistent type.
| func TestUpstreamNodesUnmarshalArray(t *testing.T) { | ||
| var nodes UpstreamNodes | ||
| if err := json.Unmarshal([]byte(`[{"host":"127.0.0.1","port":9080,"weight":2}]`), &nodes); err != nil { | ||
| t.Fatalf("unmarshal array nodes: %v", err) | ||
| } | ||
|
|
||
| if got := nodes["127.0.0.1:9080"]; got != float64(2) { | ||
| t.Fatalf("unexpected node weight: %#v", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new UpstreamNodes unmarshaler adds behavior beyond the current tests (e.g., trimming/handling whitespace around null, and defaulting a missing weight to 1 in the array form). Consider adding unit tests that cover these edge cases (e.g., nodes: null with surrounding whitespace, and an array entry missing weight) to prevent regressions.
|
Addressed the Copilot review comments by trimming JSON input/weight values before null checks, using float64(1) for default array-form node weights, and adding unit coverage for null nodes plus default weights. |
Summary
Verification