Ensure ExecutionResult timing is always populated #2144
Conversation
|
|
||
| # @api private | ||
| # Ensure that finish_time and run_time have values. | ||
| def finalize_run_time(clock) |
There was a problem hiding this comment.
This could be named better. Suggestions welcome
There was a problem hiding this comment.
Remove the comment and rename ensure_timing_set?
There was a problem hiding this comment.
Better name idea! Thanks.
There does need to be some comment there, though, in order to keep 100% documentation coverage (even though it's api private). At least that's what CI tells me :)
soulcutter
force-pushed
the
abort-with-profile
branch
4 times, most recently
from
ffd5414
to
d36fc00
Compare
|
Hmm, I don't believe that CI failure relates to this set of changes. Incorporated your suggestions otherwise, @JonRowe |
| # @api private | ||
| # Populates finished_at and run_time if it has not yet been set | ||
| def ensure_timing_set(clock) | ||
| finished_at || calculate_run_time(clock.now) |
There was a problem hiding this comment.
This works, but feels like an abuse of ||. Typically || is used to return one of two values but here we don't care about the return value -- this method is a command method, not a query method, and at the call site the return value is ignored. Using || works because it short-circuits and does not evaluate the right expression if finished_at is set...but that feels like an abuse of ||. I think it'd be far more clear if written as:
calculate_run_time(clock.now) unless finished_atAfter all, the goal of this method is to calculate the run time unless finished_at is already set -- and using unless allows us to state that more directly.
There was a problem hiding this comment.
I had originally written this as finished_at or calculate_run_time(clock.now) in the spirit of http://devblog.avdi.org/2014/08/26/how-to-use-rubys-english-andor-operators-without-going-nuts/ - but rubocop prohibits using or so I tweaked it to || (which works, but… yeah, what you said).
Anyway, I agree that the unless version is more-expressive - thanks for pointing that out.
soulcutter
force-pushed
the
abort-with-profile
branch
from
d36fc00
to
e0b5572
Compare
In the case where RSpec does not handle an error (e.g. SystemExit), we still want to capture the example timing data to not break profiling information that may get generated via `--profile`. Fixes #2142
soulcutter
force-pushed
the
abort-with-profile
branch
from
e0b5572
to
8cdb63a
Compare
|
👍 |
Ensure ExecutionResult timing is always populated
Ensure ExecutionResult timing is always populated
In the case where RSpec does not handle an error (e.g.
SystemExit), we still need to capture the example timing data in order not to break profiling information that may get generated via--profileusing those values.