Fix integration test of chat completion #690
Conversation
WalkthroughUpdates testing: integration chat streaming test removed environment-based API key handling and no longer asserts streamed content; new unit tests were added for stream/decoder behavior exercising many edge cases and error paths. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test_ChatCompletionStream
participant C as Client
participant S as Chat Service
T->>C: UpdateChatWorkspace(OrgId, ApiVersion, DeploymentId, Source)
C->>S: Configure workspace
S-->>C: Ack
T->>C: Start chat completion stream
C->>S: Request stream
loop Receive chunks
S-->>C: StreamChunk
C-->>T: Yield chunk (no accumulation/assertion)
end
sequenceDiagram
participant UT as Unit Test
participant D as Decoder
participant S as Stream wrapper
UT->>D: RegisterDecoder / NewDecoder(...)
UT->>D: Feed response bytes
D-->>UT: Next()/Event()/Err() outcomes (including EOF/errors)
UT->>S: NewStream(decoder)
S->>D: Iteration (Next/Close)
alt decoder error after iteration
D-->>S: error
S-->>UT: Propagate error via Err()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 83.80% 85.45% +1.65%
==========================================
Files 20 20
Lines 3514 3514
==========================================
+ Hits 2945 3003 +58
+ Misses 420 366 -54
+ Partials 149 145 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration/chat_test.go (1)
316-321: Make the streaming test assertive: ensure at least one chunk and check final error.As written, the test can pass even if the stream yields zero chunks (e.g., due to misconfiguration), since there is no post-loop assertion. Count chunks and assert the final
stream.Err()is nil.- for stream.Next() { - require.NoError(t, stream.Err()) - chunk := stream.Current() - require.NotNil(t, chunk) - } + gotChunks := 0 + for stream.Next() { + require.NoError(t, stream.Err()) + chunk := stream.Current() + require.NotNil(t, chunk) + gotChunks++ + } + require.NoError(t, stream.Err()) + require.Greater(t, gotChunks, 0, "expected at least one streamed chunk")This will prevent false positives where the stream is established but produces no data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
integration/chat_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
integration/chat_test.go (1)
enum.go (1)
OpenaiChatSource(129-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (go current version)
- GitHub Check: integration-tests (go latest version)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
stream_test.go (3)
225-239: Clean up the global decoder registry after the testRegisterDecoder mutates the package-level decoderTypes map. Add a t.Cleanup to delete the test key to avoid cross-test coupling if future tests use the same content type or if tests run in parallel elsewhere.
Apply this diff:
func TestRegisterDecoder_InvalidContentType(t *testing.T) { // Force the fallback branch in RegisterDecoder when mime.ParseMediaType fails key := "%%%INVALID%%%" called := false RegisterDecoder(key, func(rc io.ReadCloser) Decoder { called = true return newSSEDecoder(rc) }) + t.Cleanup(func() { + delete(decoderTypes, strings.ToLower(key)) + }) // The decoderTypes map should now contain the raw lower-cased key _, ok := decoderTypes[strings.ToLower(key)] assert.True(t, ok, "expected decoderTypes to contain fallback key") // Just sanity: constructing a response with this invalid content-type won't match // (NewDecoder parsing fails and defaults to SSE) so we only validate registration here. assert.False(t, called, "decoder factory should not be invoked in this test") }
250-262: Make fallback-to-SSE test resilient to future registrationsUsing "application/json" assumes no decoder is registered for it. If a JSON decoder is ever added, this test will start failing. Use a clearly unregistered content type to ensure stability.
Apply this diff:
- // Use an unregistered (but valid) content type so NewDecoder falls back to SSE. - res := httpRespWithBody(t, data, "application/json") + // Use an obviously unregistered (but valid) content type so NewDecoder falls back to SSE. + res := httpRespWithBody(t, data, "application/x-meilisearch-test")
295-315: Add compile-time interface assertion for fakeDecoderThis guards against future interface changes to Decoder by catching mismatches at compile time.
Apply this diff:
type fakeDecoder struct { events []Event idx int err error cur Event closed bool } +// Ensure fakeDecoder implements Decoder. +var _ Decoder = (*fakeDecoder)(nil) + func (f *fakeDecoder) Next() bool { if f.idx < len(f.events) { f.cur = f.events[f.idx] f.idx++ return true } return false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
stream_test.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
stream_test.go (1)
stream.go (5)
RegisterDecoder(17-24)Decoder(26-31)NewDecoder(38-53)Event(33-36)NewStream(197-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (go current version)
- GitHub Check: integration-tests (go latest version)
🔇 Additional comments (7)
stream_test.go (7)
4-4: Import of errors is appropriate for sentinel error testsUsed in TestStream_DecoderErrorPropagatesAfterIteration; no issues.
264-273: Double close and post-close Next() semantics verifiedSolid coverage of Close idempotency and Next() behavior after Close.
275-286: Correctly asserts behavior for 'event' line without colonMatches SSE parsing rules: field name with empty value yields empty event type and no data.
288-293: Nil decoder path is well coveredValidates NewStream’s defensive behavior and Close() no-op with nil decoder.
316-323: Error propagation from underlying decoder is correctly assertedGood use of sentinel error and ErrorIs for robustness.
325-337: Leading blank lines are correctly ignoredThis test nicely captures a subtle but common SSE edge case.
241-248: errorReader helper is already definedVerified in encoding_test.go (lines 27–31) that the
errorReadertype and itsReadmethod exist, soTestDecoder_ReadErrorPropagateswill compile successfully.
What does this PR do?
https://github.com/meilisearch/meilisearch-js/blob/main/tests/chat-workspaces.test.ts
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit