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

Do not enable seeder signal handlers in tests #16079

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Aug 16, 2023

Purpose:

There are two reasons for this. One relates to RuntimeError: set_wakeup_fd only works in main thread of the main interpreter which is triggered because tooling under pytest-xdist uses threads (sometimes) as I believe is discussed in pytest-dev/execnet#96. The other is that it messes up pytest's own handling of signals which means ctrl+c and such don't work well.

It would be great at some point to bring consistency to our signal handling. I started that awhile back in #13568 and will add a note there about taking care of the seeder as well.

Current Behavior:

New Behavior:

Testing Notes:

Daft For:

@altendky altendky added Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog Tests Changes to tests labels Aug 16, 2023
@altendky altendky mentioned this pull request Aug 16, 2023
4 tasks
@altendky altendky closed this Aug 18, 2023
@altendky altendky reopened this Aug 18, 2023
@github-actions
Copy link
Contributor

File Coverage Missing Lines
chia/seeder/dns_server.py 50.0% lines 362
Total Missing Coverage
2 lines Unknown 50%

@altendky altendky marked this pull request as ready for review August 19, 2023 14:27
@altendky altendky requested a review from a team as a code owner August 19, 2023 14:27
Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

Approved with coverage diff exception

@wallentx wallentx merged commit 538edfd into main Aug 24, 2023
388 of 390 checks passed
@wallentx wallentx deleted the dns_server_setup_process_global_state branch August 24, 2023 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog Tests Changes to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants