feat: add tests for testing all wss model backends #53
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved per-model try/except in an existing TTS integration test so failures now propagate; added synchronous and asynchronous WebSocket streaming tests that iterate all Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test (sync/async)
participant Client as FishAudio Client
participant WS as WebSocket Server
participant FS as File System
rect `#E8F7E8`
Note left of Test: iterate Model values
end
Test->>Client: open websocket connection (model)
Client->>WS: send TTS streaming request (model)
rect `#FFF4E6`
WS-->>Client: stream audio chunk (binary)*
loop collect chunks
Client->>Test: deliver chunk
Test->>FS: append chunk to per-model file
end
end
WS-->>Client: stream end
Client->>Test: final audio bytes
Test->>Test: assert audio length > 0
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for WebSocket streaming with different TTS model backends and removes conditional error handling for model availability in existing tests. The changes ensure that all model backends ("speech-1.5", "speech-1.6", "s1") are tested and expected to be available.
- Adds new test methods for both synchronous and asynchronous WebSocket streaming with all available models
- Removes try-except blocks that would skip unavailable models, making model availability a hard requirement
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/integration/test_tts_websocket_integration.py | Adds test_websocket_streaming_with_different_models and test_async_websocket_streaming_with_different_models to test all model backends via WebSocket |
| tests/integration/test_tts_integration.py | Removes exception handling that would skip unavailable models, making all models mandatory for tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/test_tts_websocket_integration.py (2)
37-51: Sync WebSocket per-model test is solid; SSL pipeline failure is likely environmental but may cause flakes.Iterating
models = get_args(Model), streaming viastream_websocket(..., model=model), asserting non-empty chunks, and saving per-model outputs is a good way to validate all backends. The reportedhttpx.ReadError/ SSL record-layer failure on this call path looks like an environment/network/TLS issue rather than a logic bug here, but if it recurs in CI you may want to add resilience (e.g., retries, conditional skip/xfail when the WebSocket endpoint is unreachable) or investigate the underlying TLS configuration of the test environment.
216-238: Async WebSocket per-model test mirrors the sync variant; consider bindingmodelin the generator for future-proofing.The async loop over
modelswithstream_websocket(text_stream(), model=model)and per-model assertions/files is correct and consistent with the sync test. To avoid any late-binding surprises if this code is refactored to run generators out of the loop, you could optionally bindmodelas a default argument inasync def text_stream(model=model): ..., but it’s not strictly necessary given the current control flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/integration/test_tts_integration.py(1 hunks)tests/integration/test_tts_websocket_integration.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/test_tts_integration.py (3)
tests/integration/conftest.py (2)
client(33-41)save_audio(53-78)src/fishaudio/client.py (2)
tts(73-77)tts(167-171)src/fishaudio/resources/tts.py (2)
convert(144-204)convert(449-510)
tests/integration/test_tts_websocket_integration.py (3)
tests/integration/conftest.py (3)
client(33-41)save_audio(53-78)async_client(45-49)src/fish_audio_sdk/websocket.py (2)
tts(39-86)tts(112-159)src/fishaudio/resources/tts.py (2)
stream_websocket(206-357)stream_websocket(512-653)
🪛 GitHub Actions: Python
tests/integration/test_tts_websocket_integration.py
[error] 46-46: WebSocket TTS streaming test failed due to SSL read error: httpx.ReadError: [SSL] record layer failure. Possible TLS/SSL handshake issue when connecting to WebSocket endpoint during model variation test.
🔇 Additional comments (1)
tests/integration/test_tts_integration.py (1)
52-57: Per-model sync TTS assertions look good; ensure allModelvariants are actually usable in CI.The loop over
models = get_args(Model)with directconvert(..., model=model)calls and length assertion is clear and gives strong signal if any backend breaks. The tradeoff is that the test will now hard-fail ifModelcontains variants not provisioned for the API key or environment, so it’s worth double-checking thatModelonly enumerates supported backends for this test context, or filtering/marking specific models if needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/integration/conftest.py (2)
47-48: LGTM - Consistent with existing fixture pattern.The asyncio import follows the same pattern as the sync client fixture (line 35 imports
time). Both are reasonable for fixture-specific functionality.Optionally, consider moving both
asyncioandtimeimports to module level (after line 6) for consistency with standard Python practices:from __future__ import annotations +import asyncio import os +import time from pathlib import PathThen remove the inline imports from both fixtures.
52-53: LGTM - Correctly implements rate-limit delay.The delay mirrors the sync client pattern and correctly uses
await asyncio.sleep()for async context. This aligns with the PR's goal of supporting WebSocket streaming tests across multiple models.Optionally, extract the delay duration to a module-level constant to keep both fixtures in sync:
# After line 13 RATE_LIMIT_DELAY = 0.3 # Delay to avoid WebSocket rate limitsThen use it in both fixtures:
time.sleep(RATE_LIMIT_DELAY) # sync client await asyncio.sleep(RATE_LIMIT_DELAY) # async client
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/integration/conftest.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/integration/conftest.py (2)
src/fishaudio/core/client_wrapper.py (4)
client(154-156)client(226-228)close(158-160)close(230-232)src/fishaudio/client.py (2)
close(100-102)close(194-196)
⏰ 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
- GitHub Check: Integration Tests
Summary by CodeRabbit