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

httpclient_adapter: use uri after filters when building request signature #1050

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

mattbnz
Copy link
Contributor

@mattbnz mattbnz commented Feb 13, 2024

If a filter changes the URI (e.g. to enforce a protocol or change the hostname of the request) this is currently unable to be mocked as webmock fetches the URI for the signature of the request before calling the filters!

If a filter changes the URI (e.g. to enforce a protocol or change the
hostname of the request) this is currently unable to be mocked as
webmock fetches the URI for the signature of the request before calling
the filters!
@bblimke
Copy link
Owner

bblimke commented Feb 14, 2024

@mattbnz thank you for submitting the PR. you are correct that one expects the signature to be based on the final url, after all filters applied. Would you care to add at least one test to https://github.com/bblimke/webmock/blob/master/spec/acceptance/httpclient/httpclient_spec.rb to verify that the behaviour works as expected?
Really appreciate your help on this!

@mattbnz
Copy link
Contributor Author

mattbnz commented Feb 14, 2024

@bblimke thank you for the great library and quick response!

I'm pretty new to ruby and rspec so any guidance on the desired test style is welcome - for now I just modified the expectations of the existing tests to include URI modification in the filter - seemed to me straightforward enough and simple enough in concept to the existing tests that it made sense vs creating a new spec/test overall... but if you prefer a completely separate test let me know.

Verified that the changed spec fails on master and passes in this branch.

Cheers

@bblimke
Copy link
Owner

bblimke commented Feb 19, 2024

@mattbnz thank you. I believe the provided extension of existing spec is sufficient 👍

@bblimke bblimke merged commit ea7792c into bblimke:master Feb 19, 2024
8 checks passed
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

Successfully merging this pull request may close these issues.

2 participants