Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default exit code behavior to match norm #521

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dkniffin
Copy link

Fixes #244

@rafaelfranca
Copy link
Member

As you can see it breaks the thor test suite. We need to make sure everything passes.

@teaforthecat
Copy link

Dang, it looks like this is failing due to a 500 response from coveralls.io . All the tests actually pass. I wonder if the run could be retried, or maybe this branch could be rebased, which would retry the run.

@rafaelfranca
Copy link
Member

No, it is not failing because of coveralls.io. There is a legit test failing:

1) Thor::Group#invoke_from_option with default type allows to invoke a class from the class binding by the given option
     Failure/Error: exit(1) if exit_on_failure?
     
     SystemExit:
       exit
     # ./spec/../lib/thor/base.rb:443:in `exit'
     # ./spec/../lib/thor/base.rb:443:in `start'
     # ./spec/group_spec.rb:132
     # ./spec/helper.rb:59:in `capture'
     # ./spec/group_spec.rb:131
     # ./spec/helper.rb:59:in `capture'
     # ./spec/group_spec.rb:130

@teaforthecat
Copy link

Oh, sorry, it is just on the 1.8.7 build, I see.

@dkniffin
Copy link
Author

I'm sorry I made this PR and just abandoned it. I'm going to work on fixing these tests now.

@dkniffin
Copy link
Author

Alright, we've got a green build. @rafaelfranca can you take another look at this?

Also, I just noticed #578. I'm not sure if/how this relates to that change. Maybe @jmax315 can elaborate

@jmax315
Copy link
Contributor

jmax315 commented Jan 27, 2018

It looks like, on casual inspection, we both fixed the same two problems, although we came at it from two different symptoms. It may still be worth pulling in my specs though (spec/fixtures/exit_status.thor and spec/script_exit_status.rb), as they directly address the symptom that started me off.
Merged the current main head into my fork, and my specs pass, but two unrelated specs fail (both in spec/actions/file_manipulation_spec.rb; lines 12 and 86). I'll look into what's going on here later and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants