Add tests for exotic rescued error capturing #774
Conversation
sambostock
force-pushed
the
exotic-error-capturing
branch
from
01c141b
to
2d723e0
Compare
language/rescue_spec.rb
Outdated
| # Standard use case | ||
| 'can capture the raised exception in a local variable' => RescueSpecs::LocalVariableCaptor, | ||
| # Exotic use cases | ||
| 'can capture the raised exception in a class variable' => RescueSpecs::ClassVariableCaptor, | ||
| 'can capture the raised exception in a constant' => RescueSpecs::ConstantCaptor, | ||
| 'can capture the raised exception in a global variable' => RescueSpecs::GlobalVariableCaptor, | ||
| 'can capture the raised exception in an instance variable' => RescueSpecs::InstanceVariableCaptor, | ||
| 'can capture the raised exception using a safely navigated setter method' => RescueSpecs::SafeNavigationSetterCaptor, | ||
| 'can capture the raised exception using a setter method' => RescueSpecs::SetterCaptor, | ||
| 'can capture the raised exception using a square brackets setter' => RescueSpecs::SquareBracketsCaptor, |
There was a problem hiding this comment.
Since most cases would be almost identical, but some required a supporting object on which to set a class/instance variable, or call a setter method, I decided to implement all cases using supporting objects with a shared API, and list them out concisely.
There was a problem hiding this comment.
In general we try to avoid dynamically-generated specs in ruby/spec.
I think a middle ground here would be to still to have 1 it per case, but share the logic with the should etc in a method, which you can define here under the describe:
def should_capture(captor)
captor.capture('some text').should == :caught # Ensure rescue body still runs
captor.captured_error.message.should == 'some text'
end
it 'can capture the raised exception in a local variable' do
should_capture(RescueSpecs::LocalVariableCaptor.new)
end
...There was a problem hiding this comment.
The common part can capture the raised exception can also be factored out with an extra describe around.
There was a problem hiding this comment.
@sambostock Could you do that change? Then I think it's ready to merge.
There was a problem hiding this comment.
@eregon I've pushed a commit that does something similar to your suggestion.
describe 'can capture the raised exception' do
it 'in a local variable' do
RescueSpecs::LocalVariableCaptor.should_capture_exception
endLet me know what you think!
|
All CI runs succeeded except Seems unrelated, so I'll just push another commit to try again. |
sambostock
force-pushed
the
exotic-error-capturing
branch
from
2d723e0
to
905f744
Compare
|
Looks like |
|
cc @eregon I've re-run a build several times and it fails either on MacOS or Windows with some infrastructure error. |
|
It looks like it passes now. |
sambostock
force-pushed
the
exotic-error-capturing
branch
from
e6c02b6
to
13db389
Compare
The idiomatic way to capture exceptions is to assign to a local variable
rescue => local_variable
However, other kinds of LVALUEs also work
rescue => $global_variable
rescue => @@class_variable
rescue => @instance_variable
rescue => Constant
rescue => receiver&.setter_method
rescue => receiver.setter_method
rescue => receiver[:key]
Some of the tests involve side effects to the global state. We can
remove the constant and class variable, but the best we can go for the
global variable is to nil it out. This is effectively identical to
removing it though, as nil is the default when accessing an undefined
global.
sambostock
force-pushed
the
exotic-error-capturing
branch
from
13db389
to
99e141f
Compare
The idiomatic way to capture exceptions is to assign to a local variable
However, other kinds of LVALUEs also work
This adds specs documenting their usage.
Closes #773
cc. @chrisseaton