-
Notifications
You must be signed in to change notification settings - Fork 182
test chat completions api in e2e case #868
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hi @delavet. 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. |
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delavet does it make sense to separate different tests into different ginkgo.It
statements?
I'd expect moving some parts to the BeforeEach
section, e.g., this part:
ginkgo.By("Creating an InferenceModel resource")
infModel := newInferenceModel(nsName)
gomega.Expect(cli.Create(ctx, infModel)).To(gomega.Succeed())
ginkgo.By("Ensuring the InferenceModel resource exists in the namespace")
gomega.Eventually(func() error {
return cli.Get(ctx, types.NamespacedName{Namespace: infModel.Namespace, Name: infModel.Name}, infModel) },
existsTimeout, interval).Should(gomega.Succeed())
I'd also expect creating some helper functions to extract the common code.
I think the e2e tests should include (each in a different ginkgo.It
statement):
- completions with prompt
- chat completions with a single [role, message].
- chat completions with multiple [role, message] entries.
This does make more sense! I will try refactoring this ASAP. |
test/e2e/epp/e2e_test.go
Outdated
|
||
ginkgo.By("Verifying connectivity through the inference extension") | ||
curlCmd := getCurlCommand(envoyName, nsName, envoyPort, modelName, curlTimeout) | ||
for _, testApi := range []string{"/completions", "/chat/completions"} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@delavet does it make sense to separate different tests into different ginkgo.It statements?
Another option is to move the ginkgo.By()
statement within the range
and include the name of the API endpoint so a user can see that each endpoint is being tested?
@nirrozenbaum @danehans I have refined this PR, and now the e2e tests include coverage for both |
/retest |
Signed-off-by: Hang Yin <[email protected]>
Changes verified: $ make test-e2e
...
STEP: Creating an InferenceModel resource @ 06/03/25 09:04:06.069
STEP: Ensuring the InferenceModel resource exists in the namespace @ 06/03/25 09:04:06.074
STEP: Verifying connectivity through the inference extension with /completions api and prompt/messages: Write as if you were a critic: San Francisco @ 06/03/25 09:04:06.082
STEP: Verifying connectivity through the inference extension with /chat/completions api and prompt/messages: [{"role": "user", "content": "Write as if you were a critic: San Francisco"}] @ 06/03/25 09:04:06.275
STEP: Verifying connectivity through the inference extension with /chat/completions api and prompt/messages: [{"role": "user", "content": "Write as if you were a critic: San Francisco"},{"role": "assistant", "content": "Okay, let's see..."},{"role": "user", "content": "Now summarize your thoughts."}] @ 06/03/25 09:04:06.369
STEP: Deleting the InferenceModel test resource. @ 06/03/25 09:04:06.454
• [0.394 seconds]
------------------------------
[AfterSuite]
/Users/solo-system-dhansen/go/src/sigs.k8s.io/gateway-api-inference-extension/test/e2e/epp/e2e_suite_test.go:140
STEP: Performing global cleanup @ 06/03/25 09:04:06.46
[AfterSuite] PASSED [0.203 seconds]
------------------------------
Ran 1 of 1 Specs in 79.008 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestAPIs (79.01s)
PASS
ok sigs.k8s.io/gateway-api-inference-extension/test/e2e/epp 79.036s /lgtm Thanks @delavet for your help! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, delavet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Hang Yin <[email protected]>
Adds
/chat/completions
API e2e test case.Fixes: #814