Fix broken exception handler #357
Conversation
| @@ -56,5 +56,35 @@ | |||
|
|
|||
| it { expect {process.start}.to raise_error Aruba::LaunchError } | |||
| end | |||
|
|
|||
| context "with a childprocess launch error" do | |||
| let(:child) { instance_double(ChildProcess::AbstractProcess) } | |||
There was a problem hiding this comment.
Mmhh... I think we should not fully mock "Classes" we not own. Just make it an double, to an instance one.
There was a problem hiding this comment.
ChildProcess is from the child_process gem, that's why I mocked it out. (https://github.com/jarib/childprocess).
There was a problem hiding this comment.
If it's made a double, we miss out on RSpec's "method check" feature.
There was a problem hiding this comment.
I know, but don't fiddle with someone else's classes. Why not using this instead: https://relishapp.com/rspec/rspec-mocks/docs/verifying-doubles/using-a-class-double
There was a problem hiding this comment.
No, that won't work. Or I don't see how.
A class double would help if I was only mocking out ChildProcess.build. And since Aruba::Processes::SpawnProcess breaks the Law Of Demeter here (the result of .build should be wrapped by another Aruba class), I can't use a const_double for it.
I mean, I can used const_double for .build because technically it "fits" (as a method on a constant), but that's not what I'm testing. If I did, I'd be forced to use :transfer_nested_constants because everything in the tests is namespaced with ChildProcess. And that wouldn't make sense, because ChildProcess is the ONLY case here of using a class method.
As for "fiddling with someone else's classes" - that's the point of using instance_double in the first place - to make the implementation based on the interface (of the 3rd party class) while being independent from the behavior. Thus, "mocking".
It may seem debatable whether using ChildProcess::AbstractProcess is a good idea, but I think it's the best option anyway. Even if I used ChildProcess::Windows::Process instead (if it was available without requiring implicitly), it would still be the exact same interface (due to inheritance). And because of inheritance, ChildProcess::AbstractProcess is the public API of childprocess anyway, so that won't change without a major bump.
All I really wanted to do is mock out the :start method, but due to the way the class is setup (and not wrapped by another Aruba class), the way the mocks are done here just exposes a design problem already in codebase (not wrapping the 3rd party class with something easier to mock out).
I can't use object_double on the child, because then the test would depend on the signature of ChildProcess::AbstractProcess#initialize (which is a private API - the only method of the class!).
So it's either terrible mocking here, or changing the Aruba API to wrap the ChildProcess.build result.
I can use a double, but that's not any less verbose ... and ... it's would just be way of completely ignoring the API altogether.
I can't think of a better alternative here than adding a new wrapper class in Aruba.
e2
force-pushed
the
e2-fix_broken_exception_handler
branch
from
e34e26e to
097db5e
Compare
|
I've reworked it and added specs. |
| @@ -115,16 +115,13 @@ def before_run; end | |||
| def after_run; end | |||
|
|
|||
| def inspect | |||
| out = stdout(:wait_for_io => 0) + stderr(:wait_for_io => 0) | |||
| out = truncate("#{stdout(:wait_for_io => 0).inspect}", 35) | |||
There was a problem hiding this comment.
Beautiful. I like that implementation much better!
|
Please address my comments / fix ruby 1.8.7 - sorry for that - and we're good to go. |
Previously, "cmd" was passed to a different method. The reason this never came up earlier is because on Unix, if a file is not in the PATH, a Aruba::LaunchError is raised much earlier. One way this could've been triggered is: if the program file existed, but wasn't accessible (e.g. EACCES error). That way it wouldn't fail the `which` test, but would fail once it was attempted to be executed.
e2
force-pushed
the
e2-fix_broken_exception_handler
branch
from
097db5e to
fb8fc4c
Compare
|
Fixed, rebased and squashed. |
|
Can this be merged? Or is there anything else here needed? |
|
Well done! No. I'm going to merge this now. |
ghost
mentioned this pull request
Summary
An old, undefined variable (
cmd) was left over after refactoring. It was replaced with the right one (commandline).Also, a spec was added to force the code to fail on Unix.
Details
This caused failures on AppVeyor, because the exception handler crashed.
This failed only on Windows because the PATH traversing didn't throw an error earlier.
Still, the error could've occurred on Unix if the program was in the PATH (so no exception there), and yet there was an error accessing a program's file (like if the file wasn't readable).
Motivation and Context
Fixes failure on AppVeyor.
How Has This Been Tested?
Locally, RSpec.
This probably can be simulated in Cucumber scenarios by having an unreadable binary file (e.g. using
chmod -r)Types of changes
Checklist: