diff --git a/AGENTS.md b/AGENTS.md index 6ef64772..43b3f5fd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,7 +26,8 @@ PyiCloud: a Python library + CLI for interacting with Apple iCloud web services. ## Test gotchas (important) - `pyproject.toml` `[tool.pytest.ini_options]` adds `--disable-socket --allow-unix-socket --timeout=2` via `addopts`. **Tests must not make network calls** and must complete fast. -- `tests/conftest.py` installs autouse fixtures that **block filesystem access**: `open`, `os.open`, `os.mkdir`, `os.makedirs`, `os.chmod` all raise unless the path contains `"python-test-results"`. New tests must mock any file I/O. +- `tests/conftest.py` installs autouse fixtures that **block most filesystem access**: `open`, `os.open`, `os.mkdir`, `os.makedirs`, `os.chmod` all raise `FileSystemAccessError` unless the path contains `"python-test-results"` — that path is the sanctioned escape hatch, not a loophole, and `tests/test_cmdline.py` uses it for session directories. +- The guard has two deliberate gaps. `pathlib` reads such as `Path.read_text()` use `io.open`, not the patched `builtins.open`, so they are never intercepted; and the `open` guards are session-scoped, so module-level code runs before they install. Loading a JSON fixture at import time relies on both, and seven test modules do. Mock file I/O outside those cases. ## Generated protobuf diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cbfdb91f..325f044d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,8 +76,17 @@ Important test constraints: - Tests **must not make network calls** and must complete fast. `pyproject.toml` adds `--disable-socket --allow-unix-socket --timeout=2` via `addopts`. -- `tests/conftest.py` installs autouse fixtures that **block filesystem access**. - New tests must mock any file I/O. +- `tests/conftest.py` installs autouse fixtures that **block most filesystem + access**. `open`, `os.open`, `os.mkdir`, `os.makedirs` and `os.chmod` raise + `FileSystemAccessError` unless the path contains `python-test-results`, which + is the sanctioned location for a test that genuinely needs a temporary file + or directory. +- Two things the guard does not cover, both used deliberately across the suite: + `pathlib` reads such as `Path.read_text()` go through `io.open` rather than + the patched `builtins.open`, and module-level code runs before the + session-scoped fixtures install. Loading a JSON fixture at import time relies + on both and is the established pattern. +- Mock file I/O that falls outside those two cases. - Add fixtures under `tests/const/` (HTTP-response fixtures) or `tests/fixtures/`. ## Coverage diff --git a/tests/test_conftest_guard.py b/tests/test_conftest_guard.py new file mode 100644 index 00000000..a9a3fc8f --- /dev/null +++ b/tests/test_conftest_guard.py @@ -0,0 +1,90 @@ +"""Tests for the filesystem guard the other tests run under. + +The guard in `tests/conftest.py` is described in CONTRIBUTING.md and AGENTS.md, +and those descriptions had drifted from it -- which produced repeated review +findings against tests that were using the guard exactly as intended. These +tests pin the behaviour the documentation now describes, so the two cannot +disagree again without something failing. +""" + +from __future__ import annotations + +import os +from pathlib import Path +import tempfile + +import pytest + +from tests.conftest import FileSystemAccessError + +ALLOWED_MARKER = "python-test-results" + + +def _allowed_path(name: str) -> str: + """Return a path inside the sanctioned location.""" + + return os.path.join(tempfile.gettempdir(), ALLOWED_MARKER, name) + + +def test_open_is_blocked_outside_the_sanctioned_path() -> None: + """`builtins.open` raises rather than touching the developer's disk.""" + + with ( + pytest.raises(FileSystemAccessError), + open( # noqa: PTH123 + "/etc/hosts", encoding="utf-8" + ) as handle, + ): + handle.read() + + +def test_making_directories_is_blocked_outside_the_sanctioned_path() -> None: + """The same applies to directory creation.""" + + with pytest.raises(FileSystemAccessError): + os.mkdir("/tmp/pyicloud-should-not-exist") # noqa: PTH102 + + with pytest.raises(FileSystemAccessError): + os.makedirs("/tmp/pyicloud-should-not-exist/nested") # noqa: PTH103 + + +def test_os_open_is_blocked_outside_the_sanctioned_path() -> None: + """`os.open` is guarded separately from `builtins.open`.""" + + with pytest.raises(FileSystemAccessError): + os.open("/etc/hosts", os.O_RDONLY) + + +def test_chmod_is_blocked_outside_the_sanctioned_path() -> None: + """The fifth guarded entry point, and the easiest to forget.""" + + with pytest.raises(FileSystemAccessError): + os.chmod("/etc/hosts", 0o644) # noqa: PTH101 + + +def test_the_sanctioned_path_is_an_escape_hatch_not_a_loophole() -> None: + """`python-test-results` paths are permitted, by design. + + `tests/test_cmdline.py` relies on this for its session directories. A + reviewer reading only "new tests must mock any file I/O" would call that a + violation; it is the guard working as intended. + """ + + target = _allowed_path("guard-check") + os.makedirs(target, exist_ok=True) # noqa: PTH103 + assert ALLOWED_MARKER in target + + +def test_pathlib_reads_are_not_intercepted() -> None: + """`Path.read_text` goes through `io.open`, which the guard does not patch. + + This is why loading a JSON fixture works, and it works inside a test body + as well as at import time -- so the ordering of the session-scoped fixtures + is not the whole explanation. Seven test modules depend on this. + """ + + conftest = Path(__file__).resolve().parent / "conftest.py" + + content = conftest.read_text(encoding="utf-8") + + assert "FileSystemAccessError" in content