-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add optional headers support to WebSocket connection #83
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,78 @@ | ||
| import os | ||
|
|
||
| import pytest | ||
| from _utils import get_client | ||
|
|
||
| # Constants | ||
| API_KEY = os.getenv("SPEECHMATICS_API_KEY") | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_with_headers(): | ||
| """Tests that a client can be created. | ||
|
|
||
| - Checks for a valid session | ||
| - Checks that 'English' is the language pack info | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This docstring isn't correct. There's no check for the language pack. |
||
| """ | ||
|
|
||
| # API key | ||
| if not API_KEY: | ||
| pytest.skip("Valid API key required for test") | ||
|
|
||
| # Create client | ||
| client = await get_client( | ||
| api_key=API_KEY, | ||
| connect=False, | ||
| ) | ||
|
|
||
| # Headers | ||
| ws_headers = {"Z-TEST-HEADER-1": "ValueOne", "Z-TEST-HEADER-2": "ValueTwo"} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the two |
||
|
|
||
| # Check we are connected OK | ||
| await client.connect(ws_headers=ws_headers) | ||
|
|
||
| # Check we are connected | ||
| assert client._is_connected | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, you access a private attribute - a public property or function would be better. |
||
|
|
||
| # Disconnect | ||
| await client.disconnect() | ||
|
|
||
| # Check we are disconnected | ||
| assert not client._is_connected | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_no_partials(): | ||
| """Tests for STT config (no partials).""" | ||
| async def test_with_corrupted_headers(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test name should convey the expected outcome. Can you improve the naming? Same for the test above.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| """Tests that a client can be created. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This docstring isn't correct - it's a test for handling corrupted/invalid headers. |
||
|
|
||
| - Checks for a valid session | ||
| - Checks that 'English' is the language pack info | ||
| """ | ||
|
|
||
| # API key | ||
| if not API_KEY: | ||
| pytest.skip("Valid API key required for test") | ||
|
|
||
| # Create client | ||
| client = await get_client( | ||
| api_key=API_KEY, | ||
| connect=False, | ||
| ) | ||
|
|
||
| # Headers | ||
| ws_headers = ["ItemOne", "ItemTwo"] | ||
|
|
||
| # Check we are connected OK | ||
| try: | ||
| await client.connect(ws_headers=ws_headers) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this, could add a timeout to avoid hanging indefinitely, waiting to connect. |
||
| except AttributeError: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| pass | ||
|
|
||
| # Check we are connected | ||
| assert not client._is_connected | ||
|
|
||
| # Disconnect (in case connected) | ||
| await client.disconnect() | ||
|
|
||
| pass | ||
| # Check we are disconnected | ||
| assert not client._is_connected | ||
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.
Can you add a topline description for the test file?