PYTHON-5784 Coverage increase for periodic_executor.py#2771
PYTHON-5784 Coverage increase for periodic_executor.py#2771aclark4life wants to merge 9 commits intomongodb:masterfrom
periodic_executor.py#2771Conversation
There was a problem hiding this comment.
Pull request overview
Adds a dedicated unit test module for pymongo.periodic_executor, targeting both the threading-based PeriodicExecutor and asyncio-based AsyncPeriodicExecutor, plus module-level executor registration/shutdown helpers, to improve overall coverage.
Changes:
- Adds new unit tests for
PeriodicExecutorlifecycle and control methods (open/close/join/wake/update_interval/skip_sleep, stop conditions). - Adds new unit tests for
AsyncPeriodicExecutorlifecycle and stop conditions. - Adds unit tests for module-level
_register_executor()and_shutdown_executors()behavior.
| async def _default_target(): | ||
| return True | ||
|
|
||
| if target is None: | ||
| target = _default_target |
There was a problem hiding this comment.
Does this need to differ from the way _make_sync above does it?
| def _run(coroutine): | ||
| return asyncio.run(coroutine) |
There was a problem hiding this comment.
The individual tests that use asynchronous methods should use AsyncUnitTest or another appropriate test class from test/asynchronous/__init__.py instead of this. Each asyncio.run() call adds a significant amount of runtime overhead to setup and teardown the event loop.
|
|
||
| class TestPeriodicExecutorRepr(unittest.TestCase): | ||
| def test_repr_contains_class_and_name(self): | ||
| ex = _make_sync(name="myexec") |
There was a problem hiding this comment.
We can move all the _make_sync() calls into an asyncSetUp/setUp method on a base class shared by all of the test classes here.
| finally: | ||
| ex.close() | ||
| ex.join(timeout=2) | ||
|
|
There was a problem hiding this comment.
Similarly, safe closing of each executor can be done in an asyncTearDown/tearDown method on a base class.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestAsyncPeriodicExecutorRepr(unittest.TestCase): |
There was a problem hiding this comment.
Can we move the AsyncPeriodicExecutor tests into test/asynchronous/ and generate the synchronous ones using synchro? That would half the amount of code we need to maintain.
There was a problem hiding this comment.
Yes. The only tests that should not go through synchro are tests that don't need modification between async and sync, as well as tests against common modules shared between them.
| await self.executor.join(timeout=2) | ||
|
|
||
|
|
||
| class TestAsyncPeriodicExecutorRepr(AsyncUnitTest): |
There was a problem hiding this comment.
This class should also inherit from AsyncPeriodicExecutorTestBase and use its setup/teardown methods.
| self.assertIn("exec", executor_repr) | ||
|
|
||
|
|
||
| class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase): |
There was a problem hiding this comment.
What's the intended purpose of having so many separate classes, each with a couple of tests? We generally only separate tests inside files into separate classes if the groupings need different infrastructure (unit vs integration) or specific setup/configuration common to them.
There was a problem hiding this comment.
This question still stands @aclark4life.
No good reason, especially in the context of removing non-behavior-checking tests. I'll push a single class (plus base class) unless we come across a reason to add more.
|
|
||
|
|
||
| class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase): | ||
| async def test_update_interval(self): |
There was a problem hiding this comment.
This test and the following don't test behavior but implementation.
|
|
||
|
|
||
| class TestAsyncPeriodicExecutorLifecycle(AsyncPeriodicExecutorTestBase): | ||
| async def test_open_starts_worker(self): |
There was a problem hiding this comment.
Same here, this is testing implementation, not behavior.
| else: | ||
| self.assertIsNotNone(self.executor._task) | ||
|
|
||
| async def test_close_sets_stopped(self): |
| self.executor = _make_executor(target=target) | ||
| self.executor.open() | ||
| if _IS_SYNC: | ||
| self.assertTrue(ran.wait(timeout=2), "target never ran") |
There was a problem hiding this comment.
Is this needed if we're calling join() with the same timeout immediately after? We could check that ran is set after the join, which would mean that the executor stopped running or else the join would timeout.
| else: | ||
| await asyncio.wait_for(ran.wait(), timeout=2) | ||
| await self.executor.join(timeout=2) | ||
| self.assertTrue(self.executor._stopped) |
There was a problem hiding this comment.
We can remove this if my above comment applies.
|
|
||
| self.executor = _make_executor(target=target) | ||
| self.executor.open() | ||
| self.assertTrue(ran.wait(timeout=2), "target never ran") |
|
|
||
| self.executor = _make_executor(target=target) | ||
| self.executor.open() | ||
| await asyncio.wait_for(ran.wait(), timeout=2) |
| call_times = [] | ||
|
|
||
| async def target(): | ||
| call_times.append(time.monotonic() if _IS_SYNC else asyncio.get_running_loop().time()) |
There was a problem hiding this comment.
Why use asyncio.get_running_loop().time() over time.monotonic()? Asyncio's default event loop returns time.monotonic() anyway: https://github.com/python/cpython/blob/b8ebd078f90007d48fcab85effadb33769cd080c/Lib/asyncio/base_events.py#L771.
| self.assertGreaterEqual(called[0], 2) | ||
|
|
||
|
|
||
| class TestShouldStop(AsyncUnitTest): |
There was a problem hiding this comment.
Both of these tests are again testing implementation, not behavior. Do they add confidence in the behavior of PeriodicExecutor when used?
| self.assertTrue(executor._thread_will_exit) | ||
|
|
||
|
|
||
| class TestRegisterExecutor(AsyncUnitTest): |
There was a problem hiding this comment.
Does this test meaningfully add coverage that our existingtest_cleanup_executors_on_client_del in test/test_monitor.py does not?
| ) | ||
|
|
||
|
|
||
| class AsyncPeriodicExecutorTestBase(AsyncUnitTest): |
There was a problem hiding this comment.
Every test except for test_join_without_open_is_safe now explicitly calls _make_executor() and .join() as part of execution, making this base class largely unused.
| finally: | ||
| threading.excepthook = orig_excepthook | ||
| self.assertEqual(len(captured_exc), 1) | ||
| self.assertIsInstance(captured_exc[0], RuntimeError) |
There was a problem hiding this comment.
Can we instead check that target is called only once due to an exception being thrown? That would be equivalent here and require less code.
| self.assertLess(call_times[1] - call_times[0], 5.0) | ||
|
|
||
| async def test_wake_causes_early_run(self): | ||
| call_count = [0] |
There was a problem hiding this comment.
This can be a plain integer if we use nonlocal in target.
There was a problem hiding this comment.
TIL nonlocal exists.
| await self.executor.join(timeout=3) | ||
| self.assertGreaterEqual(call_count[0], 2) | ||
|
|
||
| async def test_open_after_target_returns_false(self): |
There was a problem hiding this comment.
Same here as above, prefer nonlocal over the list trick.
aclark4life
force-pushed
the
PYTHON-5784
branch
5 times, most recently
from
b6846db to
5dbfb54
Compare
…cated API, gc-safe weakref assertion, non-blocking shutdown test, retrieve task exception
…ous/ via synchro - Create test/asynchronous/test_periodic_executor.py as the single source of truth for all periodic executor tests, using AsyncUnitTest with asyncSetUp/ asyncTearDown base class for executor lifecycle management - Register test_periodic_executor.py in synchro's converted_tests so the sync variant is auto-generated - Replace the manually-maintained test/test_periodic_executor.py with the synchro-generated equivalent, eliminating duplicated async/sync test code - Use _IS_SYNC branching for the small number of tests that differ between threading (PeriodicExecutor) and asyncio (AsyncPeriodicExecutor) behavior
- Replace pe_module alias with periodic_executor - Remove underscore from base test class name - Alpha sort test_periodic_executor.py in synchro list - Rename ex to executor in tests - Rename r to executor_repr in repr test - Replace boom with error in exception messages
aclark4life
force-pushed
the
PYTHON-5784
branch
2 times, most recently
from
6fa5365 to
39cf1f4
Compare
aclark4life
changed the title
aclark4life
changed the title
aclark4life
changed the title
periodic_executor.py
aclark4life
changed the title
periodic_executor.pyperiodic_executor.py
aclark4life
changed the title
periodic_executor.pyperiodic_executor.py
aclark4life
changed the title
periodic_executor.pyperiodic_executor.py
| def async_only_test(f: str) -> bool: | ||
| """Return True for async tests that should not be converted to sync.""" | ||
| return f in [ | ||
| "test_locks.py", | ||
| "test_concurrency.py", | ||
| "test_async_cancellation.py", | ||
| "test_async_loop_safety.py", | ||
| "test_async_contextvars_reset.py", | ||
| "test_async_loop_unblocked.py", | ||
| "test_async_periodic_executor.py", | ||
| ] |
|
|
||
| sys.path[0:0] = [""] | ||
|
|
||
| from test.synchronous import UnitTest, unittest |
|
|
||
| from pymongo.periodic_executor import PeriodicExecutor | ||
|
|
||
| _IS_SYNC = False |
| _IS_SYNC = False | ||
|
|
||
|
|
||
| class TestAsyncPeriodicExecutor(UnitTest): |
| def test_target_returning_false_stops_executor(self): | ||
| if _IS_SYNC: | ||
| ran = threading.Event() | ||
| else: | ||
| ran = asyncio.Event() | ||
|
|
||
| def target(): | ||
| ran.set() | ||
| return False | ||
|
|
||
| executor = self._make_executor(target=target) | ||
| executor.open() | ||
| executor.join(timeout=2) | ||
| self.assertTrue(ran.is_set(), "target never ran") | ||
|
|
PYTHON-5784
Changes in this PR
Adds
test/asynchronous/test_periodic_executor.py(synchro'd totest/test_periodic_executor.py) with unit tests forpymongo/periodic_executor.py, covering bothPeriodicExecutorandAsyncPeriodicExecutor:join()beforeopen()is a safe no-op.Falsestops the executor.skip_sleep()causes the next iteration to run without waiting the full interval.wake()interrupts the current sleep and triggers an early run.open()after the target returnedFalserestarts the executor.Test Plan
Added unit tests using
asyncio.Event/threading.Eventfor deterministic synchronization. The exception test swapsthreading.excepthookto a no-op so the intentionalRuntimeErrorin the worker thread is not surfaced by pytest's threadexception plugin (no-op for the async executor since no thread is involved).Checklist
Checklist for Author
Checklist for Reviewer