Returns exit 0 when a required argument is not provided

[msaffitz]

Simple example

#!/usr/bin/env ruby
require 'thor'

class Foo < Thor

  desc "foo", ""
  method_option :output, required: true
  def foo
    puts options[:output]
  end

end

Foo.start

$ ./foo.rb foo -output=blah                                                                                                                               [1s] 
No value provided for required options '--output'
$ echo $?                                                                                                                                                 [0s] 
0

I'd expect this to return an error exit code

[barttenbrinke]

This seems to be by design in thor - base.rb:

      rescue Thor::Error => e
        ENV["THOR_DEBUG"] == "1" ? (raise e) : config[:shell].error(e.message)
        exit(1) if exit_on_failure?

And futher down:

        def exit_on_failure?
          false
        end

[nbibler]

I believe this is also biting me. I'm using Thor in some Capistrano deployment scripts and when they fail, Capistrano is not exiting due to the 0 exit code... Even if this is by design, it should be documented somewhere and made clear the steps to produce an exit(1) event.

[nbibler]

I guess the current, correct approach is similar to:

class MyStuff < Thor
  def self.exit_on_failure?
    true
  end

  desc "epic_failure", "failure is epic"
  def epic_failure
    raise Thor::Error, "E-E-E-EPIC-C-C-c-c-c-c-.-.-."
  end
end

$ thor my_stuff:epic_failure
E-E-E-EPIC-C-C-c-c-c-c-.-.-.
$ echo $?
1

I think it just needs to be more clearly documented somewhere...

[leehambley]

I raised a similar issue directly with Bundler, who've referred me here. It's as simple as if you can't do what I ask, don't exit 0 – anything else is a bastardisation of 30 years of software sane practice.

rubygems/bundler#1892 (comment)

[b-dean]

So I know this issue was closed 5 months ago but I just wanted to mention that as a person writing command line applications it seems really weird to me that the default behavior is to exit with a 0 exit status on errors. That is so backwards.

I think the exit_on_failure? method is fine but I think it should return true by default. In fact, we monkey patch Thor::Base::ClassMethods so exit_on_failure? returns true for every CLI we make. That way if some weird corner case needs to raise an error and not exit with an error status it can have exit_on_failure? return true, but it works like normal CLI applications for every other case.

[ab]

This was very surprising to me as well. Is there some case where exiting with status 0 on errors is desirable? It doesn't seem like a good default.

Thanks b-dean for the monkey patch suggestion.

[mikz]

Maybe because of testing code? Do you test the apps, right? :)
Any error in thor will turn off your tests.

[ab]

It seems to me that you should explicitly opt in to that behavior if you really want it in testing. Personally I would test the library functionality in my app separately from the CLI. And if you want to test the CLI, you should probably be using a subprocess. Or use ThorClass#invoke directly.

[b-dean]

@mikz, It's not just a matter of whether or not we test our CLI apps. If the CLI app talks to some external system and something unexpected happens (or even a known problem) and the CLI app throws an exception, it does not make any sense to me why the default for Thor is to have this exit with a success status. Something bad happened, it should give an appropriate exit status otherwise it's a bad CLI app. Who knows the exception could even be some rare bug in Ruby itself, or some operating system level error (permissions, out of memory, etc). Those also shouldn't be 0 exit statuses (IMHO).

[mikz]

Definitely CLI apps should respond with non success error codes on failures. No dispute over that.

Thor allows you to set the exit_on_failure? in the binary and keep the .rb files raising exceptions for testing.

Thats the best of the both worlds, binary exits > 0 on errors and testing code keeps raising exceptions.

[b-dean]

I'm having a hard time understanding how having the default for exit_on_failure? being true would mess up your tests. You had said "Any error in thor will turn off your tests." but I don't really know what you mean by that. If you're testing with cucumber (maybe with aruba) and running the CLI or running thor commands, you could check that the exit code is non zero when you want to test for exceptions. If you're using rspec, you could have expect{thing.invoke(args)}.to raise_error SystemExit Or better yet, use cucumber/aruba to test the behavior of the CLI by running the CLI, and use rspec to test classes used by the CLI (that are normal ruby classes, not subclasses of Thor) and deal with the exceptions how you normally would.

My main point, is that a library that whose expressed purpose is for writing CLI applications (see the first sentence on http://whatisthor.com/), it is very strange for the default behavior (swallowing errors and always returning 0) to be breaking with the convention set by all CLI applications since the beginning of time (non-zero exit codes for failure).

[mikz]

Because aruba & cucumber are for integration testing.
If you have unexpected error in your unit tests and Thor would exit, your tests would quit too.

We agree that there should be a switch to turn it on/off. Something like when you run it with .start it will exit (because thats what you do in bin), but when instantiating them normally, it would not quit.

[leehambley]

Because aruba & cucumber are for integration testing.
If you have unexpected error in your unit tests and Thor would exit, your tests would quit too.

That rather implies that Aruba is broken, since testing failure conditions is arguably one of the more important sides of testing.

For what it's worth in pretty much every long lived POSIX compliant program, the use of specific error codes, or code ranges (not unlike HTTP) is a critical part of reliable scripting.

Thanks for making your case for Thor's (provably broken) behaviour. Correct error handling shouldn't be an option.

[b-dean]

@leehambley, I don't think @mikz was saying cucumber/aruba will quit when thor has an error. He was saying that "unit testing" would quit, where unit testing is something like rspec, test unit, or any number of other unit test frameworks. Cucumber is specifically not a unit test framework. Also using aruba and cucumber, you absolutely can test for failure conditions:

When I run `my_cli with some bad args`
Then the exit status should be 42

The reason I proposed cucumber/aruba for this problem is that I believe when you are testing the CLI, you are doing integration testing because the user will be using the CLI as their user interface. That's why cucumber exists. If you use cucumber for testing the CLI, you can still use rspec or whatever else to test the underlying classes that your CLI uses.

But maybe your CLI class is huge and all the logic for your program is right inside your Thor subclass. (That could be tricky to test anyway but nevermind that). If you insist on using unit testing frameworks to test your Thor subclasses, you could always use a mock framework to change what exit_on_failure? returns:

Before(:each) do
  MyCli.stub(:exit_on_failure?).and_return(false)
end

Then it won't exit on failure. But you can also do what I mentioned earlier and have your tests expect a SystemExit error when you were expecting your cli to raise some exception.

My point is: there are a lot of options for solving the testing problems, but having a framework whose whole purpose is for quickly and easily making CLI applications have the default behavior of exiting with 0 when there are errors is horrible.