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

Make the StubSocket slightly more believable #988

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

rzane
Copy link
Contributor

@rzane rzane commented Aug 5, 2022

Fixes #976 (comment)

  • ActiveMerchant expects #ssl_version and #cipher to be defined. Though these methods are private, it's best if webmock can try to act like the Net::HTTP to avoid gems having to detect if WebMock is enabled.
  • I also took the opportunity to make StubSocket#close and StubSocket#closed? a bit more realistic as well.
  • I also noticed that Net::HTTP#ipaddr was broken because StubSocket#peeraddr wasn't defined.

@bblimke
Copy link
Owner

bblimke commented Aug 5, 2022

Thank you @rzane . Awesome!

@bblimke bblimke merged commit fc798ea into bblimke:master Aug 5, 2022
@rzane rzane deleted the socket-methods branch August 5, 2022 20:51
do_start if @socket.is_a?(StubSocket)
if @socket.is_a?(StubSocket)
@socket&.close
@socket = nil

Choose a reason for hiding this comment

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

Why are these necessary? How is it better simulating net/http?

It causes issues when running tests with tools like https://github.com/shopify/toxiproxy where the test involves starting a successful HTTP connection to a local server, and then mimics that server failing using toxiproxy, and attempting another request. The exception raised is "undefined method `closed?' for nil", which is coming from https://github.com/ruby/net-http/blob/d39f1e35da0094759356a49a3c96fea84325c388/lib/net/http.rb#L2431.

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.

None yet

3 participants