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.

Sign up for GitHub

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

Jump to bottom

Resolve TODO about Mac OS failing tests #268

Closed
wants to merge 1 commit into from
Closed

Resolve TODO about Mac OS failing tests #268

wants to merge 1 commit into from

Conversation

kevinkjt2000
Copy link
Contributor

This actually applied to multiple operating systems, but required using a DNS server that answers to queries about localhost. Responding to those with anything but NXDOMAIN violates the DNS RFC, so it's surprising to see this be so rampant out in the wild. Anyway, the solution is to use getaddrinfo to let the OS stack handle resolution of names.

@kevinkjt2000 kevinkjt2000 requested a review from ItsDrike as a code owner March 27, 2022 21:26
ItsDrike
ItsDrike requested changes Mar 27, 2022
View reviewed changes
Copy link
Member

@ItsDrike ItsDrike left a comment

Choose a reason for hiding this comment

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

The Address.resolve_ip function and the async counterpart can now probably be removed completely, since they were only really relevant for UDP connections which required an IP, but that's no longer the case here.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
mcstatus/protocol/connection.py Outdated Show resolved Hide resolved
mcstatus/protocol/connection.py Outdated Show resolved Hide resolved
@kevinkjt2000
Copy link
Contributor Author

The Address.resolve_ip function and the async counterpart can now probably be removed completely, since they were only really relevant for UDP connections which required an IP, but that's no longer the case here.

I had the same idea. Seeing as how we just released those, maybe it's not too late to take those out from the public API without any deprecation period.

@ItsDrike ItsDrike added type: bug Something isn't working area: API Related to core API of the project state: waiting for author Waiting for author to address a review or respond to a comment labels Mar 28, 2022
@ItsDrike
Copy link
Member

Any updates on this?

@kevinkjt2000 kevinkjt2000 marked this pull request as draft June 22, 2022 23:53
@kevinkjt2000
Copy link
Contributor Author

Thanks to 21fb8ce, able to remove these TODO comments.

@PerchunPak
Copy link
Member

If it's done, could you please make it ready for review?

@kevinkjt2000
Copy link
Contributor Author

If it's done, could you please make it ready for review?

Mac OS tests still failed in CI, and I haven't looked at why yet.

@CoolCat467
Copy link
Contributor

The ConnectionResetError is interesting. The exact same thing happened during tox-test (macos-latest, 3.8) in PR #273, but when I triggered it again, everything passed. This might be something to look deeper into, because it seems like something similar happened on Windows from time to time in the past. Original find: #140

Failed run:
https://github.com/py-mine/mcstatus/runs/7371481344?check_suite_focus=true#step:6:275

Success run:
https://github.com/py-mine/mcstatus/runs/7371820372?check_suite_focus=true#step:6:141

Change between runs:
3dcd296

@ItsDrike ItsDrike added the status: stale Has had no activity for a while label Oct 13, 2022
@kevinkjt2000
Copy link
Contributor Author

Closing this stale work

@kevinkjt2000 kevinkjt2000 deleted the fix-mac-os-tests branch February 20, 2023 15:52
@PerchunPak PerchunPak added marker: up for grabs Available for anyone to take over and work on and removed state: waiting for author Waiting for author to address a review or respond to a comment labels May 11, 2023
@PerchunPak PerchunPak mentioned this pull request Aug 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsDrike ItsDrike ItsDrike requested changes

Assignees
No one assigned
Labels
area: API Related to core API of the project marker: up for grabs Available for anyone to take over and work on status: stale Has had no activity for a while type: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevinkjt2000 @ItsDrike @PerchunPak @CoolCat467