search

thoughtbot / cocaine

A small library for doing (command) lines.
https://robots.thoughtbot.com
Other
785 stars 55 forks source link

Ensuring exit status is always set, even when $? isn't #60

Closed bitgangsta closed 10 years ago

bitgangsta commented 10 years ago

Hi,

For an app I'm building ontop of cocaine, I'd like to be able to use the FakeRunner for testing in certain circumstances (for instance to log what commands will be run w/o actually running them). However, when I attempt to, the $? object is nil, and thus I get the following error:

/Users/drj/.rvm/gems/ruby-2.0.0-p247/gems/cocaine-0.5.3/lib/cocaine/command_line.rb:91:in `run': Command '<my command>' returned . Expected 0 (Cocaine::ExitStatusError)

It would seem that setting @exit_status conditionally, as you were, is unsafe since it is possible it will not be set. I have made the following (very small) change to set it to 0 when $? is unavailable. This means when using the FakeRunner (or if for some reason $? is not supported by the given runner), the exit status checking won't throw a fit.

jyurek commented 10 years ago

Hi, can you add a test case for this?

bitgangsta commented 10 years ago

@jyurek Sure I'm happy to add a test case. Can you think of a sure way to have $? return nil? For other test cases (where it is to return a return value), I see you guys have a small ruby script return the appropriate value. But for a nil value... my concern is: If state is left around from other tests $? could be set to something. And it can't be set directly, since Ruby doesn't like that. There might be some way to hook into the language and do it, but I'd have to research... Curious if you have any suggestions. It doesn't feel like this is an easy test to write (the thought crossed my mind).

jyurek commented 10 years ago

That's a good point. One way would be to extract the ternary you added into a new method, which you could stub for the purpose of testing. That might even be the only good way, actually.

jyurek commented 10 years ago

Sorry about the delay. I pulled this in. The test isn't appreciably different and, as we discussed, it's difficult to test because of how Ruby works. Thanks!