Bugfix/capsys with cli logging (again) #3832
Conversation
… when suspending - solves bug but still missing tests), reorganized functions and context managers.
|
This is great @Sup3rGeo, thanks a lot. I see the main problem is that currently we drop the captured output between live log calls because we suspend the capture manager when emitting log records, and this PR fixes that. Also great work on the refactoring, it reduces code and improves readability! 👍 |
src/_pytest/capture.py
Outdated
| if outcome.excinfo is not None: | ||
| out, err = capman.snap_global_capture() |
There was a problem hiding this comment.
Since we are refactoring things, how do you feel about renaming this to read_global_capture?
There was a problem hiding this comment.
I was trying to use the same naming convention, as capture objects implement snap.
Should we then rename everything from snap to read? It seems a bit awkward to have read in global as all it does is to snap internally.
But to be honest I don't mind having either so I leave this decision for you :)
There was a problem hiding this comment.
It is just that snap_global_capture does self._global_capturing.readouterr(), so I think renaming it here makes sense. Snap is then used just for the "Capture" classes. 😁
There was a problem hiding this comment.
Makes sense, will do it!
testing/test_capture.py
Outdated
| @@ -1387,27 +1391,53 @@ def test_pickling_and_unpickling_encoded_file(): | |||
| pickle.loads(ef_as_str) | |||
|
|
|||
|
|
|||
| def test_capsys_with_cli_logging(testdir): | |||
| def test_capture_with_live_logging(testdir): | |||
There was a problem hiding this comment.
Please parametrize this test so it also tests with the capfd fixture. 👍
|
It seems like there is still a change to be addressed but I don't know how to do it |
|
Thanks @Sup3rGeo, top notch work!
You mean the merge was blocked? That was because we configure the repository to block merges until no reviewers left the PR in the "requested changes" status. 😉 If nobody else comments until then, I will merge this later. 👍 |
|
Thanks again @Sup3rGeo! |
|
Not sure how important this is, but this appears to have broken tox-dev/tox@6c7ea27 I believe fixes what I'm seeing |
|
thanks for the heads up @asottile, but I think it is fair game to change non-public API during bugfix releases. 👍 |
|
Totally agree -- that's why I didn't make an issue 😆 |
Solves more problems found related to #3819, improving previous PR #3822 also by refactoring a bit the capture module, mostly by separating the snapping of the global capture from its suspension.
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.masterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
AUTHORSin alphabetical order;