-
Notifications
You must be signed in to change notification settings - Fork 27
refactor: get rid of unittest.TestCase
#647
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?
Conversation
WalkthroughMigrates tests from unittest-style async lifecycle to pytest-asyncio: adds pytest-asyncio plugin config, replaces many Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 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 Disabled knowledge base sources:
📒 Files selected for processing (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). (7)
🔇 Additional comments (1)
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. Comment |
b73ad72 to
77430b9
Compare
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/ably/rest/reststats_test.py (2)
26-74: Add teardown to the setup fixture.The setup fixture is missing a yield and teardown section. The asyncTearDown method at lines 76-78 will not be called by pytest.
Apply this diff to add proper teardown:
await self.ably.http.post('/stats', body=stats + previous_stats) TestRestAppStatsSetup.__stats_added = True + yield + await self.ably.close() + await self.ably_text.close()
76-78: Remove obsolete asyncTearDown method.This asyncTearDown method will not be called by pytest. The teardown should be moved into the setup fixture after a yield statement (see comment on lines 26-74).
Apply this diff to remove the obsolete method:
- async def asyncTearDown(self): - await self.ably.close() - await self.ably_text.close() -
🧹 Nitpick comments (5)
test/ably/rest/restinit_test.py (1)
15-17: Consider addingyieldfor consistency with other fixtures.This fixture lacks a
yieldstatement, unlike othersetupfixtures in this PR (e.g.,restcapability_test.py,restchannelhistory_test.py). While technically correct since there's no teardown needed, addingyieldat the end maintains a consistent pattern and makes it clearer this is a setup-only fixture.@pytest.fixture(autouse=True) async def setup(self): self.test_vars = await TestApp.get_test_vars() + yieldtest/ably/rest/restpush_test.py (1)
24-44: Consider usingscope="class"for this heavy fixture.This fixture registers 10 devices and multiple subscriptions before each test method, which could significantly slow down the test suite. Since the tests appear to read from these shared resources without modifying them destructively, a class-scoped fixture could improve performance:
- @pytest.fixture(autouse=True) + @pytest.fixture(autouse=True, scope="class") async def setup(self):However, verify that tests don't have interdependencies or modify shared state in ways that would cause flakiness with shared setup.
test/ably/rest/restcrypto_test.py (1)
204-217: Consider adding scope='class' to preserve original behavior.The original
setUpClasswas a classmethod that ran once per test class. Converting it to a function-scoped fixture changes it to run once per test method, which means the JSON file will be read and parsed repeatedly. Since the tests don't modify this state, consider addingscope='class'to preserve the original performance characteristics:- @pytest.fixture(autouse=True) + @pytest.fixture(autouse=True, scope='class') def setUpClass(self):Note: This would require the test classes to be compatible with class-scoped fixtures (typically works fine for read-only fixture data).
test/ably/realtime/realtimechannel_vcdiff_test.py (1)
36-48: Consider adding yield for consistency.While this fixture doesn't require teardown logic, adding a
yieldstatement would maintain consistency with the fixture pattern used throughout the test suite and make the fixture structure more explicit:async def setup(self): self.test_vars = await TestApp.get_test_vars() self.valid_key_format = "api:key" # Test data equivalent to JavaScript version self.test_data = [ {'foo': 'bar', 'count': 1, 'status': 'active'}, {'foo': 'bar', 'count': 2, 'status': 'active'}, {'foo': 'bar', 'count': 2, 'status': 'inactive'}, {'foo': 'bar', 'count': 3, 'status': 'inactive'}, {'foo': 'bar', 'count': 3, 'status': 'active'}, ] + yieldtest/ably/realtime/realtimechannel_test.py (1)
15-19: Channel tests use the standard async setup; consider de-duplicating laterThe async autouse
setupfixture here correctly initializestest_varsandvalid_key_formatfor each test, matching other realtime suites. Since this same body now appears in multiple classes, you might later factor it into a shared fixture orBaseAsyncTestCasehelper to avoid repetition, but that can be a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
conftest.py(1 hunks)pyproject.toml(2 hunks)test/ably/conftest.py(1 hunks)test/ably/realtime/eventemitter_test.py(1 hunks)test/ably/realtime/realtimechannel_publish_test.py(1 hunks)test/ably/realtime/realtimechannel_test.py(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(2 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)test/ably/realtime/realtimeinit_test.py(1 hunks)test/ably/realtime/realtimeresume_test.py(2 hunks)test/ably/rest/encoders_test.py(5 hunks)test/ably/rest/restauth_test.py(7 hunks)test/ably/rest/restcapability_test.py(1 hunks)test/ably/rest/restchannelhistory_test.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/rest/restchannels_test.py(1 hunks)test/ably/rest/restchannelstatus_test.py(2 hunks)test/ably/rest/restcrypto_test.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restpresence_test.py(2 hunks)test/ably/rest/restpush_test.py(2 hunks)test/ably/rest/restrequest_test.py(2 hunks)test/ably/rest/reststats_test.py(1 hunks)test/ably/rest/resttime_test.py(1 hunks)test/ably/rest/resttoken_test.py(2 hunks)test/ably/utils.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
test/ably/rest/encoders_test.py (3)
test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/resttime_test.py (1)
setup(17-20)
test/ably/rest/restrequest_test.py (5)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)
test/ably/conftest.py (1)
test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)clear_test_vars(105-115)
test/ably/rest/reststats_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)
test/ably/rest/restpush_test.py (3)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)
test/ably/realtime/realtimeresume_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/rest/restpresence_test.py (6)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)
test/ably/rest/restauth_test.py (5)
test/ably/rest/restchannelpublish_test.py (3)
setup(30-37)setup(454-459)get_ably_rest(533-535)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restrequest_test.py (1)
setup(16-27)test/ably/rest/resttime_test.py (1)
setup(17-20)
test/ably/realtime/realtimechannel_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/realtime/realtimeconnection_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/realtime/eventemitter_test.py (3)
test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/rest/restcapability_test.py (1)
setup(12-16)
test/ably/realtime/realtimeinit_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/rest/restchannelhistory_test.py (2)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannels_test.py (1)
setup(16-20)
test/ably/rest/restpaginatedresult_test.py (6)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restpresence_test.py (1)
setup(15-22)
test/ably/rest/restchannelpublish_test.py (4)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restinit_test.py (1)
setup(16-17)
test/ably/rest/restchannelstatus_test.py (3)
test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)
test/ably/rest/restchannels_test.py (2)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)
test/ably/rest/resttoken_test.py (5)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)
🪛 Gitleaks (8.30.0)
test/ably/rest/encoders_test.py
[high] 150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 354-354: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (6)
- GitHub Check: check (3.9)
- GitHub Check: check (3.10)
- GitHub Check: check (3.11)
- GitHub Check: check (3.12)
- GitHub Check: check (3.8)
- GitHub Check: check (3.13)
🔇 Additional comments (34)
conftest.py (1)
1-5: LGTM!The configuration correctly enables pytest-asyncio plugin globally for the test suite.
pyproject.toml (2)
45-46: LGTM!The pytest-asyncio dependency is correctly added with appropriate version constraints for different Python versions.
96-96: LGTM!Setting
asyncio_mode = "auto"enables automatic detection of async tests, which is appropriate for this migration.test/ably/rest/restcapability_test.py (1)
11-16: LGTM!The migration from asyncSetUp/asyncTearDown to pytest fixture is correctly implemented with proper setup, yield for teardown separation, and cleanup.
test/ably/rest/restauth_test.py (5)
29-31: LGTM!The fixture correctly initializes test_vars without requiring teardown.
171-176: LGTM!The fixture properly handles setup and teardown with yield, ensuring the ably client is closed after each test.
326-328: LGTM!The fixture correctly initializes test_vars without requiring teardown.
485-532: LGTM!The complex fixture correctly sets up mock API routes, starts interception, and properly cleans up resources after yield.
588-637: LGTM!The fixture properly manages mock setup and teardown, ensuring cleanup after each test.
test/ably/rest/restpresence_test.py (2)
14-22: LGTM!The fixture correctly handles setup and teardown with proper resource cleanup.
191-198: LGTM!The fixture properly manages channel lifecycle and client cleanup.
test/ably/utils.py (2)
17-17: LGTM!Removing unittest.TestCase inheritance is correct for the pytest migration, as test lifecycle is now managed by pytest fixtures.
36-36: LGTM!Removing IsolatedAsyncioTestCase inheritance is correct, as pytest-asyncio now handles async test execution.
test/ably/rest/encoders_test.py (4)
25-29: LGTM!The fixture correctly initializes the ably client with appropriate teardown.
147-152: LGTM!The fixture properly sets up encryption parameters for testing. The cipher key is a test fixture value, not a real secret.
260-264: LGTM!The fixture correctly initializes the binary protocol client with proper cleanup.
351-356: LGTM!The fixture properly configures encryption for binary encoding tests. The cipher key is a test fixture value.
test/ably/rest/restchannelstatus_test.py (1)
13-17: LGTM!The fixture correctly replaces the unittest-style
asyncSetUp/asyncTearDownpattern. Theyieldproperly separates setup from teardown, ensuringself.ably.close()runs after each test. This is consistent with other test files in the PR.test/ably/rest/restchannels_test.py (1)
15-20: LGTM!The fixture correctly implements the setup/teardown pattern, initializing
test_varsandablybefore yielding, then properly closing theablyclient after each test. This is consistent with the pattern used across other test files in this PR.test/ably/rest/resttime_test.py (1)
16-20: LGTM!The fixture correctly implements the async setup/teardown pattern. The
ablyclient is properly initialized before yielding and closed after each test completes.test/ably/rest/restchannelhistory_test.py (1)
16-21: LGTM: Clean migration to pytest fixture.The migration from
asyncSetUp/asyncTearDownto an autouse pytest fixture is correctly implemented. The fixture properly initializes resources, yields control to the test, and ensures cleanup viaawait self.ably.close().test/ably/conftest.py (1)
6-10: LGTM: Session-scoped fixture correctly implemented.The session-scoped fixture properly manages test application lifecycle, ensuring
TestApp.get_test_vars()is called once before all tests andTestApp.clear_test_vars()is called after all tests complete. Usingpytest_asyncio.fixturefor the session-scoped async fixture is appropriate.test/ably/rest/restrequest_test.py (1)
15-27: LGTM: Fixture correctly handles setup with test data population.The autouse fixture properly initializes the test client and pre-populates 20 test messages before yielding to the test. Cleanup is correctly handled after yield.
test/ably/rest/restpaginatedresult_test.py (1)
30-68: LGTM: Complex fixture setup handled correctly.The autouse fixture properly manages mocked API setup with respx, including starting/stopping the mock and initializing paginated result instances. The teardown sequence (stop, reset, close) is correct.
test/ably/rest/restchannelpublish_test.py (2)
29-37: LGTM: Multi-client fixture correctly implemented.The fixture properly initializes multiple Ably clients (
self.ablyandself.ably_with_client_id) and ensures both are closed in the teardown phase.
453-459: LGTM: Consistent multi-client fixture pattern.This fixture follows the same correct pattern as the first class, managing two client instances with proper cleanup.
test/ably/rest/resttoken_test.py (2)
22-28: LGTM: Standard fixture pattern correctly applied.The autouse fixture properly initializes the Ably client and test configuration, with cleanup handled after yield.
160-166: LGTM: Consistent fixture implementation.The fixture correctly follows the established pattern for test setup and teardown.
test/ably/rest/restcrypto_test.py (1)
21-28: LGTM: Standard fixture pattern correctly implemented.The autouse fixture properly manages two Ably client instances with appropriate cleanup.
test/ably/realtime/realtimechannel_publish_test.py (1)
17-20: Async autouse fixture correctly replaces per-test asyncSetUp
setupjust initializesself.test_varsviaTestApp.get_test_vars()before each test, matching the prior async setup semantics without altering test logic. Looks good.test/ably/realtime/eventemitter_test.py (1)
3-4: Fixture + import wiring for TestEventEmitter is consistent and correctAdding
import pytestand the async autousesetupfixture that populatesself.test_varsaligns this class with the rest of the realtime tests and preserves the previous setup behavior.Also applies to: 11-13
test/ably/realtime/realtimeconnection_test.py (1)
14-18: Per-test connection setup preserved via async autouse fixtureThe new
setupfixture re-establishesself.test_varsandself.valid_key_formatbefore each test, matching the old asyncSetUp behavior and supporting tests that depend on these attributes without changing their logic.test/ably/realtime/realtimeresume_test.py (1)
3-4: Resume tests now use the shared async setup patternThe added
pytestimport and async autousesetupfixture correctly mirror the common realtime setup (initializingtest_varsandvalid_key_format) without impacting the existing resume test logic.Also applies to: 27-30
test/ably/realtime/realtimeinit_test.py (1)
13-17: Init tests correctly migrated to async autouse fixtureThe new
setupfixture restorestest_varsandvalid_key_formatfor each test, preserving the original initialization semantics while aligning with the pytest fixture approach used elsewhere in the suite.
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/ably/rest/restcrypto_test.py (1)
204-218: Abstract crypto fixture behavior is correct; consider class scope for efficiencyThe new autouse fixture on
AbstractTestCryptoWithFixturecorrectly preparesself.fixture,self.params,self.cipher_params,self.cipher, andself.itemsfor each test. Since this initialization is pure and identical across tests in a class, you could make it a class-scoped fixture to better match the oldsetUpClassbehavior:- @pytest.fixture(autouse=True) - def setUpClass(self): + @pytest.fixture(scope='class', autouse=True) + def setUpClass(self):This is purely an efficiency/readability tweak; current behavior is functionally fine.
If you change the scope, re-run the crypto fixture tests to ensure pytest still discovers and applies the fixture to both
TestCryptoWithFixture128andTestCryptoWithFixture256.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
conftest.py(1 hunks)pyproject.toml(2 hunks)test/ably/conftest.py(1 hunks)test/ably/realtime/eventemitter_test.py(1 hunks)test/ably/realtime/realtimechannel_publish_test.py(1 hunks)test/ably/realtime/realtimechannel_test.py(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(2 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)test/ably/realtime/realtimeinit_test.py(1 hunks)test/ably/realtime/realtimeresume_test.py(2 hunks)test/ably/rest/encoders_test.py(5 hunks)test/ably/rest/restauth_test.py(7 hunks)test/ably/rest/restcapability_test.py(1 hunks)test/ably/rest/restchannelhistory_test.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/rest/restchannels_test.py(1 hunks)test/ably/rest/restchannelstatus_test.py(2 hunks)test/ably/rest/restcrypto_test.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restpresence_test.py(2 hunks)test/ably/rest/restpush_test.py(2 hunks)test/ably/rest/restrequest_test.py(2 hunks)test/ably/rest/reststats_test.py(2 hunks)test/ably/rest/resttime_test.py(1 hunks)test/ably/rest/resttoken_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- test/ably/realtime/realtimechannel_test.py
- test/ably/realtime/realtimeconnection_test.py
- test/ably/realtime/realtimechannel_vcdiff_test.py
- test/ably/rest/restchannels_test.py
- test/ably/rest/resttoken_test.py
- test/ably/realtime/realtimechannel_publish_test.py
- test/ably/rest/restchannelpublish_test.py
- test/ably/realtime/realtimeresume_test.py
- test/ably/rest/restpush_test.py
- test/ably/rest/restchannelstatus_test.py
- test/ably/rest/restpresence_test.py
- test/ably/rest/restcapability_test.py
🧰 Additional context used
🧬 Code graph analysis (9)
test/ably/conftest.py (1)
test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)clear_test_vars(105-115)
test/ably/rest/restchannelhistory_test.py (4)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/testapp.py (3)
TestApp(37-115)get_ably_rest(73-77)get_test_vars(41-70)
test/ably/rest/restcrypto_test.py (1)
ably/util/crypto.py (2)
CipherParams(16-42)get_cipher(163-168)
test/ably/rest/encoders_test.py (1)
ably/util/crypto.py (4)
CipherParams(16-42)secret_key(29-30)secret_key(110-111)algorithm(25-26)
test/ably/realtime/realtimeinit_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/rest/restauth_test.py (6)
test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)test/ably/rest/restrequest_test.py (1)
setup(16-27)test/ably/rest/resttime_test.py (1)
setup(17-20)test/ably/testapp.py (3)
TestApp(37-115)get_ably_rest(73-77)get_test_vars(41-70)
test/ably/rest/resttime_test.py (6)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)
test/ably/realtime/eventemitter_test.py (5)
test/ably/utils.py (1)
BaseAsyncTestCase(36-51)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/rest/restcapability_test.py (1)
setup(12-16)
test/ably/rest/restpaginatedresult_test.py (8)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpresence_test.py (1)
setup(15-22)
🪛 Gitleaks (8.30.0)
test/ably/rest/encoders_test.py
[high] 150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 354-354: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (6)
- GitHub Check: check (3.10)
- GitHub Check: check (3.12)
- GitHub Check: check (3.13)
- GitHub Check: check (3.11)
- GitHub Check: check (3.8)
- GitHub Check: check (3.9)
🔇 Additional comments (17)
pyproject.toml (1)
45-46: LGTM! pytest-asyncio configuration is correctly set up.The pytest-asyncio dependency is added with appropriate version constraints for Python 3.7 and 3.8+, and asyncio_mode = "auto" enables automatic detection of async tests, which aligns well with the fixture-based migration across the test suite.
Also applies to: 96-96
conftest.py (1)
1-5: LGTM! Proper pytest-asyncio plugin configuration.The module-level plugin loading ensures pytest-asyncio is available for all async fixtures and tests throughout the suite.
test/ably/realtime/eventemitter_test.py (1)
11-13: LGTM! Clean migration to pytest fixture.The autouse fixture properly replaces asyncSetUp. No teardown is needed for this test class, so the absence of yield is appropriate.
test/ably/rest/encoders_test.py (1)
25-29: LGTM! Fixtures correctly implement setup and teardown.All four test classes properly migrate to pytest fixtures with yield-based teardown to close the Ably client. The pattern is consistent and correct.
Note: The static analysis warnings about "Generic API Keys" on lines 150 and 354 are false positives—these are hardcoded test cipher keys (
keyfordecrypt_16), not real secrets.Also applies to: 147-152, 260-264, 351-356
test/ably/realtime/realtimeinit_test.py (1)
13-16: LGTM! Proper fixture-based setup.The migration from asyncSetUp to an autouse fixture is correct and consistent with other realtime test files in the PR.
test/ably/utils.py (1)
17-17: LGTM! Base classes properly decoupled from unittest.Removing unittest inheritance completes the migration to pytest-only test infrastructure. The classes now serve as pure mixins providing utility methods, which is the appropriate pattern for pytest-based tests.
Also applies to: 36-36
test/ably/rest/restinit_test.py (1)
15-17: LGTM! Straightforward migration to pytest fixture.The autouse fixture properly replaces asyncSetUp with minimal changes to the initialization logic.
test/ably/rest/restrequest_test.py (1)
15-27: LGTM! Fixture properly handles complex setup and teardown.The autouse fixture correctly migrates the test lifecycle, pre-populating test data in the setup phase before yield, and cleaning up the Ably client in the teardown phase after yield. This is the correct pattern for fixtures that need both initialization and cleanup.
test/ably/rest/restpaginatedresult_test.py (1)
1-1: Pytest-asyncio fixture migration for paginated result tests looks correctThe new async autouse fixture sets up the Ably client and
respxroutes once per test and reliably tears everything down viayield, matching the previous setup/teardown semantics while integrating cleanly with pytest.Please run this test module under the new pytest-asyncio configuration to confirm the async fixture is discovered correctly and the respx mocks behave as expected.
Also applies to: 30-69
test/ably/rest/resttime_test.py (1)
16-20: Async autouse fixture for time tests is consistent and safeThe new async autouse fixture correctly owns a single
AblyRestinstance per test and closes it afteryield, while tests that create extra clients still close them explicitly. This keeps resource lifetimes clear and matches the previous lifecycle.Please run this module with the updated pytest-asyncio config to confirm the async fixture is executed for both async and sync tests as intended.
test/ably/conftest.py (1)
1-1: Session-scoped async fixture for TestApp lifecycle looks appropriateThe
test_app_setupsession fixture cleanly initialisesTestAppstate once viaget_test_vars()and tears it down withclear_test_vars()after all tests. That matches the intended global test-app lifecycle and fits the pytest-asyncio model.Please confirm that
pytest-asynciois enabled in your pytest configuration so this session-scoped async fixture runs correctly and that no other code callsTestApp.clear_test_vars()independently during the session.Also applies to: 6-10
test/ably/rest/restauth_test.py (5)
29-32: TestAuth async fixture correctly centralizes test_vars setupThe new async autouse fixture that awaits
TestApp.get_test_vars()before each test keepsself.test_varsinitialisation consistent without introducing additional teardown requirements.Please verify
pytest-asynciois correctly handling this async fixture (no warnings about unawaited coroutines) when you run just theTestAuthtests.
171-176: AuthAuthorize fixture correctly manages a shared Ably client per testThe async autouse fixture now owns creation and teardown of
self.ably, while tests that need extra clients still manage those explicitly. Reassignments ofself.ablyin specific tests are handled by the fixture’s finalawait self.ably.close(), preserving previous teardown semantics.Please run the
TestAuthAuthorizesuite under pytest to ensure there are no lingering open connections or double-close issues aroundself.ably.
326-329: RequestToken fixture mirrors other test_vars-only setupsUsing an async autouse fixture here to populate
self.test_varsmatches the pattern inTestAuth; since Ably clients are created and closed inside individual tests, no teardown logic is needed in the fixture.Confirm that
TestRequestTokentests still pass and that no test implicitly depended on a class-levelasyncSetUpbeyondtest_varsinitialisation.
485-533: RenewToken fixture accurately wraps Ably client and respx router lifecycleThe new async autouse fixture sets up
self.ably, the mocked API, and routes, then tears them down viayield, including stopping/resetting the router and closing whateverself.ablycurrently refers to. This preserves the previous behavior where tests can rebindself.ablyand rely on teardown to close the final instance.Please re-run the
TestRenewTokentests and watch for any unexpected extra calls to the mocked routes, which would indicate fixture scope or teardown issues.
588-590: RenewExpiredToken fixture correctly scopes respx mocks per testThe async fixture constructs the mock router and routes, starts it, then stops and resets it after
yield. Since Ably clients are created and closed inside the tests themselves, this separation of responsibilities keeps the mocking layer and client lifecycle clean.Run just the
TestRenewExpiredTokentests to confirm the router isn’t leaking state between tests (e.g.,publish_attemptsandtime_routeresponses behave as expected on each run).Also applies to: 635-637
test/ably/rest/restcrypto_test.py (1)
21-28: RestCrypto async fixture correctly manages two Ably clients per testThe async autouse fixture now ensures both
self.ablyandself.ably2are created before each test and reliably closed afteryield, which is a faithful translation of the previous asyncSetUp/asyncTearDown pattern.Please run the crypto REST tests with pytest-asyncio enabled to confirm that no unclosed client warnings or connection leaks appear.
77430b9 to
1f9ac31
Compare
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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/ably/realtime/realtimeinit_test.py (1)
13-17: Async autouse fixture migration looks correct; verify pytest-asyncio integration andBaseAsyncTestCaseinheritanceThe async autouse
setupfixture reproduces the old async setup behavior and keeps the tests readable. Two small follow‑ups:
- Since this is an async fixture, consider using
pytest_asyncio.fixture(if that’s what you use elsewhere) to make the asyncio handling explicit and avoid relying on implicit plugin behavior. For example:-import pytest +import pytest +import pytest_asyncio @@ -class TestRealtimeInit(BaseAsyncTestCase): - @pytest.fixture(autouse=True) +class TestRealtimeInit(BaseAsyncTestCase): + @pytest_asyncio.fixture(autouse=True) async def setup(self): self.test_vars = await TestApp.get_test_vars() self.valid_key_format = "api:key"
- Please double‑check that
BaseAsyncTestCaseno longer subclassesunittest.TestCase(orIsolatedAsyncioTestCase). If it still does, pytest may treat this as a unittest‑style test case and ignore the fixture, which would break the new setup strategy.test/ably/realtime/realtimechannel_vcdiff_test.py (1)
4-4: Async autouse fixture migration looks good; please confirm pytest‑asyncio modeUsing an async autouse fixture on the test class is a clean replacement for the old async setup and keeps all per‑test initialization local. Given that the fixture is declared with
@pytest.fixture(not@pytest_asyncio.fixture), please double‑check that pytest‑asyncio is configured to treat async fixtures defined this way as asyncio‑driven (e.g.,asyncio_mode = auto), otherwise the coroutine may not be awaited in strict mode andself.test_vars/self.test_datawon’t be initialized as expected. If you intend to stay in strict mode, consider switching this (and the similar patterns in the other test modules) to@pytest_asyncio.fixturefor clarity and robustness.Also applies to: 36-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/check.yml(1 hunks)conftest.py(1 hunks)pyproject.toml(2 hunks)test/ably/conftest.py(1 hunks)test/ably/realtime/eventemitter_test.py(1 hunks)test/ably/realtime/realtimechannel_publish_test.py(1 hunks)test/ably/realtime/realtimechannel_test.py(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(2 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)test/ably/realtime/realtimeinit_test.py(1 hunks)test/ably/realtime/realtimeresume_test.py(2 hunks)test/ably/rest/encoders_test.py(5 hunks)test/ably/rest/restauth_test.py(7 hunks)test/ably/rest/restcapability_test.py(1 hunks)test/ably/rest/restchannelhistory_test.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/rest/restchannels_test.py(1 hunks)test/ably/rest/restchannelstatus_test.py(2 hunks)test/ably/rest/restcrypto_test.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restpresence_test.py(2 hunks)test/ably/rest/restpush_test.py(2 hunks)test/ably/rest/restrequest_test.py(2 hunks)test/ably/rest/reststats_test.py(2 hunks)test/ably/rest/resttime_test.py(1 hunks)test/ably/rest/resttoken_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- test/ably/realtime/realtimechannel_test.py
- conftest.py
- test/ably/rest/restrequest_test.py
- test/ably/realtime/realtimechannel_publish_test.py
- test/ably/rest/restchannelpublish_test.py
- pyproject.toml
- test/ably/rest/restcapability_test.py
- test/ably/rest/resttoken_test.py
- test/ably/realtime/realtimeresume_test.py
- test/ably/rest/restchannelstatus_test.py
- test/ably/rest/restcrypto_test.py
- test/ably/realtime/eventemitter_test.py
- test/ably/conftest.py
🧰 Additional context used
🧬 Code graph analysis (10)
test/ably/rest/restchannelhistory_test.py (5)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)
test/ably/rest/restinit_test.py (7)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)test/ably/rest/restrequest_test.py (1)
setup(16-27)
test/ably/rest/restpaginatedresult_test.py (7)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpresence_test.py (1)
setup(15-22)
test/ably/realtime/realtimechannel_vcdiff_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)
test/ably/rest/resttime_test.py (8)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)
test/ably/rest/restpresence_test.py (5)
test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)
test/ably/rest/restchannels_test.py (7)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)test/ably/rest/restpresence_test.py (1)
setup(15-22)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_rest(73-77)
test/ably/realtime/realtimeinit_test.py (9)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restcapability_test.py (1)
setup(12-16)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (1)
setup(30-37)
test/ably/realtime/realtimeconnection_test.py (4)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)
test/ably/rest/encoders_test.py (8)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)test/ably/rest/restrequest_test.py (1)
setup(16-27)test/ably/rest/resttime_test.py (1)
setup(17-20)test/ably/testapp.py (2)
TestApp(37-115)get_ably_rest(73-77)ably/util/crypto.py (4)
CipherParams(16-42)secret_key(29-30)secret_key(110-111)algorithm(25-26)
⏰ 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). (7)
- GitHub Check: check (3.10)
- GitHub Check: check (3.8)
- GitHub Check: check (3.11)
- GitHub Check: check (3.13)
- GitHub Check: check (3.9)
- GitHub Check: check (3.12)
- GitHub Check: check (3.14)
🔇 Additional comments (22)
test/ably/realtime/realtimeconnection_test.py (1)
14-15: LGTM! Clean migration from unittest to pytest fixtures.The migration from
asyncSetUpto pytest's@pytest.fixture(autouse=True)is correct and maintains the same per-test setup behavior. The pattern is consistent with other test files in this PR (eventemitter_test.py, realtimechannel_test.py, realtimeinit_test.py, realtimeresume_test.py).test/ably/rest/restpresence_test.py (1)
191-198: LGTM!The fixture correctly follows the pytest pattern with proper resource cleanup in the teardown phase (after
yield). The teardown order is also correct: releasing the channel before closing the client.test/ably/rest/encoders_test.py (5)
8-8: LGTM! Pytest import added for fixture decorators.The pytest import is correctly added to support the
@pytest.fixturedecorators used throughout this file.
25-29: LGTM! Correctly migrated to pytest fixture.The migration from
asyncSetUp/asyncTearDownto@pytest.fixture(autouse=True)is correct:
autouse=Trueensures the fixture runs before each test method- Yield-based teardown properly closes the Ably client
- Pattern is consistent with other migrated test files in the codebase
147-152: LGTM! Fixture correctly initializes encryption test dependencies.The fixture properly initializes both the Ably client and cipher parameters needed for encryption tests. The migration pattern is consistent with the rest of the file.
260-264: LGTM! Binary protocol setup correctly migrated.The fixture correctly uses the default binary protocol (no
use_binary_protocol=Falseoverride), which is appropriate for this test class. The migration pattern is correct.
351-356: LGTM! Encryption with binary protocol correctly configured.The fixture properly combines binary protocol encoding with encryption setup. The migration maintains the same initialization logic while adopting the pytest fixture pattern.
test/ably/utils.py (2)
17-17: LGTM! Base class migration aligns with pytest fixtures.The removal of unittest.TestCase inheritance is correct and necessary for the pytest-based fixture approach adopted throughout the test suite.
36-36: LGTM! Async base class migration aligns with pytest-asyncio.The removal of IsolatedAsyncioTestCase inheritance is correct and aligns with the pytest-asyncio fixture pattern used across the test suite.
test/ably/rest/restpush_test.py (1)
24-44: LGTM! Proper pytest-asyncio fixture migration.The async fixture correctly:
- Initializes test state (Ably client, devices, subscriptions) before yield
- Performs teardown (removes subscriptions, devices, closes client) after yield
- Uses autouse=True for automatic application to all test methods
This is a clean migration from unittest-style asyncSetUp/asyncTearDown to pytest fixtures.
test/ably/rest/restchannels_test.py (1)
15-20: LGTM! Clean async fixture migration.The setup fixture correctly initializes test_vars and ably client, then performs proper cleanup via yield-based teardown. This matches the pytest-asyncio pattern used consistently across the test suite.
test/ably/rest/restpaginatedresult_test.py (1)
30-68: LGTM! Comprehensive async fixture with proper mock cleanup.The setup fixture correctly:
- Initializes the Ably client with mocked HTTP routes
- Starts the mock interception
- Performs initial paginated queries
- Executes teardown (stops mocks, resets, closes client) after yield
This is a thorough migration from unittest-style lifecycle to pytest fixtures.
test/ably/rest/restinit_test.py (1)
15-17: LGTM! Minimal fixture appropriate for initialization tests.The setup fixture only initializes test_vars without teardown, which is appropriate since most tests in this file create and clean up their own short-lived Ably instances (e.g., lines 166-180, 206-216).
test/ably/rest/resttime_test.py (1)
16-20: LGTM! Proper async fixture with cleanup.The setup fixture correctly initializes the Ably client and ensures proper cleanup via yield-based teardown. This aligns with the pytest-asyncio pattern used throughout the test suite.
test/ably/rest/restchannelhistory_test.py (1)
16-21: LGTM! Clean async fixture migration.The setup fixture correctly initializes the Ably client and test_vars, then performs proper cleanup via yield-based teardown. This matches the pytest-asyncio pattern used consistently across the test suite.
.github/workflows/check.yml (1)
21-21: Remove Python 3.14 from the test matrix or ensure httpcore >= 1.0.8 is installed.Python 3.14 was officially released on October 7, 2025, and GitHub Actions fully supports it. However, httpx requires httpcore >= 1.0.8 to avoid an AttributeError on Python 3.14, and some edge-case compatibility issues remain. pytest-asyncio supports Python 3.14 as of v1.3.0. Either pin httpcore to a compatible version in your dependencies or remove Python 3.14 from the test matrix until downstream issues are fully resolved.
test/ably/rest/reststats_test.py (1)
26-27: Async autouse fixture now correctly handles both one‑time seeding and per‑test teardownThe new
setupfixture creates both REST clients, seeds stats only once viaif not TestRestAppStatsSetup.__stats_added, and always reaches theyield, soself.ably.close()/self.ably_text.close()run for every test instance. This fixes the earlier leak caused by returning before teardown while preserving the “POST only once” behavior.If you plan to run these tests under xdist or other parallelization, it may be worth double‑checking that the shared
__stats_addedflag and/statsseeding remain acceptable under concurrent execution.Also applies to: 71-76
test/ably/rest/restauth_test.py (5)
29-31: TestAuth async fixture cleanly replaces asyncSetUpThe autouse async
setupfixture that just populatesself.test_varsis a straightforward drop‑in for the old asyncSetUp with no teardown needs; usage by the synchronous and async tests below remains unchanged.
171-176: TestAuthAuthorize fixture correctly manages self.ably lifecycleThe async autouse
setupfixture now createsself.ablyandself.test_varsand tears down viayieldby always callingawait self.ably.close()post‑test. This keeps behavior equivalent to the former asyncSetUp/asyncTearDown while ensuring cleanup still happens even if a test fails, and remains compatible withVaryByProtocolTestsMetaclass/per_protocol_setup.
326-329: TestRequestToken async fixture mirrors prior asyncSetUp semanticsHere the autouse async
setupfixture only loadsself.test_vars, matching the previous per‑test initialization without introducing new teardown responsibilities; the token‑request tests below instantiate and close their own Ably clients as before.
485-487: TestRenewToken fixture correctly owns Ably + respx router lifecycleThe new async autouse
setupfixture now encapsulates creation ofself.ably, the respx mock router, and related routes, then usesyieldto guaranteeself.mocked_api.stop(quiet=True),reset(), andawait self.ably.close()are called after each test. This preserves the previous behavior while making cleanup robust to test failures, including cases where individual tests reassignself.ably.Also applies to: 528-533
588-591: TestRenewExpiredToken fixture provides robust respx teardownThe async autouse
setupfixture initializes the respx router and routes, starts it once per test, and the post‑yieldteardown (stop(quiet=True)+reset()) ensures no cross‑test leakage of HTTP mocks. Ably clients remain local to tests and are explicitly closed there, so responsibilities are cleanly separated.Also applies to: 633-637
replace `setUp`/`tearDown` methods with pytest fixtures Replaced `asyncSetUp`/`asyncTearDown` in test cases with `@pytest.fixture` for improved pytest integration. Added `pytest-asyncio` as a dependency. Updated version bump to `2.1.3`.
1f9ac31 to
c9be051
Compare
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/ably/rest/restpresence_test.py (1)
14-22: Move channel release to teardown phase.The channel is released on line 21 during setup (before
yield), but resource cleanup should occur in the teardown phase (afteryield). This is inconsistent withTestPresenceCrypt(line 197) which correctly releases the channel afteryield.
🧹 Nitpick comments (1)
conftest.py (1)
1-2: Optional: Remove leading blank lines.The two blank lines at the top of the file are unnecessary.
Apply this diff to clean up:
- - # Configure pytest-asyncio pytest_plugins = ('pytest_asyncio',)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/check.yml(1 hunks)conftest.py(1 hunks)pyproject.toml(2 hunks)test/ably/conftest.py(1 hunks)test/ably/realtime/eventemitter_test.py(1 hunks)test/ably/realtime/realtimechannel_publish_test.py(1 hunks)test/ably/realtime/realtimechannel_test.py(1 hunks)test/ably/realtime/realtimechannel_vcdiff_test.py(2 hunks)test/ably/realtime/realtimeconnection_test.py(1 hunks)test/ably/realtime/realtimeinit_test.py(1 hunks)test/ably/realtime/realtimeresume_test.py(2 hunks)test/ably/rest/encoders_test.py(5 hunks)test/ably/rest/restauth_test.py(7 hunks)test/ably/rest/restcapability_test.py(1 hunks)test/ably/rest/restchannelhistory_test.py(1 hunks)test/ably/rest/restchannelpublish_test.py(2 hunks)test/ably/rest/restchannels_test.py(1 hunks)test/ably/rest/restchannelstatus_test.py(2 hunks)test/ably/rest/restcrypto_test.py(2 hunks)test/ably/rest/restinit_test.py(1 hunks)test/ably/rest/restpaginatedresult_test.py(3 hunks)test/ably/rest/restpresence_test.py(2 hunks)test/ably/rest/restpush_test.py(2 hunks)test/ably/rest/restrequest_test.py(2 hunks)test/ably/rest/reststats_test.py(2 hunks)test/ably/rest/resttime_test.py(1 hunks)test/ably/rest/resttoken_test.py(2 hunks)test/ably/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- test/ably/rest/restinit_test.py
- test/ably/realtime/realtimechannel_vcdiff_test.py
- test/ably/rest/restchannelpublish_test.py
- test/ably/rest/restchannels_test.py
- test/ably/rest/restchannelhistory_test.py
- pyproject.toml
- test/ably/rest/resttoken_test.py
- test/ably/realtime/realtimeinit_test.py
- test/ably/rest/restchannelstatus_test.py
- test/ably/rest/resttime_test.py
- .github/workflows/check.yml
🧰 Additional context used
🧬 Code graph analysis (12)
test/ably/realtime/realtimechannel_test.py (4)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)
test/ably/realtime/eventemitter_test.py (3)
test/ably/utils.py (1)
BaseAsyncTestCase(36-51)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/rest/restcapability_test.py (1)
setup(12-16)
test/ably/rest/restpresence_test.py (3)
test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)
test/ably/conftest.py (1)
test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)clear_test_vars(105-115)
test/ably/rest/encoders_test.py (1)
ably/util/crypto.py (4)
CipherParams(16-42)secret_key(29-30)secret_key(110-111)algorithm(25-26)
test/ably/rest/restpush_test.py (4)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)
test/ably/rest/reststats_test.py (3)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restcapability_test.py (1)
setup(12-16)ably/rest/rest.py (2)
http(108-109)stats(79-85)
test/ably/realtime/realtimeresume_test.py (4)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)
test/ably/rest/restrequest_test.py (7)
test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)setup(454-459)test/ably/rest/restchannels_test.py (1)
setup(16-20)test/ably/rest/restcrypto_test.py (1)
setup(22-28)test/ably/rest/restinit_test.py (1)
setup(16-17)test/ably/rest/restpaginatedresult_test.py (1)
setup(31-68)
test/ably/rest/restpaginatedresult_test.py (4)
test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannels_test.py (1)
setup(16-20)
test/ably/rest/restcapability_test.py (10)
test/ably/realtime/eventemitter_test.py (1)
setup(12-13)test/ably/realtime/realtimechannel_test.py (1)
setup(16-18)test/ably/realtime/realtimeconnection_test.py (1)
setup(15-17)test/ably/realtime/realtimeinit_test.py (1)
setup(14-16)test/ably/realtime/realtimeresume_test.py (1)
setup(28-30)test/ably/rest/encoders_test.py (4)
setup(26-29)setup(148-152)setup(261-264)setup(352-356)test/ably/rest/restauth_test.py (5)
setup(30-31)setup(172-176)setup(327-328)setup(486-532)setup(589-637)test/ably/rest/restchannelhistory_test.py (1)
setup(17-21)test/ably/rest/restchannelpublish_test.py (2)
setup(30-37)get_ably_rest(533-535)test/ably/testapp.py (3)
TestApp(37-115)get_test_vars(41-70)get_ably_rest(73-77)
test/ably/rest/restcrypto_test.py (1)
ably/util/crypto.py (2)
CipherParams(16-42)get_cipher(163-168)
🪛 Gitleaks (8.30.0)
test/ably/rest/encoders_test.py
[high] 150-150: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 354-354: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (7)
- GitHub Check: check (3.14)
- GitHub Check: check (3.11)
- GitHub Check: check (3.13)
- GitHub Check: check (3.12)
- GitHub Check: check (3.10)
- GitHub Check: check (3.9)
- GitHub Check: check (3.8)
🔇 Additional comments (29)
test/ably/rest/encoders_test.py (5)
8-8: LGTM! Import added for pytest fixtures.The pytest import is correctly added to support the
@pytest.fixturedecorators used throughout the file.
25-29: LGTM! Correct pytest-asyncio fixture pattern.The migration from
asyncSetUp/asyncTearDownto a pytest fixture is correctly implemented. Theautouse=Trueensures this runs for each test method, and the yield-based teardown properly cleans up resources.
147-152: LGTM! Correct pytest-asyncio fixture pattern.The migration is correctly implemented with proper setup and teardown.
Regarding the static analysis hint about line 150: The hardcoded key
'keyfordecrypt_16'is a test fixture used consistently across encryption tests, not a real secret. This is a false positive.
260-264: LGTM! Consistent fixture implementation.The fixture correctly initializes the Ably client with binary protocol and ensures proper cleanup.
351-356: LGTM! Consistent fixture implementation.The fixture correctly implements the pytest-asyncio pattern with proper setup and teardown for binary protocol encryption tests.
Regarding the static analysis hint about line 354: The hardcoded key
'keyfordecrypt_16'is a test fixture, not a real secret. This is a false positive.conftest.py (1)
1-4: LGTM! pytest-asyncio plugin configured correctly.The
pytest_pluginsconfiguration properly enables pytest-asyncio for the test suite.test/ably/realtime/eventemitter_test.py (2)
3-3: LGTM! pytest import added correctly.
11-13: LGTM! Fixture correctly replaces asyncSetUp.The autouse fixture properly replaces the
asyncSetUplifecycle method and will run automatically before each test. The initialization logic is preserved correctly.test/ably/realtime/realtimeresume_test.py (2)
3-3: LGTM! pytest import added.
27-30: LGTM! Fixture correctly replaces asyncSetUp.The migration from
asyncSetUpto an autouse pytest fixture is correct and consistent with the broader pattern across realtime tests.test/ably/realtime/realtimechannel_test.py (1)
15-18: LGTM! Fixture migration is correct.The autouse pytest fixture properly replaces the
asyncSetUpmethod and follows the established pattern across the test suite.test/ably/realtime/realtimechannel_publish_test.py (1)
17-19: LGTM! Fixture correctly replaces asyncSetUp.The autouse fixture properly migrates the setup logic from
asyncSetUpto pytest-asyncio. Individual tests handle their own resource cleanup, which is appropriate for this test class.test/ably/utils.py (2)
17-17: LGTM! unittest.TestCase inheritance removed correctly.Removing the unittest base class is appropriate for the migration to pytest. The class can now be used as a plain utility/mixin class with pytest test classes.
36-36: LGTM! IsolatedAsyncioTestCase inheritance removed correctly.The removal of unittest async test case inheritance aligns with the pytest-asyncio migration. The class methods remain functional as utilities for pytest-based async tests.
test/ably/realtime/realtimeconnection_test.py (1)
14-17: LGTM! Fixture migration is correct.The autouse pytest fixture correctly replaces the
asyncSetUplifecycle method, consistent with the broader test suite migration.test/ably/rest/restpush_test.py (1)
24-44: LGTM! Fixture correctly replaces asyncSetUp/asyncTearDown with yield pattern.The fixture properly migrates both setup and teardown logic using the pytest yield pattern. Setup occurs before
yield(creating devices and subscriptions), and teardown occurs after (cleaning up resources and closing the ably client). This is the correct approach when the fixture creates shared resources (likeself.ably) used across multiple tests.test/ably/rest/restpaginatedresult_test.py (1)
30-68: LGTM! Fixture migration follows best practices.The async fixture correctly initializes resources (ably client, mocked API, paginated results) before
yield, then performs cleanup (stops mocks, closes client) in the teardown phase afteryield. This aligns with the pytest fixture pattern used throughout the PR.test/ably/conftest.py (1)
6-10: LGTM! Proper migration to pytest-asyncio session fixture.The fixture correctly manages test application lifecycle at session scope, initializing test vars before
yieldand cleaning up after. This replaces the manual event loop management with pytest-asyncio's native async fixture support.test/ably/rest/restpresence_test.py (1)
191-198: LGTM! Proper fixture teardown pattern.The
TestPresenceCryptfixture correctly releases the channel afteryieldduring teardown, following proper pytest fixture cleanup patterns.test/ably/rest/restcrypto_test.py (2)
21-28: LGTM! Standard fixture migration.The setup fixture follows the standard pattern: initializes resources before
yield, performs cleanup afteryield.
204-217: The fixture scope is correctly set to function scope and should not be changed.The current implementation runs the fixture before each test method, which is intentional. The
test_encodemethod resetsself.cipher_paramsandself.cipherduring execution, indicating that per-test initialization is required. Usingscope='class'would break this pattern by reusing the same cipher instance across tests, causing state pollution.However, renaming
setUpClasstosetuporsetup_classfor consistency with pytest conventions would improve clarity, since the function-scope pytest fixture decorator contradicts thesetUpClassclass-level naming convention.Likely an incorrect or invalid review comment.
test/ably/rest/restcapability_test.py (1)
11-16: LGTM! Standard fixture migration.The fixture correctly initializes test resources before
yieldand closes the ably client during teardown afteryield, following the established pattern.test/ably/rest/reststats_test.py (1)
26-76: LGTM! Fixture properly ensures teardown always runs.The fixture correctly inverts the conditional logic so that
yieldis always reached, ensuring the teardown code (lines 75-76) that closes both ably clients always executes. This addresses the resource leak that would have occurred with an early return beforeyield.test/ably/rest/restrequest_test.py (1)
15-27: LGTM! Fixture migration includes test data setup.The fixture properly initializes test resources and populates test data (20 events) before
yield, then closes the ably client during teardown. The pattern is consistent with other test fixtures in the PR.test/ably/rest/restauth_test.py (5)
29-31: LGTM! Simple setup fixture without teardown.The fixture only initializes
test_varswithout acquiring resources that need cleanup, so noyieldor teardown is necessary.
171-176: LGTM! Standard fixture with proper teardown.The fixture correctly initializes ably client and test_vars before
yield, then closes the ably client during teardown.
326-328: LGTM! Simple setup fixture without teardown.The fixture only initializes
test_varswithout acquiring resources that need cleanup, so noyieldor teardown is necessary.
485-532: LGTM! Complex fixture with proper mock and client teardown.The fixture correctly:
- Initializes test state, ably client, and mocked API routes before
yield- Performs teardown after
yield: stops/resets mocks withquiet=Trueand closes ably client- Uses
quiet=Trueto avoid spurious failures when not all mocked routes are called
588-637: LGTM! Complex fixture with proper mock teardown.The fixture correctly initializes test state and mocked API routes before
yield, then stops and resets mocks withquiet=Trueduring teardown. The pattern is consistent with other complex fixtures in this file.
owenpearson
left a comment
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.
looks good, couple questions:
owenpearson
left a comment
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.
lgtm 👍
replace
setUp/tearDownmethods with pytest fixturesReplaced
asyncSetUp/asyncTearDownin test cases with@pytest.fixturefor improved pytest integration. Addedpytest-asyncioas a dependency.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.