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

For issue 53957: revise unit test to focus on underlying bug of 23076. #70197

Merged
merged 1 commit into from
Mar 22, 2020

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Mar 20, 2020

Fix #53957 by revising unit test to focus on underlying bug of #23076.

Namely, this version focuses on the end-to-end behavior that the attempt to create the UDP binding will fail, regardless of the semantics of how particular DNS servers handle junk inputs.

(I spent some time trying to create a second more-focused test that would sidestep the DNS resolution, but this is not possible without more invasive changes to the internal infrastructure of ToSocketAddrs and what not. It is not worth it.)

Namely, this version focuses on the end-to-end behavior that the attempt to
create the UDP binding will fail, regardless of the semantics of how particular
DNS servers handle junk inputs.

(I spent some time trying to create a second more-focused test that would
sidestep the DNS resolution, but this is not possible without more invasive
changes to the internal infrastructure of `ToSocketAddrs` and what not. It is
not worth it.)
@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2020
@pnkfelix pnkfelix changed the title For issue 53957: revise unit tests to focus on underlying bug of 23076. For issue 53957: revise unit test to focus on underlying bug of 23076. Mar 20, 2020
@pnkfelix
Copy link
Member Author

(I would also be satisfied with just deleting the to_socket_addr_str_bad test without attempting to replace it. I just don't want to continue with the status quo of a test that fails for random contributors due to external factors that our tests should not care about.)

@LukasKalbertodt
Copy link
Member

It is kind of icky to have a regression test for a parser bug that contacts the user's DNS server. However, I agree that properly testing this would probably require a lot more work. And as this is certainly an improvement over the old test, let's merge this.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2020

📌 Commit 3db6d1c has been approved by LukasKalbertodt

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 21, 2020
…-23076, r=LukasKalbertodt

For issue 53957: revise unit test to focus on underlying bug of 23076.

Fix rust-lang#53957 by revising unit test to focus on underlying bug of rust-lang#23076.

Namely, this version focuses on the end-to-end behavior that the attempt to create the UDP binding will fail, regardless of the semantics of how particular DNS servers handle junk inputs.

(I spent some time trying to create a second more-focused test that would sidestep the DNS resolution, but this is not possible without more invasive changes to the internal infrastructure of `ToSocketAddrs` and what not. It is not worth it.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 21, 2020
Rollup of 10 pull requests

Successful merges:

 - rust-lang#70003 (symbol_names: treat ReifyShim like VtableShim.)
 - rust-lang#70051 (Allow `hir().find` to return `None`)
 - rust-lang#70126 (Fix ICE caused by truncating a negative ZST enum discriminant)
 - rust-lang#70197 (For issue 53957: revise unit test to focus on underlying bug of 23076.)
 - rust-lang#70215 (ast: Compress `AttrId` from `usize` to `u32`)
 - rust-lang#70218 (Fix deprecated Error.description() usage in docs)
 - rust-lang#70228 (Remove CARGO_BUILD_TARGET from bootstrap.py)
 - rust-lang#70231 (Add explanation message for E0224)
 - rust-lang#70232 (Tweak wording for std::io::Read::read function)
 - rust-lang#70238 (Add a test for out-of-line module passed through a proc macro)

Failed merges:

r? @ghost
@bors bors merged commit a3bdfc4 into rust-lang:master Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test to_socket_addr_str_bad is broken on macOS too
4 participants