PYTHON-5784 Coverage increase for `periodic_executor.py` by aclark4life · Pull Request #2771 · mongodb/mongo-python-driver

aclark4life · 2026-04-23T21:34:15Z

Changes in this PR

Adds test/asynchronous/test_periodic_executor.py (synchro'd to test/test_periodic_executor.py) with unit tests for pymongo/periodic_executor.py, covering both PeriodicExecutor and AsyncPeriodicExecutor:

join() before open() is a safe no-op.
Target returning False stops the executor.
Target raising an exception stops the executor (verified by asserting target is called exactly once).
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 returned False restarts the executor.

Test Plan

Added unit tests using asyncio.Event / threading.Event for deterministic synchronization. The exception test swaps threading.excepthook to a no-op so the intentional RuntimeError in the worker thread is not surfaced by pytest's threadexception plugin (no-op for the async executor since no thread is involved).

uv run --extra test python -m pytest test/asynchronous/test_periodic_executor.py test/test_periodic_executor.py -v

Checklist

Checklist for Author

Did you update the changelog (if necessary)?
Is there test coverage?
Is any followup work tracked in a JIRA ticket? If so, add link(s).

Checklist for Reviewer

Does the title of the PR reference a JIRA Ticket?
Do you fully understand the implementation? (Would you be comfortable explaining how this code works to someone else?)
Is all relevant documentation (README or docstring) updated?

Copilot

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 PeriodicExecutor lifecycle and control methods (open/close/join/wake/update_interval/skip_sleep, stop conditions).
Adds new unit tests for AsyncPeriodicExecutor lifecycle and stop conditions.
Adds unit tests for module-level _register_executor() and _shutdown_executors() behavior.

codecov-commenter · 2026-04-23T22:06:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

NoahStapp · 2026-04-24T17:15:48Z

+    async def _default_target():
+        return True
+
+    if target is None:
+        target = _default_target


Does this need to differ from the way _make_sync above does it?

NoahStapp · 2026-04-24T17:17:31Z

+def _run(coroutine):
+    return asyncio.run(coroutine)


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.

NoahStapp · 2026-04-24T17:19:03Z

+
+class TestPeriodicExecutorRepr(unittest.TestCase):
+    def test_repr_contains_class_and_name(self):
+        ex = _make_sync(name="myexec")


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.

NoahStapp · 2026-04-24T17:20:06Z

+        finally:
+            ex.close()
+            ex.join(timeout=2)
+


Similarly, safe closing of each executor can be done in an asyncTearDown/tearDown method on a base class.

NoahStapp · 2026-04-24T17:23:11Z

+# ---------------------------------------------------------------------------
+
+
+class TestAsyncPeriodicExecutorRepr(unittest.TestCase):


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.

Should #2774 get the same treatment?

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.

NoahStapp · 2026-05-04T14:30:47Z

+        await self.executor.join(timeout=2)
+
+
+class TestAsyncPeriodicExecutorRepr(AsyncUnitTest):


This class should also inherit from AsyncPeriodicExecutorTestBase and use its setup/teardown methods.

NoahStapp · 2026-05-04T14:31:29Z

+        self.assertIn("exec", executor_repr)
+
+
+class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase):


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.

This question still stands @aclark4life.

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.

NoahStapp · 2026-05-04T14:32:38Z

+
+
+class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase):
+    async def test_update_interval(self):


This test and the following don't test behavior but implementation.

NoahStapp · 2026-05-04T14:36:44Z

+
+
+class TestAsyncPeriodicExecutorLifecycle(AsyncPeriodicExecutorTestBase):
+    async def test_open_starts_worker(self):


Same here, this is testing implementation, not behavior.

NoahStapp · 2026-05-04T14:36:53Z

+        else:
+            self.assertIsNotNone(self.executor._task)
+
+    async def test_close_sets_stopped(self):


NoahStapp · 2026-05-04T14:52:37Z

+        self.executor = _make_executor(target=target)
+        self.executor.open()
+        if _IS_SYNC:
+            self.assertTrue(ran.wait(timeout=2), "target never ran")


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.

NoahStapp · 2026-05-04T14:52:46Z

+        else:
+            await asyncio.wait_for(ran.wait(), timeout=2)
+        await self.executor.join(timeout=2)
+        self.assertTrue(self.executor._stopped)


We can remove this if my above comment applies.

NoahStapp · 2026-05-04T15:06:24Z

+
+                self.executor = _make_executor(target=target)
+                self.executor.open()
+                self.assertTrue(ran.wait(timeout=2), "target never ran")


Same here as above.

NoahStapp · 2026-05-04T15:09:08Z

+
+            self.executor = _make_executor(target=target)
+            self.executor.open()
+            await asyncio.wait_for(ran.wait(), timeout=2)


NoahStapp · 2026-05-04T15:09:48Z

+        call_times = []
+
+        async def target():
+            call_times.append(time.monotonic() if _IS_SYNC else asyncio.get_running_loop().time())


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.

NoahStapp · 2026-05-05T13:38:56Z

+        self.assertGreaterEqual(called[0], 2)
+
+
+class TestShouldStop(AsyncUnitTest):


Both of these tests are again testing implementation, not behavior. Do they add confidence in the behavior of PeriodicExecutor when used?

NoahStapp · 2026-05-05T13:41:48Z

+            self.assertTrue(executor._thread_will_exit)
+
+
+class TestRegisterExecutor(AsyncUnitTest):


Does this test meaningfully add coverage that our existingtest_cleanup_executors_on_client_del in test/test_monitor.py does not?

NoahStapp · 2026-05-05T13:46:03Z

+    )
+
+
+class AsyncPeriodicExecutorTestBase(AsyncUnitTest):


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.

NoahStapp · 2026-05-05T13:48:13Z

+            finally:
+                threading.excepthook = orig_excepthook
+            self.assertEqual(len(captured_exc), 1)
+            self.assertIsInstance(captured_exc[0], RuntimeError)


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.

NoahStapp · 2026-05-05T14:11:29Z

+        self.assertLess(call_times[1] - call_times[0], 5.0)
+
+    async def test_wake_causes_early_run(self):
+        call_count = [0]


This can be a plain integer if we use nonlocal in target.

TIL nonlocal exists.

NoahStapp · 2026-05-05T14:16:40Z

+        await self.executor.join(timeout=3)
+        self.assertGreaterEqual(call_count[0], 2)
+
+    async def test_open_after_target_returns_false(self):


Same here as above, prefer nonlocal over the list trick.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

 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")
+


blink1073 · 2026-06-03T20:51:16Z

+        self.assertGreaterEqual(call_count, 2)
+
+    async def test_update_interval_changes_next_wait(self):
+        call_times = []


This call site needs to be updated to use nonlocal as well

…5% to >=80%

…ayer.py style

…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

The synchronous test was generated but not post-processed (still had 'from test.synchronous import', _IS_SYNC=False, and the async class name) because test_periodic_executor.py was missing from converted_tests. Add it and regenerate so the sync file flips _IS_SYNC=True, fixes its imports, and renames the class to TestPeriodicExecutor.

Add test_update_interval_changes_next_wait: shorten the interval during the first run and assert the next run happens promptly (rather than after the original 30s), exercising update_interval() on both executors.

update_interval() stores into _interval (float) and is called with float values, but was annotated new_interval: int. Correct the annotation on both executors and use a small positive interval in the coverage test.

Consolidate the target-exception test into the synchro'd source so both AsyncPeriodicExecutor and the threaded PeriodicExecutor are covered from a single source, and delete the async-only test file. Why this reverses the async-only split from 3340959: that commit carved the exception test out into test_async_periodic_executor.py to dodge the noisy traceback an uncaught exception prints via threading.excepthook on the background thread. But the two _run loops handle the exception differently -- the threaded version additionally takes the lock and sets _thread_will_exit, which feeds the join-on-reopen path in open(). The async test therefore did not cover that sync-only branch, leaving a real gap (also flagged by Copilot). Rather than maintain two near-identical hand-written tests (which cuts against the synchro direction this PR has otherwise followed), the test now lives in the async source with an `if _IS_SYNC:` branch that swaps threading.excepthook to a no-op for the threaded case. Verified the generated sync test passes 15/15 with no flakiness or stderr noise, which confirms the path is testable and there was no real reason to keep it async-only.

Addressed

blink1073

LGTM

Copilot AI review requested due to automatic review settings April 23, 2026 21:34

aclark4life requested a review from a team as a code owner April 23, 2026 21:34

aclark4life requested a review from NoahStapp April 23, 2026 21:34

Copilot started reviewing on behalf of aclark4life April 23, 2026 21:34 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

NoahStapp requested changes Apr 24, 2026

View reviewed changes

aclark4life requested a review from NoahStapp April 29, 2026 18:32

NoahStapp requested changes May 4, 2026

View reviewed changes

aclark4life force-pushed the PYTHON-5784 branch from bcf54c7 to 3d8a78e Compare May 4, 2026 22:46

aclark4life requested a review from NoahStapp May 4, 2026 22:47

NoahStapp previously requested changes May 5, 2026

View reviewed changes

aclark4life force-pushed the PYTHON-5784 branch 5 times, most recently from b6846db to 5dbfb54 Compare May 5, 2026 22:03

aclark4life requested a review from NoahStapp May 6, 2026 02:09

aclark4life marked this pull request as draft May 6, 2026 18:23

aclark4life force-pushed the PYTHON-5784 branch from 2595ac4 to 0b948d8 Compare May 6, 2026 18:41

aclark4life requested a review from Copilot May 6, 2026 19:47

Copilot started reviewing on behalf of aclark4life May 6, 2026 19:48 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread test/test_periodic_executor.py

Comment thread test/asynchronous/test_periodic_executor.py Outdated

aclark4life force-pushed the PYTHON-5784 branch 2 times, most recently from 6fa5365 to 39cf1f4 Compare May 6, 2026 22:15

aclark4life requested a review from Copilot May 7, 2026 02:02

Copilot started reviewing on behalf of aclark4life May 7, 2026 02:02 View session

aclark4life changed the title ~~PYTHON-5784 Increase code coverage for periodic_executor.py~~ PYTHON-5784 Improve code coverage for periodic_executor.py May 7, 2026

aclark4life changed the title ~~PYTHON-5784 Improve code coverage for periodic_executor.py~~ PYTHON-5784 Improve coverage for periodic_executor.py May 7, 2026

aclark4life changed the title ~~PYTHON-5784 Improve coverage for periodic_executor.py~~ PYTHON-5784 Improve coverage for periodic_executor.py May 7, 2026

aclark4life force-pushed the PYTHON-5784 branch from bf1fa7d to 71a240c Compare May 7, 2026 02:41

aclark4life requested a review from Copilot May 7, 2026 03:02

Copilot AI reviewed May 7, 2026

View reviewed changes

aclark4life force-pushed the PYTHON-5784 branch from 71a240c to f8405ba Compare May 29, 2026 23:36

aclark4life removed the request for review from NoahStapp May 29, 2026 23:45

aclark4life force-pushed the PYTHON-5784 branch 2 times, most recently from 4160761 to 6936ea8 Compare June 3, 2026 18:57

aclark4life marked this pull request as ready for review June 3, 2026 19:26

aclark4life requested a review from blink1073 June 3, 2026 19:26

blink1073 requested changes Jun 3, 2026

View reviewed changes

aclark4life added 14 commits June 3, 2026 19:19

PYTHON-5784 Increase code coverage for periodic_executor.py from 79.2…

88a2dc9

…5% to >=80%

PYTHON-5784 Use _run() helper for async tests to match test_network_l…

12c9a5e

…ayer.py style

PYTHON-5784 Spell out 'coro' parameter as 'coroutine' in _run helper

44e37a1

PYTHON-5784 Address Copilot feedback: remove unused import, fix depre…

cb7a1b2

…cated API, gc-safe weakref assertion, non-blocking shutdown test, retrieve task exception

Noah feedback

6bc8dd4

Fix typing errors

e6210e9

Copilot + Noah review

33423d9

PYTHON-5784 Add update_interval() coverage

372d9b7

Add test_update_interval_changes_next_wait: shorten the interval during the first run and assert the next run happens promptly (rather than after the original 30s), exercising update_interval() on both executors.

PYTHON-5784 Fix update_interval type annotation to float

ea0db63

update_interval() stores into _interval (float) and is called with float values, but was annotated new_interval: int. Correct the annotation on both executors and use a small positive interval in the coverage test.

Steve feedback

16e109f

aclark4life force-pushed the PYTHON-5784 branch from e3a771b to 16e109f Compare June 3, 2026 23:19

blink1073 approved these changes Jun 3, 2026

View reviewed changes

aclark4life merged commit ed1ddb6 into mongodb:master Jun 4, 2026
87 of 89 checks passed

aclark4life deleted the PYTHON-5784 branch June 4, 2026 15:01

aclark4life mentioned this pull request Jun 4, 2026

PYTHON-5781 Coverage increase for network_layer.py #2774

Open

6 tasks

		# ---------------------------------------------------------------------------


		class TestAsyncPeriodicExecutorRepr(unittest.TestCase):

		await self.executor.join(timeout=2)


		class TestAsyncPeriodicExecutorRepr(AsyncUnitTest):

		self.assertIn("exec", executor_repr)


		class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase):



		class TestAsyncPeriodicExecutorBasic(AsyncPeriodicExecutorTestBase):
		async def test_update_interval(self):



		class TestAsyncPeriodicExecutorLifecycle(AsyncPeriodicExecutorTestBase):
		async def test_open_starts_worker(self):

		self.assertGreaterEqual(called[0], 2)


		class TestShouldStop(AsyncUnitTest):

		self.assertTrue(executor._thread_will_exit)


		class TestRegisterExecutor(AsyncUnitTest):


		sys.path[0:0] = [""]

		from test.synchronous import UnitTest, unittest


		from pymongo.periodic_executor import PeriodicExecutor

		_IS_SYNC = False

Conversation

aclark4life commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes in this PR

Test Plan

Checklist

Checklist for Author

Checklist for Reviewer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

aclark4life commented Apr 23, 2026 •

edited

Loading

codecov-commenter commented Apr 23, 2026 •

edited

Loading