Skip to content

Commit b6846db

Browse files
aclark4lifeaclark4life
committed
Noah feedback
1 parent 4acc723 commit b6846db

2 files changed

Lines changed: 40 additions & 154 deletions

File tree

‎test/asynchronous/test_periodic_executor.py‎

Lines changed: 20 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
from __future__ import annotations
1818

1919
import asyncio
20-
import gc
2120
import sys
2221
import threading
2322
import time
@@ -26,12 +25,7 @@
2625

2726
from test.asynchronous import AsyncUnitTest, unittest
2827

29-
from pymongo import periodic_executor
30-
from pymongo.periodic_executor import (
31-
AsyncPeriodicExecutor,
32-
_register_executor,
33-
_shutdown_executors,
34-
)
28+
from pymongo.periodic_executor import AsyncPeriodicExecutor
3529

3630
_IS_SYNC = False
3731

@@ -49,11 +43,12 @@ async def target():
4943

5044
class AsyncPeriodicExecutorTestBase(AsyncUnitTest):
5145
async def asyncSetUp(self):
52-
self.executor = _make_executor()
46+
self.executor = None
5347

5448
async def asyncTearDown(self):
55-
self.executor.close()
56-
await self.executor.join(timeout=2)
49+
if self.executor is not None:
50+
self.executor.close()
51+
await self.executor.join(timeout=2)
5752

5853

5954
class TestAsyncPeriodicExecutor(AsyncPeriodicExecutorTestBase):
@@ -64,6 +59,7 @@ async def test_repr_contains_class_and_name(self):
6459
self.assertIn("exec", executor_repr)
6560

6661
async def test_join_without_open_is_safe(self):
62+
self.executor = _make_executor()
6763
try:
6864
await self.executor.join(timeout=0.01)
6965
except Exception as e:
@@ -109,18 +105,17 @@ def target():
109105
self.assertEqual(len(captured_exc), 1)
110106
self.assertIsInstance(captured_exc[0], RuntimeError)
111107
else:
112-
ran = asyncio.Event()
108+
call_count = 0
113109

114110
async def target():
115-
ran.set()
111+
nonlocal call_count
112+
call_count += 1
116113
raise RuntimeError("error")
117114

118115
self.executor = _make_executor(target=target)
119116
self.executor.open()
120117
await self.executor.join(timeout=2)
121-
self.assertTrue(ran.is_set(), "target never ran")
122-
if self.executor._task is not None and self.executor._task.done():
123-
self.executor._task.exception()
118+
self.assertEqual(call_count, 1, "target should stop after exception")
124119

125120
async def test_skip_sleep_flag_skips_interval(self):
126121
call_times = []
@@ -139,19 +134,18 @@ async def target():
139134
self.assertLess(call_times[1] - call_times[0], 5.0)
140135

141136
async def test_wake_causes_early_run(self):
142-
call_count = [0]
137+
call_count = 0
143138
if _IS_SYNC:
144139
woken = threading.Event()
145140
else:
146141
woken = asyncio.Event()
147142

148143
async def target():
149-
call_count[0] += 1
150-
if call_count[0] == 1:
144+
nonlocal call_count
145+
call_count += 1
146+
if call_count == 1:
151147
woken.set()
152-
if call_count[0] >= 2:
153-
return False
154-
return True
148+
return call_count < 2
155149

156150
self.executor = _make_executor(interval=30.0, min_interval=0.01, target=target)
157151
self.executor.open()
@@ -161,73 +155,22 @@ async def target():
161155
await asyncio.wait_for(woken.wait(), timeout=2)
162156
self.executor.wake()
163157
await self.executor.join(timeout=3)
164-
self.assertGreaterEqual(call_count[0], 2)
158+
self.assertGreaterEqual(call_count, 2)
165159

166160
async def test_open_after_target_returns_false(self):
167-
called = [0]
161+
called = 0
168162

169163
async def target():
170-
called[0] += 1
164+
nonlocal called
165+
called += 1
171166
return False
172167

173168
self.executor = _make_executor(target=target)
174169
self.executor.open()
175170
await self.executor.join(timeout=2)
176171
self.executor.open()
177172
await self.executor.join(timeout=2)
178-
self.assertGreaterEqual(called[0], 2)
179-
180-
181-
class TestShouldStop(AsyncUnitTest):
182-
if _IS_SYNC:
183-
184-
def test_returns_false_when_not_stopped(self):
185-
executor = _make_executor()
186-
self.assertFalse(executor._should_stop())
187-
self.assertFalse(executor._thread_will_exit)
188-
189-
def test_returns_true_and_sets_thread_will_exit(self):
190-
executor = _make_executor()
191-
executor._stopped = True
192-
self.assertTrue(executor._should_stop())
193-
self.assertTrue(executor._thread_will_exit)
194-
195-
196-
class TestRegisterExecutor(AsyncUnitTest):
197-
if _IS_SYNC:
198-
199-
def setUp(self):
200-
self._orig = set(periodic_executor._EXECUTORS)
201-
202-
def tearDown(self):
203-
periodic_executor._EXECUTORS.clear()
204-
periodic_executor._EXECUTORS.update(self._orig)
205-
206-
def test_register_adds_weakref(self):
207-
executor = _make_executor()
208-
before = len(periodic_executor._EXECUTORS)
209-
_register_executor(executor)
210-
self.assertEqual(len(periodic_executor._EXECUTORS), before + 1)
211-
ref = next(r for r in periodic_executor._EXECUTORS if r() is executor)
212-
del executor
213-
gc.collect()
214-
self.assertNotIn(ref, periodic_executor._EXECUTORS)
215-
216-
def test_shutdown_executors_stops_running_executors(self):
217-
ran = threading.Event()
218-
219-
def target():
220-
ran.set()
221-
return True
222-
223-
executor = _make_executor(target=target)
224-
executor.open()
225-
self.assertTrue(ran.wait(timeout=2), "target never ran")
226-
_shutdown_executors()
227-
228-
def test_shutdown_executors_safe_when_empty(self):
229-
periodic_executor._EXECUTORS.clear()
230-
_shutdown_executors()
173+
self.assertGreaterEqual(called, 2)
231174

232175

233176
if __name__ == "__main__":

‎test/test_periodic_executor.py‎

Lines changed: 20 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
from __future__ import annotations
1818

1919
import asyncio
20-
import gc
2120
import sys
2221
import threading
2322
import time
@@ -26,12 +25,7 @@
2625

2726
from test import UnitTest, unittest
2827

29-
from pymongo import periodic_executor
30-
from pymongo.periodic_executor import (
31-
PeriodicExecutor,
32-
_register_executor,
33-
_shutdown_executors,
34-
)
28+
from pymongo.periodic_executor import PeriodicExecutor
3529

3630
_IS_SYNC = True
3731

@@ -47,11 +41,12 @@ def target():
4741

4842
class PeriodicExecutorTestBase(UnitTest):
4943
def setUp(self):
50-
self.executor = _make_executor()
44+
self.executor = None
5145

5246
def tearDown(self):
53-
self.executor.close()
54-
self.executor.join(timeout=2)
47+
if self.executor is not None:
48+
self.executor.close()
49+
self.executor.join(timeout=2)
5550

5651

5752
class TestPeriodicExecutor(PeriodicExecutorTestBase):
@@ -62,6 +57,7 @@ def test_repr_contains_class_and_name(self):
6257
self.assertIn("exec", executor_repr)
6358

6459
def test_join_without_open_is_safe(self):
60+
self.executor = _make_executor()
6561
try:
6662
self.executor.join(timeout=0.01)
6763
except Exception as e:
@@ -107,18 +103,17 @@ def target():
107103
self.assertEqual(len(captured_exc), 1)
108104
self.assertIsInstance(captured_exc[0], RuntimeError)
109105
else:
110-
ran = asyncio.Event()
106+
call_count = 0
111107

112108
def target():
113-
ran.set()
109+
nonlocal call_count
110+
call_count += 1
114111
raise RuntimeError("error")
115112

116113
self.executor = _make_executor(target=target)
117114
self.executor.open()
118115
self.executor.join(timeout=2)
119-
self.assertTrue(ran.is_set(), "target never ran")
120-
if self.executor._task is not None and self.executor._task.done():
121-
self.executor._task.exception()
116+
self.assertEqual(call_count, 1, "target should stop after exception")
122117

123118
def test_skip_sleep_flag_skips_interval(self):
124119
call_times = []
@@ -137,19 +132,18 @@ def target():
137132
self.assertLess(call_times[1] - call_times[0], 5.0)
138133

139134
def test_wake_causes_early_run(self):
140-
call_count = [0]
135+
call_count = 0
141136
if _IS_SYNC:
142137
woken = threading.Event()
143138
else:
144139
woken = asyncio.Event()
145140

146141
def target():
147-
call_count[0] += 1
148-
if call_count[0] == 1:
142+
nonlocal call_count
143+
call_count += 1
144+
if call_count == 1:
149145
woken.set()
150-
if call_count[0] >= 2:
151-
return False
152-
return True
146+
return call_count < 2
153147

154148
self.executor = _make_executor(interval=30.0, min_interval=0.01, target=target)
155149
self.executor.open()
@@ -159,73 +153,22 @@ def target():
159153
asyncio.wait_for(woken.wait(), timeout=2)
160154
self.executor.wake()
161155
self.executor.join(timeout=3)
162-
self.assertGreaterEqual(call_count[0], 2)
156+
self.assertGreaterEqual(call_count, 2)
163157

164158
def test_open_after_target_returns_false(self):
165-
called = [0]
159+
called = 0
166160

167161
def target():
168-
called[0] += 1
162+
nonlocal called
163+
called += 1
169164
return False
170165

171166
self.executor = _make_executor(target=target)
172167
self.executor.open()
173168
self.executor.join(timeout=2)
174169
self.executor.open()
175170
self.executor.join(timeout=2)
176-
self.assertGreaterEqual(called[0], 2)
177-
178-
179-
class TestShouldStop(UnitTest):
180-
if _IS_SYNC:
181-
182-
def test_returns_false_when_not_stopped(self):
183-
executor = _make_executor()
184-
self.assertFalse(executor._should_stop())
185-
self.assertFalse(executor._thread_will_exit)
186-
187-
def test_returns_true_and_sets_thread_will_exit(self):
188-
executor = _make_executor()
189-
executor._stopped = True
190-
self.assertTrue(executor._should_stop())
191-
self.assertTrue(executor._thread_will_exit)
192-
193-
194-
class TestRegisterExecutor(UnitTest):
195-
if _IS_SYNC:
196-
197-
def setUp(self):
198-
self._orig = set(periodic_executor._EXECUTORS)
199-
200-
def tearDown(self):
201-
periodic_executor._EXECUTORS.clear()
202-
periodic_executor._EXECUTORS.update(self._orig)
203-
204-
def test_register_adds_weakref(self):
205-
executor = _make_executor()
206-
before = len(periodic_executor._EXECUTORS)
207-
_register_executor(executor)
208-
self.assertEqual(len(periodic_executor._EXECUTORS), before + 1)
209-
ref = next(r for r in periodic_executor._EXECUTORS if r() is executor)
210-
del executor
211-
gc.collect()
212-
self.assertNotIn(ref, periodic_executor._EXECUTORS)
213-
214-
def test_shutdown_executors_stops_running_executors(self):
215-
ran = threading.Event()
216-
217-
def target():
218-
ran.set()
219-
return True
220-
221-
executor = _make_executor(target=target)
222-
executor.open()
223-
self.assertTrue(ran.wait(timeout=2), "target never ran")
224-
_shutdown_executors()
225-
226-
def test_shutdown_executors_safe_when_empty(self):
227-
periodic_executor._EXECUTORS.clear()
228-
_shutdown_executors()
171+
self.assertGreaterEqual(called, 2)
229172

230173

231174
if __name__ == "__main__":

0 commit comments

Comments
 (0)