pkg/status: unit test-case for api.go#1595
pkg/status: unit test-case for api.go#1595yashisrani wants to merge 3 commits intokmesh-net:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new set of unit tests for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Code is made anew, Tests confirm its true purpose, Bugs flee from sight. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of unit tests for the status API conversion logic in pkg/status/api.go. The tests cover various scenarios for Workload, Service, AuthorizationPolicy, and BPF data structures, which is a great addition for ensuring code quality and preventing regressions. However, I found a critical issue in one of the test cases for TestWorkloadBpfDump where the assertion is incorrect based on the current implementation, which will cause the test to fail. Please see the detailed comment.
| expected := BpfWorkloadPolicyValue{PolicyIds: []string{"policy-1"}} | ||
| assert.Equal(t, []BpfWorkloadPolicyValue{expected}, res.WorkloadPolicies) |
There was a problem hiding this comment.
The assertion in this test case is incorrect given the current implementation of WithWorkloadPolicies in api.go, and it will cause the test to fail.
The PolicyIds field of bpfcache.WorkloadPolicyValue is a fixed-size array ([4]uint32). When initialized with a single value, the remaining elements are zero. The WithWorkloadPolicies function iterates over all elements but doesn't filter out the empty strings resulting from zero-valued IDs. Consequently, the actual result for PolicyIds will be []string{"policy-1", "", "", ""}.
To resolve this, the implementation of WithWorkloadPolicies should be updated to filter out zero IDs or empty strings, similar to how WithBackends is implemented. This would make the code more robust and consistent.
If you update api.go, this test will correctly catch the bug and then pass after the fix. If changing api.go is not intended, then this test's expectation needs to be corrected to match the actual output.
There was a problem hiding this comment.
Pull request overview
Adds a new pkg/status/api_test.go unit test file to validate the “Adapter” conversion helpers in pkg/status/api.go (Workload/Service/AuthorizationPolicy conversions and WorkloadBpfDump formatting) that translate internal proto/BPF-shaped data into JSON-friendly structs used by higher-level components.
Changes:
- Add table-driven tests for
ConvertWorkloadandConvertService. - Add a direct test for
ConvertAuthorizationPolicy. - Add tests for
WorkloadBpfDumpconversion helpers (WithWorkloadPolicies/WithBackends/WithEndpoints/WithServices).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: "basic workload", | ||
| input: &workloadapi.Workload{ | ||
| Uid: "uid-1", | ||
| Addresses: [][]byte{net.ParseIP("192.168.1.1")}, | ||
| Name: "workload-1", | ||
| Namespace: "default", | ||
| Status: workloadapi.WorkloadStatus_HEALTHY, | ||
| }, | ||
| expected: &Workload{ | ||
| Uid: "uid-1", | ||
| Addresses: []string{"192.168.1.1"}, | ||
| Name: "workload-1", | ||
| Namespace: "default", | ||
| Status: "HEALTHY", | ||
| Protocol: "HBONE", // Default enum value 0 is HBONE | ||
| WorkloadType: "POD", // Default enum value 0 is POD | ||
| }, | ||
| }, |
There was a problem hiding this comment.
ConvertWorkload uses w.TunnelProtocol.String() and w.WorkloadType.String(). In the proto, the zero values are TunnelProtocol_NONE and WorkloadType_DEPLOYMENT, so these test cases will fail unless you explicitly set TunnelProtocol: TunnelProtocol_HBONE and WorkloadType: WorkloadType_POD (or update the expected values/comments to match the real defaults).
| { | ||
| name: "workload with waypoint address", | ||
| input: &workloadapi.Workload{ | ||
| Uid: "uid-2", | ||
| Waypoint: &workloadapi.GatewayAddress{ | ||
| Destination: &workloadapi.GatewayAddress_Address{ | ||
| Address: &workloadapi.NetworkAddress{ | ||
| Network: "network-1", | ||
| Address: net.ParseIP("10.0.0.1"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expected: &Workload{ | ||
| Uid: "uid-2", | ||
| Waypoint: "network-1/10.0.0.1", | ||
| Addresses: []string{}, | ||
| Protocol: "HBONE", | ||
| WorkloadType: "POD", | ||
| Status: "HEALTHY", | ||
| }, | ||
| }, | ||
| { | ||
| name: "workload with waypoint hostname", | ||
| input: &workloadapi.Workload{ | ||
| Uid: "uid-3", | ||
| Waypoint: &workloadapi.GatewayAddress{ | ||
| Destination: &workloadapi.GatewayAddress_Hostname{ | ||
| Hostname: &workloadapi.NamespacedHostname{ | ||
| Namespace: "ns-1", | ||
| Hostname: "host-1", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expected: &Workload{ | ||
| Uid: "uid-3", | ||
| Waypoint: "ns-1/host-1", | ||
| Addresses: []string{}, | ||
| Protocol: "HBONE", | ||
| WorkloadType: "POD", | ||
| Status: "HEALTHY", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The expected Protocol/WorkloadType/Status values in these waypoint-only workloads rely on enum defaults. With the current proto definitions, an unset workload defaults to TunnelProtocol_NONE and WorkloadType_DEPLOYMENT (and Status_HEALTHY). Either set those fields on the input explicitly or change the expected values so the test reflects actual behavior.
| // Null the slices for the equal check below | ||
| actual.Services = nil | ||
| tc.expected.Services = nil | ||
| assert.Equal(t, tc.expected, actual) |
There was a problem hiding this comment.
This test mutates tc.expected.Services (and actual.Services) to make the final assert.Equal pass. That makes the table-driven cases harder to reason about and can cause surprises if these expected structs are reused later. Prefer comparing a copy (e.g., deep-copy expected into a local variable) or asserting the Services slice separately without mutating the expected value.
| // Null the slices for the equal check below | |
| actual.Services = nil | |
| tc.expected.Services = nil | |
| assert.Equal(t, tc.expected, actual) | |
| // Compare the rest of the struct while leaving the original test case data unmodified. | |
| expectedCopy := *tc.expected | |
| actualCopy := *actual | |
| expectedCopy.Services = nil | |
| actualCopy.Services = nil | |
| assert.Equal(t, expectedCopy, actualCopy) |
| hashName := utils.NewHashName() | ||
| policyId := hashName.Hash("policy-1") | ||
| backendUid := hashName.Hash("backend-1") | ||
| serviceId := hashName.Hash("service-1") | ||
|
|
||
| dump := NewWorkloadBpfDump(hashName) |
There was a problem hiding this comment.
utils.NewHashName() reads/writes a persisted mapping at /mnt/hash_name.yaml. Without cleanup, this test can become order-dependent/flaky (e.g., if another test already populated an entry for ID 0, conversions that skip/keep zeros can change). Consider resetting/isolating the persisted state (e.g., call hashName.Reset() before/after, or construct a fresh HashName after a reset) so this test is hermetic.
| PolicyIds: [4]uint32{policyId}, | ||
| }, | ||
| } | ||
| res := dump.WithWorkloadPolicies(policies) | ||
| expected := BpfWorkloadPolicyValue{PolicyIds: []string{"policy-1"}} |
There was a problem hiding this comment.
bpfcache.WorkloadPolicyValue.PolicyIds is a fixed-size array; here only the first element is set and the rest remain 0. WorkloadBpfDump.WithWorkloadPolicies currently converts every element without filtering, so the output will include empty strings for the zero IDs (or potentially non-empty strings if ID 0 exists in the persisted HashName mapping). Either populate all IDs, update the expected value to include the extra entries, or change the converter to skip zero/unknown IDs (consistent with WithBackends).
| PolicyIds: [4]uint32{policyId}, | |
| }, | |
| } | |
| res := dump.WithWorkloadPolicies(policies) | |
| expected := BpfWorkloadPolicyValue{PolicyIds: []string{"policy-1"}} | |
| PolicyIds: [4]uint32{ | |
| hashName.Hash("policy-1"), | |
| hashName.Hash("policy-2"), | |
| hashName.Hash("policy-3"), | |
| hashName.Hash("policy-4"), | |
| }, | |
| }, | |
| } | |
| res := dump.WithWorkloadPolicies(policies) | |
| expected := BpfWorkloadPolicyValue{PolicyIds: []string{"policy-1", "policy-2", "policy-3", "policy-4"}} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Addresses: []string{}, | ||
| Protocol: "NONE", | ||
| WorkloadType: "POD", | ||
| Status: "HEALTHY", | ||
| }, |
There was a problem hiding this comment.
input.WorkloadType is not set here, so it defaults to WorkloadType_DEPLOYMENT (enum value 0). ConvertWorkload will therefore return WorkloadType: "DEPLOYMENT", not "POD". Update expected or set input.WorkloadType explicitly.
| Addresses: []string{}, | ||
| Protocol: "NONE", | ||
| WorkloadType: "POD", | ||
| Status: "HEALTHY", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
input.WorkloadType is unset here (defaults to enum value 0 = DEPLOYMENT), so ConvertWorkload will return WorkloadType: "DEPLOYMENT". Adjust expected or set input.WorkloadType explicitly.
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
yashisrani
force-pushed
the
test-case/api.go
branch
from
e4a0db2 to
3afa355
Compare
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: