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

Improve Ctrl-C handling of spec #5719

Merged

Conversation

makenowjust
Copy link
Contributor

Against this code:

require "spec"

puts "start"
it "never ends" do
  loop { sleep 1 }
end

Before this PR:

before

After:

after

I think the spec process should be exited with non-zero status on Ctrl-C.

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
Copy link
Member

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.

@straight-shoota
Copy link
Member

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).

@bew
Copy link
Contributor

bew commented Feb 13, 2018

To fix the exit status, I think #5413 should do the trick.

@makenowjust
Copy link
Contributor Author

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.

@makenowjust
Copy link
Contributor Author

makenowjust commented Feb 14, 2018

@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.

@straight-shoota
Copy link
Member

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.
I understand limiting the execution of specs beforehand and aborting the spec run manually with Ctrl+C to be very similar. I usually hit Ctrl+C when I know all the specs I'm really interested have already run and I don't want to wait for the the test to execute. Then I'd want to see a proper results summary which don't just scream red error everytime - because there's no error, aborting a spec using Ctrl+C is intentional behaviour by the user.

@straight-shoota
Copy link
Member

Either way, it would be great if the summary also showed the number of specs that were not run due to aborting the run.

@makenowjust
Copy link
Contributor Author

@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 heavy_process is interrupted by Ctrl-C. I think such cases are often appeared in the real world because a span between two specs is short, but a time running a spec is long.

I assume a spec is interrupted on Ctrl-C always. Of course it is not true, and it is better if the "spec" library handles this correctly. But it needs more changes and the benefit of this improvement is little. I'd like to keep the change of this PR small and simple.

Either way, it would be great if the summary also showed the number of specs that were not run due to aborting the run.

It sounds good! But unfortunately, it is impossible for the current implementation because it cannot get the number of all specs before finished.

@straight-shoota
Copy link
Member

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.

@sdogruyol
Copy link
Member

This covers the original issue and good to merge as it's 👍

@sdogruyol sdogruyol merged commit 68575de into crystal-lang:master Apr 13, 2018
@sdogruyol sdogruyol added this to the Next milestone Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants