-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve Ctrl-C handling of spec #5719
Improve Ctrl-C handling of spec #5719
Conversation
Spec::RootContext.print_results(elapsed_time) | ||
exit 1 unless Spec::RootContext.succeeded | ||
Spec::RootContext.print_results(elapsed_time, @@aborted) | ||
exit 1 unless Spec::RootContext.succeeded && !@@aborted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the expanded condition, I would rather use if !Spec::RootContext.succeeded || @aborted
here. That logic is easier to follow.
I would print Aborted! in yellow because it's not necessarily an error. And the summary line should use the regular color coding (green if no spec failed). |
To fix the exit status, I think #5413 should do the trick. |
Actually the finish handler does not run on this code: require "spec"
at_exit { exit }
puts "start"
it "never ends" do
loop { sleep 1 }
end I believe #5413 fixes this. |
@straight-shoota I can accept that Abort! prints with yellow (but I don't think yellow is better than red, so I'll keep it for now.) However I disagree about the latter part. A success of specs means that all specs are succeeded. |
Success means that all the specs that were run passed. You can limit the specs to run so spec or spec group using the command line arguments. According to your logic, these shouldn't ever be green either because not all specs are run. |
Either way, it would be great if the summary also showed the number of specs that were not run due to aborting the run. |
@straight-shoota I wrote all expected specs at first but it was bother to explain what is expected spec, so I reworded this to all specs. However it is small thing. At least one spec is not succeeded on pressed Ctrl-C in many cases. require "spec"
it "heavy_process" do
puts "begin"
heavy_process!
puts "end"
end On the above example, you press Ctrl-C between printing "begin" and "end", then the spec of I assume a spec is interrupted on Ctrl-C always. Of course it is not true, and it is better if the
It sounds good! But unfortunately, it is impossible for the current implementation because it cannot get the number of all specs before finished. |
I don't get it why aborting the run while a spec is being executed should pose a problem. Just treat that spec as aborted like all the following ones. Doesn't matter if it has already begun. Yeah, I see, that list is gonna require more effort and that's really too much for this PR. |
This covers the original issue and good to merge as it's 👍 |
Against this code:
Before this PR:
After:
I think the spec process should be exited with non-zero status on Ctrl-C.