test(worker): Add tests for blocking_timeout behavior in LockManager #600
Conversation
drazisil-codecov
commented
Dec 8, 2025
drazisil-codecov
commented
- Introduced tests to verify that blocking_timeout=None causes indefinite blocking, preventing retry logic from functioning correctly.
- Added a test to ensure that a valid blocking_timeout enables retry logic by raising LockError.
- Updated BundleAnalysisProcessorTask tests to confirm that the default blocking_timeout is used, preventing indefinite blocking.
- Adjusted assertions to reflect changes in retry logic and blocking_timeout handling.
- Introduced tests to verify that blocking_timeout=None causes indefinite blocking, preventing retry logic from functioning correctly. - Added a test to ensure that a valid blocking_timeout enables retry logic by raising LockError. - Updated BundleAnalysisProcessorTask tests to confirm that the default blocking_timeout is used, preventing indefinite blocking. - Adjusted assertions to reflect changes in retry logic and blocking_timeout handling.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #600 +/- ##
=======================================
Coverage 93.87% 93.87%
=======================================
Files 1284 1284
Lines 46667 46667
Branches 1522 1522
=======================================
Hits 43809 43809
Misses 2548 2548
Partials 310 310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
thomasrockhu-codecov
left a comment
thomasrockhu-codecov
left a comment
There was a problem hiding this comment.
With all the comments, it's really difficult to understand what is going on in the tests, please update to make it human-readable
| blocking_timeouts = [] | ||
| lock_called = threading.Event() | ||
|
|
||
| def mock_lock(*args, **kwargs): |
There was a problem hiding this comment.
probably shouldn't be function in a function here and more of a helper
| if kwargs.get("blocking_timeout") is None: | ||
| # With blocking_timeout=None, Redis blocks forever and never raises LockError | ||
| # This simulates the blocking behavior - it would hang indefinitely | ||
| # We use a longer sleep to ensure the thread hangs | ||
| time.sleep(1.0) # Simulate blocking (longer than thread timeout) | ||
| return blocking_lock | ||
| else: | ||
| # With blocking_timeout set, Redis raises LockError immediately | ||
| raise LockError() |
There was a problem hiding this comment.
| if kwargs.get("blocking_timeout") is None: | |
| # With blocking_timeout=None, Redis blocks forever and never raises LockError | |
| # This simulates the blocking behavior - it would hang indefinitely | |
| # We use a longer sleep to ensure the thread hangs | |
| time.sleep(1.0) # Simulate blocking (longer than thread timeout) | |
| return blocking_lock | |
| else: | |
| # With blocking_timeout set, Redis raises LockError immediately | |
| raise LockError() | |
| if kwargs.get("blocking_timeout") is not None: | |
| raise LockError() | |
| time.sleep(1.0) | |
| return blocking_lock |
I think this is a better pardigm
| # With blocking_timeout set, Redis raises LockError immediately | ||
| raise LockError() | ||
|
|
||
| mock_redis.lock = mock_lock |
There was a problem hiding this comment.
I don't think this is necessary
| exception_raised = [] | ||
| completed = threading.Event() | ||
|
|
||
| def test_locked(): |
There was a problem hiding this comment.
yeah, I don't love all these helper functions for just one test function. either pull it out or not. also, are all these excepts really necessary? it seems like it's just confusing the code
| thread = threading.Thread(target=test_locked) | ||
| thread.daemon = True | ||
| thread.start() |
There was a problem hiding this comment.
if we're doing threading code, perhaps it should exist in a separate helper function as well
