test: Initial end-to-end test with gRPC messages #820
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: irar2 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @irar2! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @irar2. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/retest |
| namespace = "ns1" | ||
| ) | ||
|
|
||
| func TestServer(t *testing.T) { |
There was a problem hiding this comment.
This seems to be an integration test, not a unit test.
should be placed accordingly in the repo.
additionally, current code base has under integration dir hermetic_test.go which includes some of the code that was added here.
I think it would be good to:
- identify the covered (and uncovered) things that this PR aims to add. we should add only the logic that doesn't exist.
- converge to a single integration test suite. I'm not sure it makes sense to have multiple integration tests with different setups, different structuring, etc.
There was a problem hiding this comment.
I changed the test to become more of a unit test rather than an end to end test
test/utils/server.go
Outdated
| logger := klog.Background() | ||
| ctx := klog.NewContext(context.Background(), logger) | ||
| ctx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
why this is needed in test?
logger is not needed in test for sure. context usually initialized with context.Background() in tests.
|
@irar2 thanks for the PR. Can you resolve the review feedback and update the PR? |
k8s-ci-robot
added
do-not-merge/invalid-commit-message
Signed-off-by: Ira <[email protected]>
k8s-ci-robot
added
size/L
|
In making changes in response to the review comments above, I tried to do a rebase. This resulted in 185 modified files. In this PR there are only two modified files. |
Added an initial end-to-end test with gRPC messages.
In particular, this test tests the streaming server's processing of the various Envoy gRPC messages. An in-memory buffered listener is used for the gRPC server, thus eliminating the need to find free ports in the test system. The code has been structured with several helper functions, that should make it easier in the future to add more tests of this nature in the future to the system.
This test uses a dummy scheduler to choose the target pod. The goal here was to test the streaming server, to setup infrastructure for more similar tests, and not necessarily test the scheduler.