Log a warning when methods are reset on a frozen object #496
Conversation
| @@ -85,6 +85,9 @@ def restore_original_method | |||
| restore_original_visibility | |||
|
|
|||
| @method_is_proxied = false | |||
| rescue => e | |||
There was a problem hiding this comment.
We should rescue the specific exception here? It'll be at least a RunTimeError...
|
Thanks for tackling this :) |
| context "on a frozen object" do | ||
| it "removes the method double" do | ||
| target.to receive(:foo).and_return(:baz) | ||
| expect_warning_without_call_site(/Unable to remove stub method foo because the object was frozen./).at_least(:once) |
There was a problem hiding this comment.
The test passes, but the warning is still issued to $stdout. Is that a bug in rspec-support?
There was a problem hiding this comment.
I don't see how that's possible... o_O
There was a problem hiding this comment.
Also the at_least(:once) call should be unnecessary.
There was a problem hiding this comment.
My initial pass didn't have at_least(:once), but then it fails:
Failure/Error: expect_warning_without_call_site(/Unable to remove stub method foo because the object was frozen./)
(Kernel).warn(any args)
expected: 1 time with any arguments
received: 2 times with any arguments
There was a problem hiding this comment.
Here's what the output looks like showing the messages:
All examples were filtered out; ignoring {:focus=>true}
..................................................................................................................................................................................................................................................................................................................................*.................*.............................................................................................................................................................................................................................................................................................*.*.......................Unable to remove stub method foo because the object was frozen.
.............Unable to remove stub method foo because the object was frozen.
......................................................*.......................................................................................................................................................................................................................................................................................................................................................................
There was a problem hiding this comment.
It could of course be happening 3 times...
There was a problem hiding this comment.
It looks like I can remove the at_least(:once) if I also remove the reset object, but then the test becomes less clear. Alternately, I can change the order of the expectations are declared, and then the at_least(:once) isn't necessary.
There was a problem hiding this comment.
clean up the mock manually so it doesn't need to be cleaned up outside the test
There was a problem hiding this comment.
But we can't because it's frozen, right?
There was a problem hiding this comment.
I opened a PR to supress it jcoyne#1
|
Thanks for your help @JonRowe. I squished your PR into this one. |
| @@ -85,6 +86,15 @@ def restore_original_method | |||
| restore_original_visibility | |||
|
|
|||
| @method_is_proxied = false | |||
| rescue RuntimeError => e | |||
There was a problem hiding this comment.
Ah ok, this needs to be TypeError on 1.8.7...
There was a problem hiding this comment.
I'm now wondering if it would be better to rescue StandardError and inspect the message for the word frozen... Thoughts @myronmarston, am I being too picky?
There was a problem hiding this comment.
I think checking frozen? on the singleton class (as it is now) is a less brittle approach.
There was a problem hiding this comment.
Just drop back to a bare rescue?
There was a problem hiding this comment.
Yep, that sounds fine.
One thing thats unclear to me: what purpose does @show_frozen_warnings serve? I don't see any spec showing what it does...
There was a problem hiding this comment.
Prevents the message appearing twice for the same method. We're still reseting mocks twice.
|
Closing in favour of #527 |
No description provided.