Show subprocess stdout and stderr on pytest failure #29890

thomashopkins32 · 2025-04-09T15:00:09Z

PR summary

Displays the captured stdout and stderr when a pytest test that uses the subprocess helper exits in failure.

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

timhoffm

Thanks for the PR.

timhoffm · 2025-04-09T15:10:40Z

lib/matplotlib/testing/__init__.py

@@ -105,6 +105,9 @@ def subprocess_run_for_testing(command, env=None, timeout=60, stdout=None,
            import pytest
            pytest.xfail("Fork failure")
        raise
+    except subprocess.CalledProcessError as e:
+        import pytest
+        pytest.xfail(f"Subprocess failed with exit code {e.returncode}:\n{e.stdout}\n{e.stderr}")


This should be a plain fail not an xfail, also made it a bit more verbose to make stdout and stderr output easier to read:

Suggested change

pytest.xfail(f"Subprocess failed with exit code {e.returncode}:\n{e.stdout}\n{e.stderr}")

pytest.fail(

f"Subprocess failed with exit code {e.returncode}:\n\n"

f"STDOUT:\n{e.stdout}\n\nSTDERR:\n{e.stderr}")

Thanks! I agree I like yours better. Just made the change.

timhoffm · 2025-04-09T16:21:32Z

In fact, it's a little more complicated than that. We have use cases where we expect the subprocess to fail (or more generally are ok with that), e.g.
https://github.com/matplotlib/matplotlib/blob/7e997ae87b2abd927830445df5d768c835503504/lib/matplotlib/tests/test_backends_interactive.py#L462-473

So we cannot unconditionally pytest.fail on a CalledProcessError. The correct way forward depends on the common use cases. Options are:

wrap usages of subprocess_run_for_testing() or subprocess_run_helper() (which uses it) in a try/expect directly inside the test functions if the function does not further catch CalledProcessError (to be checked whether TimeoutError is relevant too). This would we reasonable if we only have very few such cases.
Introduce a flag subprocess_run_for_testing(..., fail_on_error=...). This let's the call site choose the behavior. I'd set this to False by default, because a function automatically failing a test can be surprising. The behavior should be explicitly opt-in: subprocess_run_for_testing(..., fail_on_error=True).

thomashopkins32 · 2025-04-09T16:21:37Z

Looks like some tests catch the subprocess.CalledProcessError to validate the error outputs. Should this be refactored such that the actual warnings/errors are caught or should this PR be closed?

For example, b43b568 is a change that could be made to make sure that the actual UserWarning is caught rather than shown through the stderr of the subprocess.

thomashopkins32 · 2025-04-09T16:25:26Z

Introduce a flag subprocess_run_for_testing(..., fail_on_error=...). This let's the call site choose the behavior. I'd set this to False by default, because a function automatically failing a test can be surprising. The behavior should be explicitly opt-in: subprocess_run_for_testing(..., fail_on_error=True).

I like this approach for keeping this PR small and in case tests actually need to catch a subprocess error. The tests I have seen can be refactored to set this fail_on_error to True in the future similar to my previous comment.

QuLogic · 2025-04-10T03:55:08Z

For example, b43b568 is a change that could be made to make sure that the actual UserWarning is caught rather than shown through the stderr of the subprocess.

That change is invalid though; as the comment says, the test is specifically ignoring all other failures.

thomashopkins32 · 2025-04-10T13:08:52Z

@QuLogic Yeah I realized that after it failed again, sorry about that. So the errors definitely need to propagate through I agree, just wanted to see if it was easy to add the outputs to stdout and stderr from the subprocess back into the main process to make errors more visible.

thomashopkins32 · 2025-04-10T13:37:03Z

This may work better now. It should show the subprocess' stdout and stderr both when the subprocess fails and when it succeeds. This way, when someone uses log_cli=True they can get the outputs of the subprocess in addition to the main process even when tests pass.

If you think this change is unnecessary feel free to close the PR.

lib/matplotlib/testing/__init__.py

tacaswell · 2025-07-30T18:14:21Z

@thomashopkins32 Can you please squash this to one commit?

tacaswell · 2025-07-30T18:21:55Z

Confirmed that this comes out nicely in the logs.

tacaswell

modulo squashing.

For full disclosure, Tom is a co-worker so scale my approval by whatever conflict of interest factor you want.

timhoffm · 2025-07-30T20:37:01Z

so scale my approval by whatever conflict of interest factor you want.

This is a low-risk change, so I'm happy to merge with any 1.x approval factor.

Thanks @ Tom x 2 (to stay in the math terminology 😆)

github-actions bot reviewed Apr 9, 2025

View reviewed changes

timhoffm reviewed Apr 9, 2025

View reviewed changes

thomashopkins32 requested a review from timhoffm April 10, 2025 19:40

melissawm added this to First Time Contributors May 23, 2025

melissawm moved this to Needs review in First Time Contributors May 23, 2025

timhoffm mentioned this pull request Jul 30, 2025

Mnt/test qol improvements #30373

Merged

1 task

tacaswell reviewed Jul 30, 2025

View reviewed changes

lib/matplotlib/testing/__init__.py Outdated Show resolved Hide resolved

tacaswell reviewed Jul 30, 2025

View reviewed changes

lib/matplotlib/testing/__init__.py Outdated Show resolved Hide resolved

tacaswell added this to the v3.11.0 milestone Jul 30, 2025

tacaswell approved these changes Jul 30, 2025

View reviewed changes

Show subprocess stdout and stderr on pytest failure

126507f

thomashopkins32 force-pushed the capture-subprocess-test-errors branch from 9165149 to 126507f Compare July 30, 2025 18:44

timhoffm approved these changes Jul 30, 2025

View reviewed changes

timhoffm merged commit 9573103 into matplotlib:main Jul 30, 2025
34 of 37 checks passed

github-project-automation bot moved this from Needs review to Waiting for author in First Time Contributors Jul 30, 2025

QuLogic moved this from Waiting for author to Merged in First Time Contributors Jul 30, 2025

thomashopkins32 deleted the capture-subprocess-test-errors branch July 30, 2025 20:39

-        pytest.xfail(f"Subprocess failed with exit code {e.returncode}:\n{e.stdout}\n{e.stderr}")
+        pytest.fail(
+            f"Subprocess failed with exit code {e.returncode}:\n\n"
+            f"STDOUT:\n{e.stdout}\n\nSTDERR:\n{e.stderr}")

Uh oh!

Show subprocess stdout and stderr on pytest failure #29890

Show subprocess stdout and stderr on pytest failure #29890

Uh oh!

Conversation

thomashopkins32 commented Apr 9, 2025

PR summary

PR checklist

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

thomashopkins32 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Apr 9, 2025

Uh oh!

thomashopkins32 commented Apr 9, 2025

Uh oh!

thomashopkins32 commented Apr 9, 2025

Uh oh!

QuLogic commented Apr 10, 2025

Uh oh!

thomashopkins32 commented Apr 10, 2025

Uh oh!

thomashopkins32 commented Apr 10, 2025

Uh oh!

Uh oh!

Uh oh!

tacaswell commented Jul 30, 2025

Uh oh!

tacaswell commented Jul 30, 2025

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

timhoffm commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!