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

Use __send__ instead of send #3045

Closed
wants to merge 1 commit into from

Conversation

richardplatel
Copy link
Contributor

RSpec::Core::OutputWrapper uses send to call class methods on IO objects, but some IO objects (e.g. TCPSocket) redefine send. Use the equivalent __send__ method instead.

https://apidock.com/ruby/BasicObject/__send__

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Can you add a spec that would fail with send, but would work with send?

@@ -9,6 +9,11 @@ module RSpec::Core
expect(output.string).to eq("message\nanother message").and eq(wrapper.string)
end

it 'proxies IO instance methods with __send__ to be compatiable with IOs that redefine send' do
expect(output).to receive(:__send__).with(:puts, 'message')
Copy link
Member

Choose a reason for hiding this comment

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

Please accept my apologies for not making myself clear.
Would it be possible to add a real-world case here that would fail with send but would work with __send__? TCPSocket or others?

Copy link
Contributor Author

@richardplatel richardplatel Jul 27, 2023

Choose a reason for hiding this comment

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

Thanks for looking at my PR @pirj!

How about this test?

@richardplatel richardplatel force-pushed the safer-method-send branch 3 times, most recently from e4e4591 to 31220b3 Compare July 27, 2023 14:04
@richardplatel
Copy link
Contributor Author

richardplatel commented Aug 1, 2023

@pirj Could use some help here figuring out how to write this test.

Seems like making a real TCPSocket is not ideal but I'm not sure how to mock it in a way that still tests the change.

Or maybe there another approach?

JonRowe added a commit that referenced this pull request Aug 5, 2023
JonRowe added a commit that referenced this pull request Aug 5, 2023
@JonRowe
Copy link
Member

JonRowe commented Aug 5, 2023

Thanks for spotting this, I've merged your commit with an updated spec. (0ff6358)

@JonRowe JonRowe closed this Aug 5, 2023
@JonRowe
Copy link
Member

JonRowe commented Feb 4, 2024

This has been released in 3.12.3, sorry about the delay

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.

3 participants