diff --git a/CHANGES.rst b/CHANGES.rst index 76f2b0066..dfefd60ba 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -51,6 +51,18 @@ Unreleased commands. :issue:`3107` :pr:`3228` - Add ``click.get_pager_file`` for file-like access to an output pager. :pr:`1572` +- Add ``capture`` parameter to :class:`CliRunner` with two modes: ``sys`` + (default) and ``fd``. ``fd`` redirects file descriptors ``1`` and ``2`` + via :func:`os.dup2` so output that bypasses ``sys.stdout`` (stale stream + references, C extensions, subprocesses, ``faulthandler``) is captured + with proper isolation. :issue:`854` :issue:`2412` :issue:`2468` + :issue:`2497` :issue:`2761` :issue:`2827` :issue:`2865` +- Revert the ``8.3.3`` change that exposed the original file descriptor + via ``fileno()`` on the redirected ``CliRunner`` streams in the default + capture mode. ``os.dup2(w, sys.stdout.fileno())`` calls inside a CLI no + longer mutate the host runner's stdout, which broke Pytest's ``fd``-level + capture teardown. C-level consumers that need a real ``fd`` should use + ``capture="fd"``. :issue:`3384` :pr:`3391` Version 8.3.3 ------------- diff --git a/docs/testing.md b/docs/testing.md index 73f88f547..3c2efe0ff 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -204,50 +204,62 @@ Prompts will be emulated so they write the input data to the output stream as well. If hidden input is expected then this does not happen. -## File Descriptors and Low-Level I/O - -{class}`CliRunner` captures output by replacing -`sys.stdout` and `sys.stderr` with in-memory -{class}`~io.BytesIO`-backed wrappers. This is -Python-level redirection: calls to {func}`~click.echo`, -{func}`print`, or `sys.stdout.write()` are captured, but -the wrappers have no OS-level file descriptor. - -Code that calls `fileno()` on `sys.stdout` or -`sys.stderr`, like {mod}`faulthandler`, -{mod}`subprocess`, or C extensions, would normally crash -with {exc}`io.UnsupportedOperation` inside -{class}`CliRunner`. - -To avoid this, {class}`CliRunner` preserves the original -stream's file descriptor and exposes it via `fileno()` on -the replacement wrapper. - -This means: -- **Python-level writes** (`print()`, `click.echo()`, - ...) are captured as usual. -- **fd-level writes** (C code writing directly to the - file descriptor) go to the original terminal and are - **not** captured. - -This is the same trade-off that -[pytest](https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html) -makes with its two capture modes: - -- `capsys`, which captures Python-level output, where - `fileno()` raises `UnsupportedOperation` and fd-level - writes are not captured. -- `capfd`, which captures fd-level output via - `os.dup2()`, where `fileno()` works and fd-level - writes *are* captured. - -Rather than implementing a full `capfd`-style mechanism, -{class}`CliRunner` takes the simpler path: expose the -original `fd` so that standard library helpers keep -working, while accepting that their output is not -captured. - -```{versionchanged} 8.3.3 -`fileno()` on the redirected streams now returns the -original stream's file descriptor instead of raising. +## Capture modes + +{class}`CliRunner` captures output by replacing `sys.stdout` and `sys.stderr` +with in-memory wrappers. The `capture` parameter controls which strategy is +used. + +### `capture="sys"` (default) + +Captures Python-level writes (`print()`, `click.echo()`, `sys.stdout.write()`). +It is fast and sufficient for most Click applications. + +Code that holds a reference to the original `sys.stdout` (like a library that +does `from sys import stdout` at import time) bypasses the capture and its +output is lost. + +In this mode `sys.stdout.fileno()` and `sys.stderr.fileno()` raise +{exc}`io.UnsupportedOperation`, matching the pre-`8.3.3` behavior. C-level +consumers ({mod}`faulthandler`, {mod}`subprocess`, C extensions) that expect a +real file descriptor must opt into the `capture="fd"` mode. + +### `capture="fd"` + +Redirects OS file descriptors `1` and `2` to a temporary file via +{func}`os.dup2`, inspired by [Pytest's +`capfd`](https://docs.pytest.org/en/stable/how-to/capture-stdout-stderr.html). +This catches output that bypasses `sys.stdout`, including: + +- Stale references to the original `sys.stdout` and `sys.stderr`. +- Logging frameworks that cache the original stream (like `structlog` or the + stdlib's `logging` module). +- C extensions and subprocesses that write directly to `fd 1` or `fd 2`. + +```python +from click.testing import CliRunner +from myapp import cli + + +def test_captures_everything(): + runner = CliRunner(capture="fd") + result = runner.invoke(cli) + # result.stdout contains both Python-level and fd-level output + assert "expected output" in result.stdout +``` + +In this mode `sys.stdout.fileno()` returns the saved (pre-redirection) `fd`, so +{mod}`faulthandler` and similar consumers keep working. Writes to `fd 1` and +`fd 2` land in the capture tmpfile, so `os.dup2()` calls inside the CLI no +longer leak into the host runner's stdout. + +```{note} +`capture="fd"` is not available on Windows. +``` + +```{versionchanged} 8.4.0 +Added the `capture` parameter. The default `sys` mode no longer exposes the +original `fd` through `fileno()`, reverting the change introduced in `8.3.3` +that broke Pytest's `fd`-level capture teardown. Use `capture="fd"` to restore +that behavior with proper isolation. ``` diff --git a/src/click/testing.py b/src/click/testing.py index 04e7f1d92..1eb81d29e 100644 --- a/src/click/testing.py +++ b/src/click/testing.py @@ -22,6 +22,8 @@ from .core import Command +CaptureMode = t.Literal["sys", "fd"] + class EchoingStdin: def __init__(self, input: t.BinaryIO, output: t.BinaryIO) -> None: @@ -67,6 +69,39 @@ def _pause_echo(stream: EchoingStdin | None) -> cabc.Iterator[None]: stream._paused = False +class _FDCapture: + """Redirect a file descriptor to a temporary file for capture. + + Saves the current target of *targetfd* via :func:`os.dup`, then + redirects it to a temporary file via :func:`os.dup2`. On + :meth:`stop`, restores the original ``fd`` and returns the captured + bytes. Inspired by Pytest's ``FDCapture``. + + .. versionadded:: 8.4.0 + """ + + def __init__(self, targetfd: int) -> None: + self._targetfd = targetfd + self.saved_fd: int = -1 + self._tmpfile: t.BinaryIO | None = None + + def start(self) -> None: + self.saved_fd = os.dup(self._targetfd) + self._tmpfile = tempfile.TemporaryFile(buffering=0) + os.dup2(self._tmpfile.fileno(), self._targetfd) + + def stop(self) -> bytes: + assert self._tmpfile is not None, "_FDCapture.start() was not called" + os.dup2(self.saved_fd, self._targetfd) + os.close(self.saved_fd) + self.saved_fd = -1 + self._tmpfile.seek(0) + data = self._tmpfile.read() + self._tmpfile.close() + self._tmpfile = None + return data + + class BytesIOCopy(io.BytesIO): """Patch ``io.BytesIO`` to let the written stream be copied to another. @@ -104,14 +139,12 @@ class _NamedTextIOWrapper(io.TextIOWrapper): """A :class:`~io.TextIOWrapper` with custom ``name`` and ``mode`` that does not close its underlying buffer. - An optional ``original_fd`` preserves the file descriptor of the - stream being replaced, so that C-level consumers that call - :meth:`fileno` (``faulthandler``, ``subprocess``, ...) still work. - Inspired by pytest's ``capsys``/``capfd`` split: see :doc:`/testing` - for details. - - .. versionchanged:: 8.3.3 - Added ``original_fd`` parameter and :meth:`fileno` override. + When ``CliRunner`` runs in ``fd`` mode, ``_original_fd`` is patched to + point at the saved (pre-redirection) ``fd``, so C-level consumers that call + :meth:`fileno` (like ``faulthandler`` or ``subprocess``) keep working. In + the default ``sys`` mode ``_original_fd`` stays at ``-1`` and + :meth:`fileno` raises :exc:`io.UnsupportedOperation`, matching the + pre-``8.3.3`` behavior. """ def __init__( @@ -119,14 +152,12 @@ def __init__( buffer: t.BinaryIO, name: str, mode: str, - *, - original_fd: int = -1, **kwargs: t.Any, ) -> None: super().__init__(buffer, **kwargs) self._name = name self._mode = mode - self._original_fd = original_fd + self._original_fd: int = -1 def close(self) -> None: """The buffer this object contains belongs to some other object, @@ -137,15 +168,10 @@ def close(self) -> None: """ def fileno(self) -> int: - """Return the file descriptor of the original stream, if one was - provided at construction time. - - This allows C-level consumers (``faulthandler``, ``subprocess``, - signal handlers, ...) to obtain a valid fd without crashing, even - though the Python-level writes are redirected to an in-memory - buffer. - - .. versionadded:: 8.3.3 + """Return the file descriptor of the saved original stream when + ``CliRunner`` runs in ``fd`` mode. Otherwise delegate to + :class:`~io.TextIOWrapper`, which raises + :exc:`io.UnsupportedOperation` for a ``BytesIO``-backed buffer. """ if self._original_fd >= 0: return self._original_fd @@ -272,6 +298,21 @@ class CliRunner: will automatically echo the input. :param catch_exceptions: Whether to catch any exceptions other than ``SystemExit`` when running :meth:`~CliRunner.invoke`. + :param capture: Selects the output capture strategy. ``sys`` (default) + captures Python-level writes only and leaves + :meth:`sys.stdout.fileno` raising :exc:`io.UnsupportedOperation`, so + user code that calls :func:`os.dup2` on ``sys.stdout.fileno()`` cannot + clobber the host runner's stdout. ``fd`` redirects file descriptors + ``1`` and ``2`` via :func:`os.dup2` to a temporary file, also catching + output from stale stream references, C extensions, and subprocesses. + ``fd`` is not supported on Windows. + + .. versionchanged:: 8.4.0 + Added the ``capture`` parameter. The default ``sys`` mode no longer + exposes the original fd through :meth:`fileno`, reverting the change + introduced in ``8.3.3`` that broke Pytest's ``fd``-level capture + teardown. Use ``capture="fd"`` to restore that behavior with proper + isolation. :issue:`3384` .. versionchanged:: 8.2 Added the ``catch_exceptions`` parameter. @@ -286,11 +327,21 @@ def __init__( env: cabc.Mapping[str, str | None] | None = None, echo_stdin: bool = False, catch_exceptions: bool = True, + capture: CaptureMode = "sys", ) -> None: + if capture not in {"sys", "fd"}: + raise ValueError( + f"capture={capture!r} is not valid. Choose from 'sys' or 'fd'." + ) + if capture == "fd" and sys.platform == "win32": + raise ValueError( + f"capture={capture!r} is not supported on Windows. Use 'sys'." + ) self.charset = charset self.env: cabc.Mapping[str, str | None] = env or {} self.echo_stdin = echo_stdin self.catch_exceptions = catch_exceptions + self.capture: CaptureMode = capture def get_default_prog_name(self, cli: Command) -> str: """Given a command object it will return the default program name @@ -355,20 +406,6 @@ def isolation( stream_mixer = StreamMixer() - # Preserve the original file descriptors so that C-level - # consumers (faulthandler, subprocess, etc.) can still obtain a - # valid fd from the redirected streams. The original streams - # may themselves lack a fileno() (e.g. when CliRunner is used - # inside pytest's capsys), so we fall back to -1. - def _safe_fileno(stream: t.IO[t.Any]) -> int: - try: - return stream.fileno() - except (AttributeError, io.UnsupportedOperation): - return -1 - - old_stdout_fd = _safe_fileno(old_stdout) - old_stderr_fd = _safe_fileno(old_stderr) - if self.echo_stdin: bytes_input = echo_input = t.cast( t.BinaryIO, EchoingStdin(bytes_input, stream_mixer.stdout) @@ -388,7 +425,6 @@ def _safe_fileno(stream: t.IO[t.Any]) -> int: encoding=self.charset, name="", mode="w", - original_fd=old_stdout_fd, ) sys.stderr = _NamedTextIOWrapper( @@ -397,7 +433,6 @@ def _safe_fileno(stream: t.IO[t.Any]) -> int: name="", mode="w", errors="backslashreplace", - original_fd=old_stderr_fd, ) @_pause_echo(echo_input) # type: ignore @@ -579,7 +614,27 @@ def invoke( if catch_exceptions is None: catch_exceptions = self.catch_exceptions + # Set up fd capture before isolation replaces sys.stdout and sys.stderr. + cap_out: _FDCapture | None = None + cap_err: _FDCapture | None = None + + if self.capture == "fd": + cap_out = _FDCapture(1) + cap_err = _FDCapture(2) + try: + cap_out.start() + cap_err.start() + except OSError: + cap_out = cap_err = None + with self.isolation(input=input, env=env, color=color) as outstreams: + # Point the captured streams' fileno() at the saved (original) + # fd so that C-level consumers like faulthandler keep working + # while fd 1/2 are redirected to the capture tmpfile. + if cap_out is not None and cap_err is not None: + sys.stdout._original_fd = cap_out.saved_fd # type: ignore[union-attr] + sys.stderr._original_fd = cap_err.saved_fd # type: ignore[union-attr] + return_value = None exception: BaseException | None = None exit_code = 0 @@ -620,6 +675,18 @@ def invoke( finally: sys.stdout.flush() sys.stderr.flush() + + # Stop fd capture and merge the captured bytes into + # the stdout/stderr BytesIO streams. BytesIOCopy mirrors + # those writes into outstreams[2] automatically. + if cap_out is not None and cap_err is not None: + fd_out = cap_out.stop() + fd_err = cap_err.stop() + if fd_out: + outstreams[0].write(fd_out) + if fd_err: + outstreams[1].write(fd_err) + stdout = outstreams[0].getvalue() stderr = outstreams[1].getvalue() output = outstreams[2].getvalue() diff --git a/tests/test_testing.py b/tests/test_testing.py index c8fca4d4f..649db6eda 100644 --- a/tests/test_testing.py +++ b/tests/test_testing.py @@ -1,4 +1,5 @@ import faulthandler +import io import os import pdb import sys @@ -522,26 +523,227 @@ def cli(): assert pdb.Pdb.__init__ is original -def test_faulthandler_enable(runner): - """``faulthandler.enable()`` inside ``CliRunner`` should not crash with - ``io.UnsupportedOperation: fileno``. +needs_fd_capture = pytest.mark.skipif( + sys.platform == "win32", + reason="fd capture not supported on Windows", +) + + +def test_capture_invalid_mode(): + """Invalid capture mode raises ValueError.""" + with pytest.raises(ValueError, match="capture"): + CliRunner(capture="invalid") # type: ignore[arg-type] + + +@pytest.mark.skipif(sys.platform != "win32", reason="Windows-only test") +def test_capture_fd_windows_error(): + """fd capture raises ValueError on Windows.""" + with pytest.raises(ValueError, match="not supported on Windows"): + CliRunner(capture="fd") + + +@needs_fd_capture +def test_capture_fd_os_write(): + """capture='fd' captures writes to fd 1/2 that bypass sys.stdout.""" + + @click.command() + def cli(): + click.echo("python stdout") + os.write(1, b"fd stdout\n") + os.write(2, b"fd stderr\n") + + runner = CliRunner(capture="fd") + result = runner.invoke(cli) + assert "python stdout" in result.stdout + assert "fd stdout" in result.stdout + assert "fd stderr" in result.stderr + + +@needs_fd_capture +def test_capture_fd_stale_reference(): + """capture='fd' captures writes from stale stdout references (issue #2874). + + Simulates ``from sys import stdout`` at import time, which grabs + the real stdout connected to fd 1. The stale object's underlying + FileIO uses fd 1, so redirecting fd 1 captures its writes. + """ + # open(1, ..., closefd=False) creates a writer whose underlying + # FileIO uses fd 1 directly. This mirrors the real scenario: + # the original sys.stdout is a TextIOWrapper -> BufferedWriter -> + # FileIO(fd=1). + stale_stdout = open(1, "w", closefd=False) # noqa: SIM115 + + @click.command() + def cli(): + stale_stdout.write("stale write\n") + stale_stdout.flush() + click.echo("normal write") + + runner = CliRunner(capture="fd") + result = runner.invoke(cli) + assert "normal write" in result.stdout + assert "stale write" in result.stdout + + +@needs_fd_capture +def test_capture_fd_logging_handler(tmp_path): + """capture='fd' captures logging output from a handler holding a stale + stderr reference (issue #2827). + + stdlib logging.StreamHandler grabs sys.stderr at configuration time. + Under normal CliRunner (sys-level capture), the handler still writes + to the original stream object and output is lost. fd-level capture + redirects the underlying file descriptor, so the writes are captured. + """ + import logging + + # Create a writer backed by the real fd 2, simulating a handler + # configured at import time before pytest or CliRunner replaced + # sys.stderr. open(2, closefd=False) mirrors the real scenario: + # the original sys.stderr is a TextIOWrapper -> BufferedWriter -> + # FileIO(fd=2). + stale_stderr = open(2, "w", closefd=False) # noqa: SIM115 + handler = logging.StreamHandler(stale_stderr) + handler.setFormatter(logging.Formatter("%(message)s")) + + logger = logging.getLogger(f"click_test_{tmp_path.name}") + logger.addHandler(handler) + logger.setLevel(logging.INFO) + logger.propagate = False + + @click.command() + def cli(): + logger.info("log from stale handler") + click.echo("normal echo") + + # sys-level capture misses the log line (it bypasses sys.stderr). + runner_sys = CliRunner(capture="sys") + result_sys = runner_sys.invoke(cli) + assert "normal echo" in result_sys.output + assert "log from stale handler" not in result_sys.output + + # fd-level capture catches it by redirecting fd 2. + runner_fd = CliRunner(capture="fd") + result_fd = runner_fd.invoke(cli) + assert "normal echo" in result_fd.output + assert "log from stale handler" in result_fd.output + + logger.removeHandler(handler) + + +@needs_fd_capture +def test_capture_fd_faulthandler(): + """faulthandler.enable() works with capture='fd' (issue #2865).""" + + @click.command() + def cli(): + faulthandler.enable() + click.echo("after faulthandler") + + runner = CliRunner(capture="fd") + result = runner.invoke(cli) + assert result.exit_code == 0 + assert "after faulthandler" in result.output - ``faulthandler.enable()`` needs a real OS file descriptor to register - its signal handler. ``CliRunner`` replaces ``sys.stderr`` with a - ``BytesIO`` wrapper that has no ``fileno()``, causing the call to fail. - Reproduce:https://github.com/pallets/click/issues/2865 +def test_capture_sys_fileno_raises(): + """capture='sys' leaves fileno() raising UnsupportedOperation, so user + code that does ``os.dup2(w, sys.stdout.fileno())`` cannot mutate the host + runner's stdout (issue #3384). """ @click.command() - @click.option("--flag", type=bool, default=True) - def cli(flag): - click.echo("Executing main function...") - if flag: - click.echo("Registering faulthandler") - faulthandler.enable() - click.echo("Finished executing main function.") + def cli(): + with pytest.raises(io.UnsupportedOperation): + sys.stdout.fileno() + with pytest.raises(io.UnsupportedOperation): + sys.stderr.fileno() + click.echo("ok") - result = runner.invoke(cli, ["--flag", True]) + runner = CliRunner(capture="sys") + result = runner.invoke(cli) assert result.exit_code == 0, result.output - assert "Finished executing main function." in result.output + assert "ok" in result.stdout + + +@needs_fd_capture +def test_capture_fd_stderr_separation(): + """capture='fd' properly separates fd-level stdout and stderr.""" + + @click.command() + def cli(): + click.echo("py-out") + click.echo("py-err", err=True) + os.write(1, b"fd-out\n") + os.write(2, b"fd-err\n") + + runner = CliRunner(capture="fd") + result = runner.invoke(cli) + assert "py-out" in result.stdout + assert "fd-out" in result.stdout + assert "py-err" in result.stderr + assert "fd-err" in result.stderr + # Mixed output has all of them + assert "py-out" in result.output + assert "py-err" in result.output + assert "fd-out" in result.output + assert "fd-err" in result.output + + +@needs_fd_capture +def test_capture_fd_nesting(): + """Nested CliRunner.invoke() with fd capture works correctly.""" + + @click.command("inner") + def inner_cli(): + click.echo("inner") + + @click.command("outer") + def outer_cli(): + click.echo("outer") + os.write(1, b"outer fd\n") + inner_runner = CliRunner(capture="fd") + inner_result = inner_runner.invoke(inner_cli) + click.echo(f"inner captured: {inner_result.stdout.strip()}") + + runner = CliRunner(capture="fd") + result = runner.invoke(outer_cli) + assert "outer" in result.stdout + assert "outer fd" in result.stdout + assert "inner captured: inner" in result.stdout + + +@needs_fd_capture +@pytest.mark.parametrize("runner_capture", ["sys", "fd"]) +@pytest.mark.parametrize("pytest_fixture", ["capsys", "capfd"]) +def test_capture_pytest_matrix( + request: pytest.FixtureRequest, + runner_capture: str, + pytest_fixture: str, +) -> None: + """Matrix of ``CliRunner`` capture modes and Pytest capture fixtures.""" + pytest_cap = request.getfixturevalue(pytest_fixture) + + @click.command() + def cli() -> None: + click.echo("inside-stdout") + click.echo("inside-stderr", err=True) + + runner = CliRunner(capture=runner_capture) + result = runner.invoke(cli) + + # CliRunner sees Click's output. + assert result.exit_code == 0 + assert "inside-stdout" in result.stdout + assert "inside-stderr" in result.stderr + + # Pytest capture machinery is still working after invoke(). + print("post-invoke-stdout") + print("post-invoke-stderr", file=sys.stderr) + captured = pytest_cap.readouterr() + assert "post-invoke-stdout" in captured.out + assert "post-invoke-stderr" in captured.err + + # Click output is not leaking into Pytest. + assert "inside-stdout" not in captured.out + assert "inside-stderr" not in captured.err