-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
don't print the duration in quiet mode #822
don't print the duration in quiet mode #822
Conversation
@mattwynne does this do it for the Tcl situation or do we need to be able to remove the duration from |
@@ -85,8 +85,8 @@ def print_stats(features, options) | |||
@io.puts scenario_summary(runtime) {|status_count, status| format_string(status_count, status)} | |||
@io.puts step_summary(runtime) {|status_count, status| format_string(status_count, status)} | |||
|
|||
@io.puts(format_duration(features.duration)) if features && features.duration | |||
|
|||
@io.puts(format_duration(features.duration)) if features && features.duration && options[:duration] |
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.
I wonder if we still need that features.duration
check. Could it ever be nil? Looks quite defensive.
LGTM @richarda. How come you can't remove the hack? |
@mattwynne in the current state of this branch, removing the line that substitutes a fixed duration of 0m0.012 leaves a handful of scenarios failing with what seems to be their actual duration. I was just still thinking what might be a good way to address that. The failing scenarios are using These are are the tests that break without that substitution:
|
You're right actually, it seems nonsensical to get the duration of a dry-run, so I think it would be OK for that to imply quiet. |
Can you review the other ones to see whether it would make sense just to add |
Yes. I'll review those scenarios and take a look at dry-run. |
@mattwynne You said yourself that If you really want to remove "the hack", then a |
Sounds like @brasmusson has thought about this more than me, and we should have a separate |
@mattwynne when I ran the build locally against the latest |
I'm good with |
921084a
to
7df7251
Compare
Has it really been since March?! I've been too busy. Thanks for your contribution @richarda, sorry to have stalled on this for so long. |
Ha! No problem. I was just here looking at open issues again too. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This attempts to resolve #816
I'm not able to remove the referenced hack just yet, but I'll keep working on it.