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

#11702 move some signal code #11703

Merged
merged 7 commits into from Oct 7, 2022

Conversation

exarkun
Copy link
Member

@exarkun exarkun commented Oct 5, 2022

Scope and purpose

Fixes #11702

This was glyph's idea while looking at #11685 which refactored a lot of code but left all of the signal-handling pieces inside base.py and posixbase.py. I pulled this code movement out because the branch for #11685 is over 1000 lines of diff including these changes.

This is intended to be even less than a refactoring. It's just mechanical movement of source code from one place to another.

Diff coverage here is not great but all of the uncovered code was also uncovered in its original location and all this does is move it.

@exarkun exarkun marked this pull request as ready for review October 5, 2022 19:41
@chevah-robot chevah-robot requested a review from a team October 5, 2022 19:41
Copy link
Contributor

@twm twm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code movement and minor changes look reasonable to me. Definitely organizational improvement. Thanks for splitting this out!

From the coverage results I guess we aren't running any CFReactor tests in CI? That seems bad, but oh well, Apple makes it such a pain to their users' detriment.

@adiroiban
Copy link
Member

on macOS we only run 3.10-macos-12-default-tests , with the default reactor on macOS. I don't know which reactor is the default on macOS... I guess is the posix select.

I guess this is ok for now. If somebody cares about macOS, the CI infrastructure is ready to run the tests with a non-default reactor.
We do it for Windows, for which the tests are executed on both select and iocp.

Thanks Jean-Paul for the cleanup. Thanks Tom for the review.

@exarkun
Copy link
Member Author

exarkun commented Oct 7, 2022

On the GitHub Actions macOS job, the CFReactor tests are all skipped like:

No module named 'CFNetwork'

So maybe it's just a matter of missing some dependencies.

@exarkun exarkun merged commit 8cb7594 into twisted:trunk Oct 7, 2022
@exarkun exarkun deleted the 11702-move-some-signal-code branch October 7, 2022 00:18
@adiroiban
Copy link
Member

So maybe it's just a matter of missing some dependencies.

Thanks. True. Those are the cfreactor targeted tests... and that would be a first step.

At least for windows, we also try to run the whole tests, not only the iocp tests, using the iocp reactor. For a better coverage, and in the case in which the targeted iocp have missing coverage.

@exarkun
Copy link
Member Author

exarkun commented Oct 7, 2022

I filed #11708

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