test(supervisors): fix occasional assertion failures and hangs #2431
Conversation
…lly' Ensure proper cleanup in 'test_multiprocess' by wrapping the supervisor shutdown logic in a 'finally' block. This guarantees that the supervisor is properly terminated and all processes are joined, avoiding potential resource leaks or state residuebetween test runs.
|
Now there are two problems, one is that the process hangs when assertion fails(This PR solves), and the second is that it hangs on execution. related: I create a Windows environment, but it's hard to reproduce.🙃 |
A10-second timeout has been added to the subprocess call within the test_multiprocess.py to ensure it does not hang indefinitely in case of failures or slow systems.
|
Ah, It seems to make sense now. Successful cases are all executed for more than 7s, that is, when the ping is executed, the process is really closed, and then the timeout returns false. I add a timeout of 1 second to the assertion here, and the total execution time is more than 3 s, indicating that the ping method is taken here. The same.
|
|
See? No need for retry. 😌👍 Thanks for your time investigating this. 🙏 |
vvanglro
changed the title
| @@ -34,6 +34,7 @@ def new_function(): | |||
| "-c", | |||
| f"from {module} import {name}; {name}.__wrapped__()", | |||
| ], | |||
| timeout=10, | |||
There was a problem hiding this comment.
Oh, this was added earlier when I was investigating why it was hanging. No need for it now, I'll remove it later
- Replace fixed sleep with retry mechanism for process status checks- Enhance test stability by allowing for variable process startup times
Head branch was pushed to by a user without write access
|
Thanks. |
|
We can't control it with precision, so we might as well retry. Just make sure the dead process is eventually started successfully. |
Kludex
dismissed
their stale review
Retries are not a good sign in a test suite.
|
@abersheeran suggestions to solve this flaky test? |
|
Very rare, Run it many times before encountering. It looks like the restart of the process is delayed?
|
|
Ah, does the unit test code have to be statistically covered too? |
I think using while to check is not a good idea in unit tests. To be honest, I think it would be better to just kill all processes in the tree externally in case of weird scheduling errors. |
|
@Kludex I think the issue is resolved in the latest commit. |
- Add retry mechanism for unstable test_multiprocess_health_check- Ensure server is running before killing the process - Add assertion to check process is alive before killing it
|
🤦 This use case seems very unstable under Windows, and perhaps increasing the retry count is the correct way to suppress it. |
- Add a `with_retry` decorator in tests/utils.py to handle retries - Apply the decorator to the `test_multiprocess_health_check` function - Remove the ad-hoc retry logic specific to `test_multiprocess_health_check`
- Add pragma nocover comment to async_wrapper and sync_wrapper functions- This change excludes the decorator logic from code coverage metrics
- Add pragma nocover comment to exclude async_wrapper return statement from coverage - This change helps to avoid unnecessary test coverage metrics
|
Using a retry decorator might be better, as I have observed that some test cases are also unstable, such as ws cases, subsequently, it can be considered to use this decorator for these unstable use cases. |
Summary
It's not entirely clear why the assertion fails here.
assert not process.is_alive()Make sure that test resources can be cleaned up anyway.
Checklist