feat: add optional headers support to WebSocket connection#83
feat: add optional headers support to WebSocket connection#83
Conversation
Introduce an optional `ws_headers` parameter to the `connect` method in `VoiceAgentClient`. This allows users to pass custom headers when establishing the WebSocket connection to the Speechmatics API.
|
Can you add a comment with the testing you've done? Thanks. |
Introduce two new tests to validate header handling in the STT client. `test_with_headers` checks successful connection using valid headers, while `test_with_corrupted_headers` ensures connection failure with invalid header format.
@LArmstrongDev - new test has been added to include valid and invalid header testing. |
| """Tests that a client can be created. | ||
|
|
||
| - Checks for a valid session | ||
| - Checks that 'English' is the language pack info |
There was a problem hiding this comment.
This docstring isn't correct. There's no check for the language pack.
| async def test_no_partials(): | ||
| """Tests for STT config (no partials).""" | ||
| async def test_with_corrupted_headers(): | ||
| """Tests that a client can be created. |
There was a problem hiding this comment.
This docstring isn't correct - it's a test for handling corrupted/invalid headers.
| # Check we are connected OK | ||
| try: | ||
| await client.connect(ws_headers=ws_headers) | ||
| except AttributeError: |
There was a problem hiding this comment.
Why do we silently pass here? This will mask any bugs that come up in the client connection code. Given that the headers are corrupted, I assume we expect a failure - but here, if connect() raises no exception somehow, the test would still pass (false positive).
| await client.connect(ws_headers=ws_headers) | ||
|
|
||
| # Check we are connected | ||
| assert client._is_connected |
There was a problem hiding this comment.
Here, you access a private attribute - a public property or function would be better.
| @pytest.mark.asyncio | ||
| async def test_no_partials(): | ||
| """Tests for STT config (no partials).""" | ||
| async def test_with_corrupted_headers(): |
There was a problem hiding this comment.
The test name should convey the expected outcome. Can you improve the naming? Same for the test above.
|
In the test, you have shared setup. This should be extracted into a fixture to avoid repetition and make it easier to extend - e.g. for the API key check, the client creation each time, and to add |
|
|
||
| # Check we are connected OK | ||
| try: | ||
| await client.connect(ws_headers=ws_headers) |
There was a problem hiding this comment.
With this, could add a timeout to avoid hanging indefinitely, waiting to connect.
| @pytest.mark.asyncio | ||
| async def test_no_partials(): | ||
| """Tests for STT config (no partials).""" | ||
| async def test_with_corrupted_headers(): |
There was a problem hiding this comment.
This only tests passing a list instead of a dict.
Could expand test coverage to test a few other cases - empty dict, headers with non-string values, very large values etc, with parametrizing a few test headers.
|
Several comments in the invalid header test have been copied from the valid headers test and not updated, making them inaccurate. Rather than fixing them, most should just be removed - the code is self-explanatory and the comments add noise, rather than clarity. Comments should only be present where they add context the code can't provide for ease of understanding and readability. |
| @@ -1,8 +1,78 @@ | |||
| import os | |||
There was a problem hiding this comment.
Can you add a topline description for the test file?
|
What's the actual behaviour in the malformed headers test case? When sending an invalid websocket header, what connection error message do we expect? This should be included in the test. |
| ) | ||
|
|
||
| # Headers | ||
| ws_headers = {"Z-TEST-HEADER-1": "ValueOne", "Z-TEST-HEADER-2": "ValueTwo"} |
There was a problem hiding this comment.
For the two ws_headers used, it would be clearer to pull them out as constants and have a VALID_WS_HEADERS, INVALID_WS_HEADERS, or something similar, rather than hard-coding them into the tests.
2 participants
Introduce an optional
ws_headersparameter to theconnectmethod inVoiceAgentClientand the underlyingAsyncClient. This allows users to pass custom headers when establishing the WebSocket connection to the Speechmatics API.Test
tests/voice/test_06_stt_config.pytests for valid and invalidws_headerspassed to the connect method.