Send timing/cache report to stderr#4946
Conversation
| buffered_output = f.getvalue() | ||
| f.close() | ||
| return buffered_output | ||
|
|
There was a problem hiding this comment.
consider:
from contextlib import closing
@staticmethod
def _consume_stringio(f):
with closing(f):
f.flush()
return f.getvalue()
There was a problem hiding this comment.
I don't think this has much value unless the creation of f is part of the idiom. Here f is created far away, and there already so many other opportunities for it to leak that I suspect that this will be more misleading than helpful.
There was a problem hiding this comment.
its really just a way to eliminate the buffered_output variable in this case by being able to directly return the output of f.getvalue().
| indent=True, timing=True, cache_stats=True, | ||
| errfile = open(os.path.join(global_options.logdir, '{}.err.log'.format(run_id)), 'w') | ||
| settings = PlainTextReporter.Settings(log_level=log_level, outfile=outfile, errfile=errfile, | ||
| color=False, indent=True, timing=True, cache_stats=True, |
There was a problem hiding this comment.
would it make more sense to interleave (e.g. errfile=outfile) in the case of writing to logs vs stdio?
There was a problem hiding this comment.
Yea, probably.
EDIT: Eh... there are two issues here. 1) there is a convention of preserving the stdout/stderr split when executing processes, to make it meaningful, 2) since we've captured them as split during the bootstrap process, we can't usefully re-interleave those initial messages.
Will leave as is. Easy enough to change later.
| class ReporterDestination(object): | ||
| OUT = 'out' | ||
| ERR = 'err' | ||
|
|
There was a problem hiding this comment.
Was thinking debuggability, but I guess there would actually be a slight performance difference here, huh? Will switch.
| buffered_output = f.getvalue() | ||
| f.close() | ||
| return buffered_output | ||
|
|
There was a problem hiding this comment.
I don't think this has much value unless the creation of f is part of the idiom. Here f is created far away, and there already so many other opportunities for it to leak that I suspect that this will be more misleading than helpful.
Problem
--timeis very useful to have enabled by default in automated environments, but currently renders tostdout.Solution
Add
errfiletoPlainTextReporter, and render the timing and cache hit report ("epilog") to it.Result
--timecan safely be used in more cases.