Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove signal testing order dependency #2382

Merged
merged 5 commits into from
Aug 11, 2024

Conversation

maxfischer2781
Copy link
Contributor

Summary

This PR adds explicit management to signal handling for signal-tests.

  • Add an assert_signal utility to test that a signal was received in a block. The utility sets-and-resets a custom signal handler to work the same regardless of the previous signal handler state.
  • Use the helper on supervisors/test_signal.py. These tests were discovered to be order dependent with other tests (see Cooperative signal handling #1600 (comment) and Cooperative signal handling #1600 (comment)), specifically the other supervisor/* tests that set but do not reset signal handlers.

Notes

The changes do not touch on the --timeout-graceful-shutdown behaviour also noted in #1600 (comment). I haven't figured out whether these are actually related, but the changes make test_signal.py reliable again for diagnosing the error.

The utility is not used in test_server.py as those tests need control of the specific signal handler type, namely also using asyncio's add_signal_handler.

The utility is not used in the other supervisor/* tests as these test the actual signal handlers, not just receiving a signal.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
    • There are no changes to documented behaviour.

@maxfischer2781 maxfischer2781 changed the title Remove ordering dependency Remove signal testing order dependency Jul 5, 2024
Kludex
Kludex previously approved these changes Jul 20, 2024
@Kludex Kludex self-assigned this Jul 20, 2024
@Kludex
Copy link
Member

Kludex commented Jul 20, 2024

@maxfischer2781 I don't think this PR solves the issue.

The same lines are still uncovered with this PR: 63, 93.

tests/supervisors/test_signal.py                     54      2  96.30%   63, 93

@Kludex Kludex dismissed their stale review July 20, 2024 08:38

The same test lines are still uncovered.

@Kludex Kludex assigned maxfischer2781 and unassigned Kludex Jul 20, 2024
@Kludex
Copy link
Member

Kludex commented Jul 20, 2024

I'll merge #2394 which will add pragmas on the lines I mentioned, but I don't want to have those pragmas. My goal is just to reach 100% so this kind of issue doesn't happen again.

@maxfischer2781
Copy link
Contributor Author

maxfischer2781 commented Jul 20, 2024

Doesn't fix what issue on what lines? This PR is only about the order dependency of the test due to signal handling, not what the test actually does.

FWIW, looking at the test code I have no idea how 63, 93 could possibly ever run (i.e. why they are even part of the test). One is after explicitly awaiting an unset Event (i.e. has to wait indefinitely before running), and the other is "run" as part of a server that is killed off before running anything (i.e. is expected never to run); I'm rather surprised that 92 is actually run, to be honest.

@Kludex Kludex enabled auto-merge (squash) August 11, 2024 07:12
@Kludex Kludex merged commit 0f513d2 into encode:master Aug 11, 2024
15 checks passed
@Kludex
Copy link
Member

Kludex commented Aug 11, 2024

I think this was flaky. I just merged on master and failed there: https://github.com/encode/uvicorn/actions/runs/10337934097/job/28615369649

@maxfischer2781
Copy link
Contributor Author

Feel free to revert the merge if it’s causing problems.

@Kludex
Copy link
Member

Kludex commented Aug 13, 2024

I think @vvanglro PR fixed? 👀

@vvanglro
Copy link
Contributor

We can be watching to see if it will be again. There's no need to revert, this PR is improving the quality of the code as it didn't assert signal before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants