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.
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
Fix stubbing prepended only methods #1218
Fix stubbing prepended only methods #1218
Changes from all commits
604dc95
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we could remove all the checks in
restore_original_visibility
when this is only called when@method_stasher.method_is_stashed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it affect the performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's memoized so I don't think we'll see anything noticeable in practice. If we disregard memoization and run simple benchmark,
object.method(method_name).owner
is about 3 times faster:From running this:
Well, or we can remove the test and calling out that do not override
method
, which I fully support :PThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm clearly on your side with you that one shouldn't override
Kernel
's internals with unrelated application-level implementations. However, someone's code may break with a weird message, and they will open a ticket, and we'll have to address it, since even though this might not be a good style to overridemethod
, it's not our call to enforce that in any way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, of course, that's why I was half joking there :)
Half joking, half serious, because there are a lot of other methods we're relying on, and of course we won't want to be so defensive to do it this way for most of the built-in methods? (or a rule of thumb is, whenever someone is filing a ticket 🤷 This is also relying on someone isn't trying to break RSpec intentionally)
I see
method
orsend
is more commonly overridden with a completely different behaviour (if it's the same behaviour like some people will overridemethod_missing
also withrespond_to_missing?
properly, that would be mostly alright, but issues might still happen especially for mocks implementation which often talks about proxies), and that's why we have__send__
we can use, sadly not formethod
and it may be confusing that__method__
is actually a completely different method! Not like__send__
is an alias ofsend
.Sorry that this is probably falling down to a rant. To be more on the topic, since the test is already there, I don't think a regression is acceptable. However if we never added that test, I would think it's fine to just say "don't override
method
". Maybe RSpec 5, or maybe it doesn't matter after all. It's just one method right now. We don't have an army ofObject.instance_method(method_name).bind(object)
yet. Or do we? Maybe it's inside one of the support libraries already? 😅 To be honest I won't be surprised!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We defend against a number of quite odd things:
rspec/rspec-core#2886, rspec/rspec-support#431 and in a few other places.
Good point, it is! 😄
https://github.com/rspec/rspec-support/blob/d63133f478408c1d965e673b96ad10ef5a5d183f/lib/rspec/support.rb#L53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my, I was guilty here. I am sure I was once lazy and stubbed
File.read
without explicit about the arguments, and it caused some issues. However being explicit about a specific argument did the trick for me.I wonder if this kinds of things will need a more general approach. For example, will this work?
Ok, thank you for spotting that. I updated the code to use it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can think of three cases we've touched:
File.read
,File.expand_path
,Mutex.new
)Kernel.method
)Example
(to_s
,initialize
)Not sure if there could be a common solution to all of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried this does work:
We can have a list of preserved classes.
We can always do it via the same trick.
Edited: Fixed above
send_kernel
usage.For
to_s
andinitialize
I think we should just raise an error like now, indeed. It would be too much work for those to be overridden. Even if we can stick with__to_s__
or__initialize__
and play around withallocate
, it's easily broken with other integration.I didn't mean a common solution to all of above. I meant a common solution for the same issue. For example, we use the same way to resolve
File.read
orMutex.new
(a module method) got overridden. We use the same way to resolve a particular instance method got overridden.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our policy is to do the method work around for cases as they occur
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test can catch the need for
!Mocks.space.any_instance_recorder_for(owner, true)
in the other condition. It was discovered in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44417#note_429658296 thanks to @engwan !