Copilot + Noah review

aclark4life · aclark4life · commit 71a240c5765b · 2026-05-06T22:41:37.000-04:00
diff --git a/test/asynchronous/test_async_periodic_executor.py b/test/asynchronous/test_async_periodic_executor.py
@@ -0,0 +1,48 @@
+# Copyright 2026-present MongoDB, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Async-only unit tests for AsyncPeriodicExecutor."""
+
+from __future__ import annotations
+
+import sys
+
+sys.path[0:0] = [""]
+
+from test.asynchronous import AsyncUnitTest, unittest
+
+from pymongo.periodic_executor import AsyncPeriodicExecutor
+
+
+class TestAsyncPeriodicExecutorExceptions(AsyncUnitTest):
+    async def test_target_exception_stops_executor(self):
+        call_count = 0
+
+        async def target():
+            nonlocal call_count
+            call_count += 1
+            raise RuntimeError("error")
+
+        executor = AsyncPeriodicExecutor(
+            interval=30.0, min_interval=0.01, target=target, name="test"
+        )
+        executor.open()
+        await executor.join(timeout=2)
+        if executor._task is not None and executor._task.done():
+            executor._task.exception()
+        self.assertEqual(call_count, 1, "target should stop after exception")
+
+
+if __name__ == "__main__":
+    unittest.main()
diff --git a/test/asynchronous/test_periodic_executor.py b/test/asynchronous/test_periodic_executor.py
@@ -30,38 +30,27 @@
 _IS_SYNC = False
 
 
-def _make_executor(interval=30.0, min_interval=0.01, target=None, name="test"):
-    if target is None:
+class TestAsyncPeriodicExecutor(AsyncUnitTest):
+    def _make_executor(self, interval=30.0, min_interval=0.01, target=None, name="test"):
+        if target is None:
 
-        async def target():
-            return True
-
-    return AsyncPeriodicExecutor(
-        interval=interval, min_interval=min_interval, target=target, name=name
-    )
-
-
-class AsyncPeriodicExecutorTestBase(AsyncUnitTest):
-    async def asyncSetUp(self):
-        self.executor = None
-
-    async def asyncTearDown(self):
-        if self.executor is not None:
-            self.executor.close()
-            await self.executor.join(timeout=2)
+            async def target():
+                return True
 
+        executor = AsyncPeriodicExecutor(
+            interval=interval, min_interval=min_interval, target=target, name=name
+        )
+        self.addAsyncCleanup(self._close_executor, executor)
+        return executor
 
-class TestAsyncPeriodicExecutor(AsyncPeriodicExecutorTestBase):
-    async def test_repr_contains_class_and_name(self):
-        executor = _make_executor(name="exec")
-        executor_repr = repr(executor)
-        self.assertIn("AsyncPeriodicExecutor", executor_repr)
-        self.assertIn("exec", executor_repr)
+    async def _close_executor(self, executor):
+        executor.close()
+        await executor.join(timeout=2)
 
     async def test_join_without_open_is_safe(self):
-        self.executor = _make_executor()
+        executor = self._make_executor()
         try:
-            await self.executor.join(timeout=0.01)
+            await executor.join(timeout=0.01)
         except Exception as e:
             self.fail(f"join() raised unexpected Exception {e}")
 
@@ -75,48 +64,11 @@ async def target():
             ran.set()
             return False
 
-        self.executor = _make_executor(target=target)
-        self.executor.open()
-        await self.executor.join(timeout=2)
+        executor = self._make_executor(target=target)
+        executor.open()
+        await executor.join(timeout=2)
         self.assertTrue(ran.is_set(), "target never ran")
 
-    async def test_target_exception_stops_executor(self):
-        if _IS_SYNC:
-            ran = threading.Event()
-            captured_exc: list = []
-            orig_excepthook = threading.excepthook
-
-            def _capture_excepthook(args):
-                captured_exc.append(args.exc_value)
-
-            threading.excepthook = _capture_excepthook
-            try:
-
-                def target():
-                    ran.set()
-                    raise RuntimeError("error")
-
-                self.executor = _make_executor(target=target)
-                self.executor.open()
-                self.executor.join(timeout=2)
-                self.assertTrue(ran.is_set(), "target never ran")
-            finally:
-                threading.excepthook = orig_excepthook
-            self.assertEqual(len(captured_exc), 1)
-            self.assertIsInstance(captured_exc[0], RuntimeError)
-        else:
-            call_count = 0
-
-            async def target():
-                nonlocal call_count
-                call_count += 1
-                raise RuntimeError("error")
-
-            self.executor = _make_executor(target=target)
-            self.executor.open()
-            await self.executor.join(timeout=2)
-            self.assertEqual(call_count, 1, "target should stop after exception")
-
     async def test_skip_sleep_flag_skips_interval(self):
         call_times = []
 
@@ -126,10 +78,10 @@ async def target():
                 return False
             return True
 
-        self.executor = _make_executor(interval=30.0, min_interval=0.001, target=target)
-        self.executor.skip_sleep()
-        self.executor.open()
-        await self.executor.join(timeout=3)
+        executor = self._make_executor(interval=30.0, min_interval=0.001, target=target)
+        executor.skip_sleep()
+        executor.open()
+        await executor.join(timeout=3)
         self.assertGreaterEqual(len(call_times), 2)
         self.assertLess(call_times[1] - call_times[0], 5.0)
 
@@ -147,15 +99,15 @@ async def target():
                 woken.set()
             return call_count < 2
 
-        self.executor = _make_executor(interval=30.0, min_interval=0.01, target=target)
-        self.executor.open()
+        executor = self._make_executor(interval=30.0, min_interval=0.01, target=target)
+        executor.open()
         if _IS_SYNC:
             woken.wait(timeout=2)
         else:
             assert isinstance(woken, asyncio.Event)
             await asyncio.wait_for(woken.wait(), timeout=2)
-        self.executor.wake()
-        await self.executor.join(timeout=3)
+        executor.wake()
+        await executor.join(timeout=3)
         self.assertGreaterEqual(call_count, 2)
 
     async def test_open_after_target_returns_false(self):
@@ -166,11 +118,11 @@ async def target():
             called += 1
             return False
 
-        self.executor = _make_executor(target=target)
-        self.executor.open()
-        await self.executor.join(timeout=2)
-        self.executor.open()
-        await self.executor.join(timeout=2)
+        executor = self._make_executor(target=target)
+        executor.open()
+        await executor.join(timeout=2)
+        executor.open()
+        await executor.join(timeout=2)
         self.assertGreaterEqual(called, 2)
 
 
diff --git a/test/test_periodic_executor.py b/test/test_periodic_executor.py
@@ -23,43 +23,34 @@
 
 sys.path[0:0] = [""]
 
-from test import UnitTest, unittest
+from test.synchronous import UnitTest, unittest
 
 from pymongo.periodic_executor import PeriodicExecutor
 
-_IS_SYNC = True
+_IS_SYNC = False
 
 
-def _make_executor(interval=30.0, min_interval=0.01, target=None, name="test"):
-    if target is None:
+class TestAsyncPeriodicExecutor(UnitTest):
+    def _make_executor(self, interval=30.0, min_interval=0.01, target=None, name="test"):
+        if target is None:
 
-        def target():
-            return True
-
-    return PeriodicExecutor(interval=interval, min_interval=min_interval, target=target, name=name)
-
-
-class PeriodicExecutorTestBase(UnitTest):
-    def setUp(self):
-        self.executor = None
-
-    def tearDown(self):
-        if self.executor is not None:
-            self.executor.close()
-            self.executor.join(timeout=2)
+            def target():
+                return True
 
+        executor = PeriodicExecutor(
+            interval=interval, min_interval=min_interval, target=target, name=name
+        )
+        self.addCleanup(self._close_executor, executor)
+        return executor
 
-class TestPeriodicExecutor(PeriodicExecutorTestBase):
-    def test_repr_contains_class_and_name(self):
-        executor = _make_executor(name="exec")
-        executor_repr = repr(executor)
-        self.assertIn("PeriodicExecutor", executor_repr)
-        self.assertIn("exec", executor_repr)
+    def _close_executor(self, executor):
+        executor.close()
+        executor.join(timeout=2)
 
     def test_join_without_open_is_safe(self):
-        self.executor = _make_executor()
+        executor = self._make_executor()
         try:
-            self.executor.join(timeout=0.01)
+            executor.join(timeout=0.01)
         except Exception as e:
             self.fail(f"join() raised unexpected Exception {e}")
 
@@ -73,48 +64,11 @@ def target():
             ran.set()
             return False
 
-        self.executor = _make_executor(target=target)
-        self.executor.open()
-        self.executor.join(timeout=2)
+        executor = self._make_executor(target=target)
+        executor.open()
+        executor.join(timeout=2)
         self.assertTrue(ran.is_set(), "target never ran")
 
-    def test_target_exception_stops_executor(self):
-        if _IS_SYNC:
-            ran = threading.Event()
-            captured_exc: list = []
-            orig_excepthook = threading.excepthook
-
-            def _capture_excepthook(args):
-                captured_exc.append(args.exc_value)
-
-            threading.excepthook = _capture_excepthook
-            try:
-
-                def target():
-                    ran.set()
-                    raise RuntimeError("error")
-
-                self.executor = _make_executor(target=target)
-                self.executor.open()
-                self.executor.join(timeout=2)
-                self.assertTrue(ran.is_set(), "target never ran")
-            finally:
-                threading.excepthook = orig_excepthook
-            self.assertEqual(len(captured_exc), 1)
-            self.assertIsInstance(captured_exc[0], RuntimeError)
-        else:
-            call_count = 0
-
-            def target():
-                nonlocal call_count
-                call_count += 1
-                raise RuntimeError("error")
-
-            self.executor = _make_executor(target=target)
-            self.executor.open()
-            self.executor.join(timeout=2)
-            self.assertEqual(call_count, 1, "target should stop after exception")
-
     def test_skip_sleep_flag_skips_interval(self):
         call_times = []
 
@@ -124,10 +78,10 @@ def target():
                 return False
             return True
 
-        self.executor = _make_executor(interval=30.0, min_interval=0.001, target=target)
-        self.executor.skip_sleep()
-        self.executor.open()
-        self.executor.join(timeout=3)
+        executor = self._make_executor(interval=30.0, min_interval=0.001, target=target)
+        executor.skip_sleep()
+        executor.open()
+        executor.join(timeout=3)
         self.assertGreaterEqual(len(call_times), 2)
         self.assertLess(call_times[1] - call_times[0], 5.0)
 
@@ -145,15 +99,15 @@ def target():
                 woken.set()
             return call_count < 2
 
-        self.executor = _make_executor(interval=30.0, min_interval=0.01, target=target)
-        self.executor.open()
+        executor = self._make_executor(interval=30.0, min_interval=0.01, target=target)
+        executor.open()
         if _IS_SYNC:
             woken.wait(timeout=2)
         else:
             assert isinstance(woken, asyncio.Event)
             asyncio.wait_for(woken.wait(), timeout=2)
-        self.executor.wake()
-        self.executor.join(timeout=3)
+        executor.wake()
+        executor.join(timeout=3)
         self.assertGreaterEqual(call_count, 2)
 
     def test_open_after_target_returns_false(self):
@@ -164,11 +118,11 @@ def target():
             called += 1
             return False
 
-        self.executor = _make_executor(target=target)
-        self.executor.open()
-        self.executor.join(timeout=2)
-        self.executor.open()
-        self.executor.join(timeout=2)
+        executor = self._make_executor(target=target)
+        executor.open()
+        executor.join(timeout=2)
+        executor.open()
+        executor.join(timeout=2)
         self.assertGreaterEqual(called, 2)
 
 
diff --git a/tools/synchro.py b/tools/synchro.py
@@ -190,6 +190,7 @@ def async_only_test(f: str) -> bool:
         "test_async_loop_safety.py",
         "test_async_contextvars_reset.py",
         "test_async_loop_unblocked.py",
+        "test_async_periodic_executor.py",
     ]
 
 
@@ -252,7 +253,6 @@ def async_only_test(f: str) -> bool:
     "test_monitoring.py",
     "test_mongos_load_balancing.py",
     "test_on_demand_csfle.py",
-    "test_periodic_executor.py",
     "test_pooling.py",
     "test_raw_bson.py",
     "test_read_concern.py",
