remove reqwest unit tests, dev-dependency#43
Merged
complexspaces merged 1 commit intorustls:mainfrom Dec 21, 2023
Merged
Conversation
Previously the `verification` mod had a unit test, `can_verify_server_cert`, that ensured using the platform verifier with `reqwest` could be done without error. The reason for this was that the `reqwest` API consumes custom verifiers to use with a Rustls client config as `&dyn Any` inputs, and then downcasts at runtime to the required `Arc<dyn ServerCertVerifier>` - this means that if `rustls-platform-verifier` uses a different Rustls version than `reqwest` a runtime panic would occur. However, having this unit test in place means we can't update `rustls-platform-verifier` to a new Rustls release until the `reqwest` ecosystem first updates. This is suboptimal, as `reqwest` itself has many dependencies that need similar updates. This commit removes the unit test. Ensuring the Rustls versions match should be handled by downstream consumers that have chosen to use `reqwest`. There are other libraries one might use `rustls-platform-verifier` with, and we shouldn't block useful updates to this crate on `reqwest`. In general one already has to be careful about mixing/matching Rustls versions across dependencies, the fact that `reqwest` makes this a runtime concern is unfortunate, but not a great reason to avoid keeping this crate in sync with the rest of the Rustls ecosystem.
djc
approved these changes
Dec 19, 2023
Member
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
It's not great but I guess it's an improvement.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously the
verificationmod had a unit test,can_verify_server_cert, that ensured using the platform verifier withreqwestcould be done without error.The reason for this was that the
reqwestAPI consumes custom verifiers to use with a Rustls client config as&dyn Anyinputs, and then downcasts at runtime to the requiredArc<dyn ServerCertVerifier>- this means that ifrustls-platform-verifieruses a different Rustls version thanreqwesta runtime panic would occur.However, having this unit test in place means we can't update
rustls-platform-verifierto a new Rustls release until thereqwestecosystem first updates. This is suboptimal, asreqwestitself has many dependencies that need similar updates.This commit removes the unit test. Ensuring the Rustls versions match should be handled by downstream consumers that have chosen to use
reqwest. There are other libraries one might userustls-platform-verifierwith, and we shouldn't block useful updates to this crate onreqwest. In general one already has to be careful about mixing/matching Rustls versions across dependencies, the fact thatreqwestmakes this a runtime concern is unfortunate, but not a great reason to avoid keeping this crate in sync with the rest of the 1st-party Rustls ecosystem.