-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
Allow successful mocking of Mutex#synchronize #1575
Allow successful mocking of Mutex#synchronize #1575
Conversation
defined?(Mutex) is always true, back to ruby 2.7 - this check was intending to avoid reassigning the constant if it already exists, but accidentally skipped assigning it at all.
Rather than relying on the order of namespace-searching. It'll be clearer to future travellers what's going on here this 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.
The ‘unless defined?(Mutex)’ suggests that if it happened that Mutex was somehow loaded before rspec-mocks, then we consider that we don’t need to load rspec-support’s stashed mutex.
I find your explanation interesting, but I couldn’t really follow. Could you explain in more detail?
In any case, thanks for the fix!
The failures seem to be coming from a warning issued on proxy.rb:14 |
Well, I think either change would suffice, but I'm not confident I understand how 'stashing' is supposed to behave. Is the intent to make it so that the code in this library uses The explanation though! Basically (if I'm reading the code right myself), that In fact, it's pretty illustrative - if you swap the mocking of |
Exactly.
Ha! Indeed. I must admit that ‘ruby -e “puts defined?(Mutex)”’ prints “true”, so my assumption is that the stashing didn’t work properly. If I change Mutex.new to ::Mutex.new, no specs fail. |
I also find it unnecessary to conditionally require the mutex from rspec-support as we do this unconditionally from message_expectation.rb. And we do refer to it as Support::Mutex in MessageExpectation. |
Hmm, the warning being issued is that, after calling |
Instead of the existing approach, follow the pattern used by MessageExpectation - do the Support-require at the top, and jus reference Support::Mutex directly when instantiating instead of assigning the constant
You're right, that other usage is completely analogous, simpler, and resolves the CI failures (locally at least). |
We also require rspec-support’s proxy in lib/rspec/mocks unconditionally. But the failing library_wide_checks spec seems to skip liading any other gems, and their use on the class level results in a failure. I don’t agree or disagree with that, I suggest to avoid breaking its peace especially since for us those solutions are equivalent. |
Okay, this looks to be passing (on everything but ruby-head, which is also failing rspec-mocks/main atm). I still suspect the spec belongs somewhere more specific, if anyone has guidance about that, but I think it's otherwise good to review now :-) |
The code that actually "stashes" |
…-synchronize Allow successful mocking of Mutex#synchronize
Released in 3.13.1 |
Thank you, @nevinera ! |
Ah, definitely looks like a load-order issue that I just side-stepped then. Thanks, that'll be a good pattern to understand for later :-) |
Fixes the issue described in #1574, which might have other presentations.
Fundamentally, it appears that the Mutex used by
RSpec::Mocks::Proxy
is not necessarily using the 'stashed' originalMutex
implementation. I believe that this problem will happen only when the first mock attempted is a mock of the Mutex class, as that's when the Proxy instance gets created with its@messages_received_mutex
instance (though I'm unsure how often Proxy is recreated).This PR should fix #1574, but I'm not confident that the spec I've included is appropriate, or in the right place.