Deprecate isolated_filesystem and document its limits

CHANGES.md

@@ -43,6 +43,14 @@ Unreleased
   `DeprecationWarning` until Click 9.0: `LazyFile`, `KeepOpenFile`,
   `make_default_short_help`, `PacifyFlushWrapper`, and `safecall`.
   {issue}`3099` {pr}`3695`
+- Deprecate {meth}`CliRunner.isolated_filesystem`. It relies on
+  {func}`os.chdir`, which mutates process-global state and is not
+  thread-safe. The helper predates Python 3 and modern pytest: use a
+  temporary directory ({class}`tempfile.TemporaryDirectory` or pytest's
+  `tmp_path` fixture) with absolute paths instead. For running tests in
+  parallel, use process-based isolation (such as `pytest-xdist`) rather
+  than threads, since {meth}`CliRunner.invoke` also redirects the
+  process-global standard streams. {issue}`3501` {issue}`3700` {pr}`3704`
 
 ## Version 8.4.2
 

docs/testing.md

@@ -7,8 +7,12 @@
 Click provides the {ref}`click.testing <testing>` module to help you invoke
 command line applications and check their behavior.
 
-These tools should only be used for testing since they change
-the entire interpreter state for simplicity. They are not thread-safe!
+```{warning}
+These tools should only be used for testing since they change the entire
+interpreter state for simplicity. They are not thread-safe: see
+[Running tests in parallel](#running-tests-in-parallel) for the supported
+way to run CLI tests concurrently.
+```
 
 The examples use [pytest](https://docs.pytest.org/en/stable/) style tests.
 
@@ -119,8 +123,11 @@ def test_sync():
 
 ## File System Isolation
 
-The {meth}`CliRunner.isolated_filesystem` context manager sets the current
-working directory to a new, empty folder.
+To test a command that reads or writes files, give each test its own
+directory and pass absolute paths to the command. Pytest's
+[`tmp_path`](https://docs.pytest.org/en/stable/how-to/tmp_path.html) fixture
+provides a fresh directory per test; {class}`tempfile.TemporaryDirectory`
+works without pytest.
 
 ```{code-block} python
 :caption: cat.py
@@ -139,36 +146,47 @@ def cat(f):
 from click.testing import CliRunner
 from cat import cat
 
-def test_cat():
-   runner = CliRunner()
-   with runner.isolated_filesystem():
-      with open('hello.txt', 'w') as f:
-          f.write('Hello World!')
+def test_cat(tmp_path):
+   hello = tmp_path / 'hello.txt'
+   hello.write_text('Hello World!')
 
-      result = runner.invoke(cat, ['hello.txt'])
-      assert result.exit_code == 0
-      assert result.output == 'Hello World!\n'
+   runner = CliRunner()
+   result = runner.invoke(cat, [str(hello)])
+   assert result.exit_code == 0
+   assert result.output == 'Hello World!\n'
 ```
 
-Pass in a path to control where the temporary directory is created.
-In this case, the directory will not be removed by Click. Its useful
-to integrate with a framework like Pytest that manages temporary files.
+Because the directory is unique to the test and the command receives an
+absolute path, nothing leaks between tests and the working directory is never
+changed.
 
-```{code-block} python
-:caption: test_cat.py
+### Running tests in parallel
 
-from click.testing import CliRunner
-from cat import cat
+Isolating the file system with pytest is thread-safe: each test, or each thread,
+works in its own directory through absolute paths, with no shared state.
 
-def test_cat_with_path_specified():
-   runner = CliRunner()
-   with runner.isolated_filesystem('~/test_folder'):
-      with open('hello.txt', 'w') as f:
-          f.write('Hello World!')
+{meth}`CliRunner.invoke` is different. While a command runs it swaps several
+pieces of process-global state into place (like `sys.stdout`, `sys.stderr`,
+`sys.stdin`, etc). Two invocations active at the same time in one interpreter
+overwrite each other's state, so the runner cannot be driven from several
+threads at once.
+
+```{warning}
+Do not parallelize CLI tests with threads. Use process-based isolation
+instead, where each worker has its own interpreter and its own copy of those
+globals. [pytest-xdist](https://pytest-xdist.readthedocs.io/) does this with
+`pytest --numprocesses=auto`.
+```
+
+Click's own test suite uses exactly this setup: the `tests-random` dependency
+group and the `random` tox environment in
+[`pyproject.toml`](https://github.com/pallets/click/blob/main/pyproject.toml)
+run the suite under `pytest-randomly` and `pytest-xdist` to surface ordering
+bugs and races.
 
-      result = runner.invoke(cat, ['hello.txt'])
-      assert result.exit_code == 0
-      assert result.output == 'Hello World!\n'
+```{deprecated} 8.5.0
+{meth}`CliRunner.isolated_filesystem` is deprecated and will be removed in
+Click 9.0. Use a temporary directory with absolute paths, as shown above.
 ```
 
 ## Input Streams

docs/upgrade-guides.md

@@ -13,7 +13,7 @@ This guide assumes the user is on version 8.3.X.
 ### Deprecations
 
 For each deprecation, provide a brief explanation, and direct users to new function / class if available.
-- TBD
+- `CliRunner.isolated_filesystem()` is deprecated and will be removed in Click 9.0. The helper predates Python 3 and modern pytest, and it relies on `os.chdir`, which mutates process-global state and is therefore not thread-safe. Replace it with a temporary directory (`tempfile.TemporaryDirectory`, or pytest's `tmp_path` fixture) and pass absolute paths to the command instead of relying on the current working directory. To run commands in parallel, use process-based isolation (such as `pytest-xdist`) rather than threads, since `CliRunner.invoke()` also redirects the process-global standard streams and other interpreter-wide state. See [#3700](https://github.com/pallets/click/issues/3700), [#3501](https://github.com/pallets/click/issues/3501) and the [testing guide](testing.md#running-tests-in-parallel).
 
 ### Removals with prior deprecation
 

src/click/testing.py

@@ -747,13 +747,39 @@ def isolated_filesystem(
         that affect the contents of the CWD to prevent them from
         interfering with each other.
 
+        .. warning::
+            This helper predates Python 3 and modern pytest, and is not
+            thread-safe: it relies on :func:`os.chdir`, which mutates
+            process-global state, and :meth:`invoke` swaps the
+            process-global standard streams too. Parallelize tests with
+            processes (``pytest-xdist``), not threads. Locking the
+            runner (:pr:`3511`, :pr:`3520`, :pr:`3530`) and a
+            ``set_filesystem()`` API (:issue:`3123`) were declined:
+            neither removes the global-state mutation. See :issue:`3700`
+            and :issue:`3501`.
+
         :param temp_dir: Create the temporary directory under this
             directory. If given, the created directory is not removed
             when exiting.
 
+        .. deprecated:: 8.5.0
+            Will be removed in Click 9.0. Use
+            :class:`tempfile.TemporaryDirectory` or pytest's
+            ``tmp_path`` fixture with absolute paths instead.
+
         .. versionchanged:: 8.0
             Added the ``temp_dir`` parameter.
         """
+        import warnings
+
+        warnings.warn(
+            "'isolated_filesystem' is deprecated and will be removed in Click"
+            " 9.0. Use 'tempfile.TemporaryDirectory' or pytest's 'tmp_path'"
+            " fixture with absolute paths instead.",
+            DeprecationWarning,
+            stacklevel=3,
+        )
+
         cwd = os.getcwd()
         dt = tempfile.mkdtemp(dir=temp_dir)
         os.chdir(dt)

tests/test_arguments.py

@@ -102,7 +102,7 @@ def from_bytes(arg):
     )
 
 
-def test_file_args(runner):
+def test_file_args(runner, tmp_path):
     @click.command()
     @click.argument("input", type=click.File("rb"))
     @click.argument("output", type=click.File("wb"))
@@ -113,16 +113,15 @@ def inout(input, output):
                 break
             output.write(chunk)
 
-    with runner.isolated_filesystem():
-        result = runner.invoke(inout, ["-", "hello.txt"], input="Hey!")
-        assert result.output == ""
-        assert result.exit_code == 0
-        with open("hello.txt", "rb") as f:
-            assert f.read() == b"Hey!"
+    hello = tmp_path / "hello.txt"
+    result = runner.invoke(inout, ["-", str(hello)], input="Hey!")
+    assert result.output == ""
+    assert result.exit_code == 0
+    assert hello.read_bytes() == b"Hey!"
 
-        result = runner.invoke(inout, ["hello.txt", "-"])
-        assert result.output == "Hey!"
-        assert result.exit_code == 0
+    result = runner.invoke(inout, [str(hello), "-"])
+    assert result.output == "Hey!"
+    assert result.exit_code == 0
 
 
 def test_path_allow_dash(runner):
@@ -136,7 +135,7 @@ def foo(input):
     assert result.exit_code == 0
 
 
-def test_file_atomics(runner):
+def test_file_atomics(runner, tmp_path):
     @click.command()
     @click.argument("output", type=click.File("wb", atomic=True))
     def inout(output):
@@ -146,14 +145,12 @@ def inout(output):
             old_content = f.read()
             assert old_content == b"OLD\n"
 
-    with runner.isolated_filesystem():
-        with open("foo.txt", "wb") as f:
-            f.write(b"OLD\n")
-        result = runner.invoke(inout, ["foo.txt"], input="Hey!", catch_exceptions=False)
-        assert result.output == ""
-        assert result.exit_code == 0
-        with open("foo.txt", "rb") as f:
-            assert f.read() == b"Foo bar baz\n"
+    foo = tmp_path / "foo.txt"
+    foo.write_bytes(b"OLD\n")
+    result = runner.invoke(inout, [str(foo)], input="Hey!", catch_exceptions=False)
+    assert result.output == ""
+    assert result.exit_code == 0
+    assert foo.read_bytes() == b"Foo bar baz\n"
 
 
 def test_stdout_default(runner):

tests/test_basic.py

@@ -415,7 +415,7 @@ def cli(case):
     assert result.output == repr(expected)
 
 
-def test_file_option(runner):
+def test_file_option(runner, tmp_path):
     @click.command()
     @click.option("--file", type=click.File("w"))
     def input(file):
@@ -426,17 +426,17 @@ def input(file):
     def output(file):
         click.echo(file.read())
 
-    with runner.isolated_filesystem():
-        result_in = runner.invoke(input, ["--file=example.txt"])
-        result_out = runner.invoke(output, ["--file=example.txt"])
+    example = tmp_path / "example.txt"
+    result_in = runner.invoke(input, [f"--file={example}"])
+    result_out = runner.invoke(output, [f"--file={example}"])
 
     assert not result_in.exception
     assert result_in.output == ""
     assert not result_out.exception
     assert result_out.output == "Hello World!\n\n"
 
 
-def test_file_lazy_mode(runner):
+def test_file_lazy_mode(runner, tmp_path):
     do_io = False
 
     @click.command()
@@ -450,76 +450,74 @@ def input(file):
     def output(file):
         pass
 
-    with runner.isolated_filesystem():
-        os.mkdir("example.txt")
+    example = tmp_path / "example.txt"
+    example.mkdir()
 
-        do_io = True
-        result_in = runner.invoke(input, ["--file=example.txt"])
-        assert result_in.exit_code == 1
+    do_io = True
+    result_in = runner.invoke(input, [f"--file={example}"])
+    assert result_in.exit_code == 1
 
-        do_io = False
-        result_in = runner.invoke(input, ["--file=example.txt"])
-        assert result_in.exit_code == 0
+    do_io = False
+    result_in = runner.invoke(input, [f"--file={example}"])
+    assert result_in.exit_code == 0
 
-        result_out = runner.invoke(output, ["--file=example.txt"])
-        assert result_out.exception
+    result_out = runner.invoke(output, [f"--file={example}"])
+    assert result_out.exception
 
     @click.command()
     @click.option("--file", type=click.File("w", lazy=False))
     def input_non_lazy(file):
         file.write("Hello World!\n")
 
-    with runner.isolated_filesystem():
-        os.mkdir("example.txt")
-        result_in = runner.invoke(input_non_lazy, ["--file=example.txt"])
-        assert result_in.exit_code == 2
-        assert "Invalid value for '--file': 'example.txt'" in result_in.output
+    non_lazy = tmp_path / "non_lazy.txt"
+    non_lazy.mkdir()
+    result_in = runner.invoke(input_non_lazy, [f"--file={non_lazy}"])
+    assert result_in.exit_code == 2
+    assert f"Invalid value for '--file': '{non_lazy}'" in result_in.output
 
 
-def test_path_option(runner):
+def test_path_option(runner, tmp_path):
     @click.command()
     @click.option("-O", type=click.Path(file_okay=False, exists=True, writable=True))
     def write_to_dir(o):
         with open(os.path.join(o, "foo.txt"), "wb") as f:
             f.write(b"meh\n")
 
-    with runner.isolated_filesystem():
-        os.mkdir("test")
+    test_dir = tmp_path / "test"
+    test_dir.mkdir()
 
-        result = runner.invoke(write_to_dir, ["-O", "test"])
-        assert not result.exception
+    result = runner.invoke(write_to_dir, ["-O", str(test_dir)])
+    assert not result.exception
 
-        with open("test/foo.txt", "rb") as f:
-            assert f.read() == b"meh\n"
+    with open(test_dir / "foo.txt", "rb") as f:
+        assert f.read() == b"meh\n"
 
-        result = runner.invoke(write_to_dir, ["-O", "test/foo.txt"])
-        assert "is a file" in result.output
+    result = runner.invoke(write_to_dir, ["-O", str(test_dir / "foo.txt")])
+    assert "is a file" in result.output
 
     @click.command()
     @click.option("-f", type=click.Path(exists=True))
     def showtype(f):
         click.echo(f"is_file={os.path.isfile(f)}")
         click.echo(f"is_dir={os.path.isdir(f)}")
 
-    with runner.isolated_filesystem():
-        result = runner.invoke(showtype, ["-f", "xxx"])
-        assert "does not exist" in result.output
+    result = runner.invoke(showtype, ["-f", str(tmp_path / "xxx")])
+    assert "does not exist" in result.output
 
-        result = runner.invoke(showtype, ["-f", "."])
-        assert "is_file=False" in result.output
-        assert "is_dir=True" in result.output
+    result = runner.invoke(showtype, ["-f", str(tmp_path)])
+    assert "is_file=False" in result.output
+    assert "is_dir=True" in result.output
 
     @click.command()
     @click.option("-f", type=click.Path())
     def exists(f):
         click.echo(f"exists={os.path.exists(f)}")
 
-    with runner.isolated_filesystem():
-        result = runner.invoke(exists, ["-f", "xxx"])
-        assert "exists=False" in result.output
+    result = runner.invoke(exists, ["-f", str(tmp_path / "xxx")])
+    assert "exists=False" in result.output
 
-        result = runner.invoke(exists, ["-f", "."])
-        assert "exists=True" in result.output
+    result = runner.invoke(exists, ["-f", str(tmp_path)])
+    assert "exists=True" in result.output
 
 
 def test_choice_option(runner):

tests/test_deprecations.py

@@ -23,3 +23,9 @@ def test_stream_helper_deprecated(module, name):
 def test_utilities_deprecated(name):
     with pytest.warns(DeprecationWarning, match=name):
         getattr(click.utils, name)
+
+
+def test_isolated_filesystem_deprecated(runner):
+    with pytest.warns(DeprecationWarning, match="isolated_filesystem"):
+        with runner.isolated_filesystem():
+            pass