Create LogCaptureHandler if necessary (closes #6240) #6283
Conversation
src/_pytest/logging.py
Outdated
| with catching_logs(self.log_file_handler, level=self.log_file_level): | ||
| yield | ||
| else: | ||
| with catching_logs(self.log_file_handler, level=self.log_file_level): |
There was a problem hiding this comment.
I don't like that catching_logs implicitly logs to LogCaptureHandler if self.log_file_hander is None. I would suggest to be more explicit and pass
self.log_file_handler if self.log_file_handler else LogCaptureHandler() as the first arg to catching_logs.
Then your modification to the catching_logs ctx can be reverted.
WDYT?
There was a problem hiding this comment.
@thisch
IIRC there are other places where using a new / anonymous LogCaptureHandler could be used, therefore I've changed it to handle that for the arg being None here already. (it avoids creating it then in several places etc)
But of couse for this patch alone your suggestion might make sense / seems good probably.
There was a problem hiding this comment.
@thisch, I've updated the PR to follow your suggestion.
src/_pytest/logging.py
Outdated
| with catching_logs(self.log_file_handler, level=self.log_file_level): | ||
| yield | ||
| else: | ||
| with catching_logs( |
There was a problem hiding this comment.
@thisch shouldn't we keep the reference to the anonymous capture handler?
if self.log_file_handler is None:
self.log_file_handler = LogCaptureHandler()
with catching_logs(self.log_file_handler, level=self.log_file_level):There was a problem hiding this comment.
AFAICS we don't have to. TBH I don't yet understand why this fix fixes the problem in #6240. I'll take a closer look in the following days.
src/_pytest/logging.py
Outdated
| with catching_logs(self.log_file_handler, level=self.log_file_level): | ||
| yield | ||
| else: | ||
| with catching_logs( |
There was a problem hiding this comment.
AFAICS we don't have to. TBH I don't yet understand why this fix fixes the problem in #6240. I'll take a closer look in the following days.
|
@thisch ping |
|
@felixn @thisch |
|
@felixn please let me know if you'd like me to help you finish this, or rather prefer me to do so. |
There was a problem hiding this comment.
Hi @felixn,
As mentioned here: #6240 (comment), this approach looks good to me, but requires minor changes (and a rebase).
If you're not able to update the PR, let me know.
|
@thisch, your review on this PR is still pending; what do you think about the latest version? |
|
Thanks for the review and positive feedback, @thisch and @bluetech. |
#6240
All credit goes to @blueyed in:
#6240 (comment)
Should I add a testcase to test this specific behaviour?