Skip to content

tests: add trailing slashes to mount to match docs recommendation? #6935

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

Open
allrob23 opened this issue Apr 22, 2025 · 2 comments
Open

tests: add trailing slashes to mount to match docs recommendation? #6935

allrob23 opened this issue Apr 22, 2025 · 2 comments

Comments

@allrob23
Copy link
Contributor

allrob23 commented Apr 22, 2025

I was digging into the docs and noticed this:

The adapter will be chosen based on a longest prefix match. Be mindful prefixes such as http://localhost will also match http://localhost.other.com or http://[email protected]. It’s recommended to terminate full hostnames with a /.

link

While checking out some tests that uses the Session.mount(), I saw that a few don’t follow this recommendation. For example, in test_session_get_adapter_prefix_matching (https://github.com/psf/requests/blob/main/tests/test_requests.py#L1620):

prefix = "https://example.com"  # no trailing slash
more_specific_prefix = prefix + "/some/path"  # no trailing slash
...
s.mount(prefix, prefix_adapter)
s.mount(more_specific_prefix, more_specific_prefix_adapter)

I know that the tests work great and do their job, but adding trailing slashes (e.g., https://example.com/ and https://example.com/some/path/) would align them with the docs and make the prefixes more precise.

Here are the tests I noticed:

  • test_transport_adapter_ordering
  • test_session_get_adapter_prefix_matching
  • test_session_get_adapter_prefix_matching_mixed_case
  • test_session_get_adapter_prefix_matching_is_case_insensitive

Would it be worth opening a PR to update these to include trailing slashes? The tests would stay the same, just following the docs best practice.

@allrob23 allrob23 changed the title Adding Trailing Slashes to Mount Test Prefixes to Match Docs Recommendation tests: add trailing slashes to mount to Match Docs Recommendation Apr 22, 2025
@allrob23 allrob23 changed the title tests: add trailing slashes to mount to Match Docs Recommendation tests: add trailing slashes to mount to match docs recommendation? Apr 22, 2025
@sigmavirus24
Copy link
Contributor

The tests do not need to follow the recommendations for users as they're testing exactly what they're meant to. If you want to add a test case so the documented recommendation doesn't ever break that's fine but the logic is so fundamentally simple that I can't imagine why it would break this but not the existing tests without extreme malice

@allrob23
Copy link
Contributor Author

Thanks for the feedback @sigmavirus24. I totally get that the existing tests are doing their job.

So I opened a PR adding two simple tests: one for prefixes with a trailing / (following the docs) and one for prefixes without it (showing the warned behavior). Could you please take a look when you get a chance? Appreciate it!

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

No branches or pull requests

2 participants