test(envoy): add missing unit tests for envoy util functions#2529
Conversation
|
@yehuditkerido: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @yehuditkerido. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@yehuditkerido can you please fix the invalid commit? |
ef6d255 to
1ffdcca
Compare
nirrozenbaum
left a comment
There was a problem hiding this comment.
@yehuditkerido thanks, overall looks good.
left few minor comments, also needs to rebase :)
| @@ -82,6 +85,89 @@ func TestExtractHeaderValue(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestGenerateHeadersMutation(t *testing.T) { | |||
| }, | ||
| }, | ||
| { | ||
| name: "RequestBody response with unsorted headers gets sorted", |
There was a problem hiding this comment.
is this test needed considering the previous one? isn't it testing the same?
There was a problem hiding this comment.
Each one tests a different case branch in the type switch — RequestHeaders vs RequestBody — so they cover different code paths. But since the "multiple response objects" test already uses both, I will remove this one.
| }, | ||
| }, | ||
| { | ||
| name: "unknown response type is skipped", |
There was a problem hiding this comment.
an is an unknown response?
There was a problem hiding this comment.
It's a response type that the function doesn't handle in its type switch. Renamed the test to make it clearer.
| }, | ||
| }, | ||
| { | ||
| name: "mixed response types sorted independently", |
There was a problem hiding this comment.
I wouldn't say this is response types, maybe response objects?
| name: "mixed response types sorted independently", | |
| name: "multiple response objects are sorted independently", |
a63cbc4 to
47c251a
Compare
| name: "unhandled response type is skipped", | ||
| responses: []*extProcPb.ProcessingResponse{ | ||
| { | ||
| Response: &extProcPb.ProcessingResponse_ResponseHeaders{ |
There was a problem hiding this comment.
ahhhh now got it. it's a ResponseHeaders.
thanks for clarifying.
I actually think we should update the test util function to cover that as well. how do we compare responses today in the tests for response pugins?
There was a problem hiding this comment.
Today tests are working since ResponseHeader is with single header so no sorting needed. Added ResponseHeaders and ResponseBody cases to the type switch, with tests for both. The "unhandled" test now uses ResponseTrailers instead.
…adersInResponses These envoy util functions lacked test coverage. Add 13 table-driven test cases covering edge cases like nil safety, empty inputs, map iteration non-determinism, and mixed response types. Signed-off-by: Yehudit Kerido <yehudit1987@gmail.com>
47c251a to
5e15ff5
Compare
nirrozenbaum
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thanks!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nirrozenbaum, yehuditkerido The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…adersInResponses (kubernetes-sigs#2529) These envoy util functions lacked test coverage. Add 13 table-driven test cases covering edge cases like nil safety, empty inputs, map iteration non-determinism, and mixed response types. Signed-off-by: Yehudit Kerido <yehudit1987@gmail.com>
…adersInResponses (kubernetes-sigs/gateway-api-inference-extension#2529) These envoy util functions lacked test coverage. Add 13 table-driven test cases covering edge cases like nil safety, empty inputs, map iteration non-determinism, and mixed response types. Signed-off-by: Yehudit Kerido <yehudit1987@gmail.com>
…adersInResponses (kubernetes-sigs/gateway-api-inference-extension#2529) These envoy util functions lacked test coverage. Add 13 table-driven test cases covering edge cases like nil safety, empty inputs, map iteration non-determinism, and mixed response types. Signed-off-by: Yehudit Kerido <yehudit1987@gmail.com>
What type of PR is this?
/kind test
What this PR does / why we need it:
Add missing unit tests for
GenerateHeadersMutationandSortSetHeadersInResponses— two envoy util functions that were recently added without test coverage.TestGenerateHeadersMutation— 4 cases covering empty map, single header, multiple headers (with sort for map non-determinism), and empty value.TestSortSetHeadersInResponses— 9 cases covering nil safety,RequestHeaders/RequestBodysorting, single header skip, unknown response type skip, nilHeaderMutation, and mixed response types.Which issue(s) this PR fixes:
Fixes #2528
Does this PR introduce a user-facing change?: