From 556012e4094d995062f3012a36d04666b0a104ca Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Mon, 4 Aug 2025 15:50:32 +0200 Subject: [PATCH] functional2: forbid chdir and setting environment These are a footgun and are not acceptable in functional2 due to thread safety, effects on other tests, etc. Co-authored-by: Commentator2.0 Change-Id: I8d7285061eaa9bab27edd52f3646024c8cf605e5 --- doc/manual/src/contributing/testing.md | 6 +++ .../commands/test_custom_sub_commands.py | 4 -- tests/functional2/pyproject.toml | 37 ++++++++++++++++- tests/functional2/testlib/environ.py | 40 +++++++++++++++++++ tests/functional2/testlib/fixtures/command.py | 1 - tests/functional2/testlib/fixtures/env.py | 9 +++-- .../testlib/fixtures/pytest_command.py | 1 - .../functional2/testlib/fixtures/snapshot.py | 7 ++-- .../testlib/fixtures/test_snapshot.py | 1 + tests/functional2/testlib/utils.py | 6 +-- 10 files changed, 93 insertions(+), 19 deletions(-) create mode 100644 tests/functional2/testlib/environ.py diff --git a/doc/manual/src/contributing/testing.md b/doc/manual/src/contributing/testing.md index 3e19e33ae..0e2969781 100644 --- a/doc/manual/src/contributing/testing.md +++ b/doc/manual/src/contributing/testing.md @@ -62,6 +62,12 @@ For `installcheck` specifically, first run `just install` before running the tes Finer-grained filtering within a test suite is also possible using the [--gtest_filter](https://google.github.io/googletest/advanced.html#running-a-subset-of-the-tests) command-line option to a test suite executable, or the `GTEST_FILTER` environment variable. +### Inspecting failures + +The test suite emits logs in `build/meson-logs/`; the full textual failure logs are in `build/meson-logs/testlog.txt`. + +If you want a much nicer experience of viewing the logs in a structured manner, use `xunit-viewer --results build/meson-logs/testlog.junit.xml --server` to view them in a web browser. + ### Unit test support libraries There are headers and code which are not just used to test the library in question, but also downstream libraries. diff --git a/tests/functional2/commands/test_custom_sub_commands.py b/tests/functional2/commands/test_custom_sub_commands.py index 9df28920b..6ac7ea3f8 100644 --- a/tests/functional2/commands/test_custom_sub_commands.py +++ b/tests/functional2/commands/test_custom_sub_commands.py @@ -1,12 +1,8 @@ import stat import sys - import pytest - from pathlib import Path - from textwrap import dedent - from functional2.testlib.fixtures.env import ManagedEnv from functional2.testlib.fixtures.nix import Nix diff --git a/tests/functional2/pyproject.toml b/tests/functional2/pyproject.toml index ad9041ee9..9f0b0357b 100644 --- a/tests/functional2/pyproject.toml +++ b/tests/functional2/pyproject.toml @@ -102,6 +102,8 @@ task-tags = ["TODO", "FIXME", "XXX"] # SIM2: double negation in comparasions (`not a == b` instead of `a != b` etc) # SIM3: yoda conditions use `foo == "Foo"` instead of `"Foo" == foo` # SIM9: defaults for get and zip +# TID: banning of certain imports +# TID251: ban certain APIs # TD: enforce TODO comment style # disabled TD001: allow FIXME and XXX comments # disabled TD003: todos don't require an explicit issue link for us @@ -151,7 +153,6 @@ task-tags = ["TODO", "FIXME", "XXX"] # PYI: things for pyi files # SLF: accessing of private members # SLOT: subclassing of builtin-types (str, tuple, namedtuple) related issues -# TID: banning of certain imports # TC: typechecking imports (typechecking imports required to be inside of a `if TYPECHECKING` block # FLY: string joins # I: import sorting; covered by other rulesets @@ -162,7 +163,7 @@ task-tags = ["TODO", "FIXME", "XXX"] # PGH: things about pygrep, we don't use # FURB: covered by other rule sets # TRY: try and raise related things, not helpful as we only do testing -select = ["E4", "E7", "E9", "F", "ERA", "ASYNC", "ANN0", "ANN2", "A", "C4", "EM", "ISC", "INP", "LOG", "G", "PIE", "T20", "PT", "Q", "RSE", "RET", "SIM", "TD", "ARG", "PTH", "N", "PERF", "PLC", "PLE", "UP", "RUF"] +select = ["E4", "E7", "E9", "F", "ERA", "ASYNC", "ANN0", "ANN2", "A", "C4", "EM", "ISC", "INP", "LOG", "G", "PIE", "T20", "PT", "Q", "RSE", "RET", "SIM", "TID251", "TD", "ARG", "PTH", "N", "PERF", "PLC", "PLE", "UP", "RUF"] ignore = ["ANN002", "ANN003", "TD001", "TD003", "PLE1", "RUF005"] [tool.ruff.lint.per-file-ignores] @@ -191,3 +192,35 @@ suppress-none-returning = true [tool.ruff.lint.flake8-errmsg] # Allow raw strings to be up to 10 characters in error messages max-string-length = 10 + +[tool.ruff.lint.flake8-tidy-imports] + +[tool.ruff.lint.flake8-tidy-imports.banned-api] +# Forbid symbols that are footguns in our codebase due to unsafe global state +"os.environ".msg = """ +Environment variables are process wide and are unsafe to set as they interfere with other tests. Instead, use: + - env= in subprocess functions + - functional2.testlib.commands.Command.with_env for starting subprocesses + - functional2.testlib.environ for read-only access +""" +"os.environb".msg = """ +Environment variables are process wide and are unsafe to set as they interfere with other tests. Instead, use: + - env= in subprocess functions + - functional2.testlib.commands.Command.with_env for starting subprocesses + - functional2.testlib.environ for read-only access +""" +"os.putenv".msg = """ +Environment variables are process wide and are unsafe to set as they interfere with other tests. Instead, use: + - env= in subprocess functions + - functional2.testlib.commands.Command.with_env for starting subprocesses + - functional2.testlib.environ for read-only access +""" +"os.unsetenv".msg = """ +Environment variables are process wide and are unsafe to set as they interfere with other tests. Instead, use: + - env= in subprocess functions + - functional2.testlib.commands.Command.with_env for starting subprocesses + - functional2.testlib.environ for read-only access +""" +"os.chdir".msg = "Changing current directory is process wide and may interfere with other tests. Use cwd= while spawning a process instead." +"os.fchdir".msg = "Changing current directory is process wide and may interfere with other tests. Use cwd= while spawning a process instead." +"contextlib.chdir".msg = "Changing current directory is process wide and may interfere with other tests. Use cwd= while spawning a process instead." diff --git a/tests/functional2/testlib/environ.py b/tests/functional2/testlib/environ.py new file mode 100644 index 000000000..b13b02d03 --- /dev/null +++ b/tests/functional2/testlib/environ.py @@ -0,0 +1,40 @@ +""" +Safe (read-only) environment access. This exists entirely so that we can ban +os.environ in our codebase. +""" + +import collections.abc +import os +from typing import TypeVar, overload +from collections.abc import Iterator + +_T = TypeVar("_T") + + +class ReadonlyDict(collections.abc.Mapping[str, str]): + def __init__(self, inner: os._Environ[str]): + self.inner = inner + + def __getitem__(self, key: str) -> str: + return self.inner[key] + + def __iter__(self) -> Iterator[str]: + return iter(self.inner) + + def __len__(self) -> int: + return len(self.inner) + + @overload + def get(self, key: str, /) -> str | None: ... + @overload + def get(self, key: str, /, default: str | _T) -> str | _T: ... + + def get(self, key: str, /, default: str | _T = None) -> str | _T: + return self.inner.get(key, default=default) + + def copy(self) -> dict[str, str]: + return self.inner.copy() + + +# SAFETY: wrapped to not allow mutation +environ = ReadonlyDict(os.environ) # noqa: TID251 diff --git a/tests/functional2/testlib/fixtures/command.py b/tests/functional2/testlib/fixtures/command.py index 756400f49..ebb49022f 100644 --- a/tests/functional2/testlib/fixtures/command.py +++ b/tests/functional2/testlib/fixtures/command.py @@ -11,7 +11,6 @@ import pytest from functional2.testlib.fixtures.env import ManagedEnv from functional2.testlib.terminal_code_eater import eat_terminal_codes - logger = logging.getLogger(__name__) diff --git a/tests/functional2/testlib/fixtures/env.py b/tests/functional2/testlib/fixtures/env.py index 0bdf39555..b4e8ad92b 100644 --- a/tests/functional2/testlib/fixtures/env.py +++ b/tests/functional2/testlib/fixtures/env.py @@ -1,10 +1,11 @@ import dataclasses import logging -import os import platform import shutil from pathlib import Path +from functional2.testlib.environ import environ + import pytest SLASHES_IN_STORE_PATH_UNTIL_PACKAGE = "/nix/store/hash-program_name/".count("/") @@ -149,15 +150,15 @@ class _Dirs: class ManagedEnv: def __init__(self, tmp_path: Path): # Things fetched from the global env - build_shell = os.environ.get("BUILD_TEST_SHELL") - global_path = os.environ.get("PATH") + build_shell = environ.get("BUILD_TEST_SHELL") + global_path = environ.get("PATH") # `NIX_BIN_DIR` either propagated from us or set by meson # Set to the codebase internal output if started standalone # This is where the current lix binaries are located. # local import to avoid cyclic dependencies from functional2.testlib.utils import lix_base_folder # noqa: PLC0415 - lix_bin = Path(os.environ.get("NIX_BIN_DIR", lix_base_folder / "outputs/out/bin")) + lix_bin = Path(environ.get("NIX_BIN_DIR", lix_base_folder / "outputs/out/bin")) self._env = {} self.path = _ManagedPath(build_shell) diff --git a/tests/functional2/testlib/fixtures/pytest_command.py b/tests/functional2/testlib/fixtures/pytest_command.py index 4839419a1..984caa37d 100644 --- a/tests/functional2/testlib/fixtures/pytest_command.py +++ b/tests/functional2/testlib/fixtures/pytest_command.py @@ -1,6 +1,5 @@ import pytest from _pytest.fixtures import FixtureRequest - from functional2.testlib.fixtures.command import Command from functional2.testlib.fixtures.env import ManagedEnv diff --git a/tests/functional2/testlib/fixtures/snapshot.py b/tests/functional2/testlib/fixtures/snapshot.py index 943b056b8..efc93878f 100644 --- a/tests/functional2/testlib/fixtures/snapshot.py +++ b/tests/functional2/testlib/fixtures/snapshot.py @@ -1,4 +1,3 @@ -import os from collections.abc import Callable from logging import Logger from pathlib import Path @@ -9,6 +8,8 @@ import pytest from _pytest.config import Config from _pytest.fixtures import FixtureRequest +from functional2.testlib.environ import environ + def pytest_addoption(parser: pytest.Parser) -> None: """ @@ -26,9 +27,7 @@ def pytest_addoption(parser: pytest.Parser) -> None: @pytest.fixture(scope="session") def do_snapshot_update(request: FixtureRequest) -> bool: - return request.config.getoption("accept-tests") or ( - os.environ.get("_NIX_TEST_ACCEPT") is not None - ) + return request.config.getoption("accept-tests") or (environ.get("_NIX_TEST_ACCEPT") is not None) class Snapshot: diff --git a/tests/functional2/testlib/fixtures/test_snapshot.py b/tests/functional2/testlib/fixtures/test_snapshot.py index b21d19d15..bc6a7483b 100644 --- a/tests/functional2/testlib/fixtures/test_snapshot.py +++ b/tests/functional2/testlib/fixtures/test_snapshot.py @@ -25,6 +25,7 @@ def _get_f2_snapshot_files(additional_files: FileDeclaration) -> FileDeclaration "functional2": { "testlib": { "__init__.py": CopyFile("../__init__.py"), + "environ.py": CopyFile("../environ.py"), "fixtures": { "__init__.py": CopyFile("__init__.py"), "snapshot.py": CopyFile("snapshot.py"), diff --git a/tests/functional2/testlib/utils.py b/tests/functional2/testlib/utils.py index 07343c60a..cd6674a8d 100644 --- a/tests/functional2/testlib/utils.py +++ b/tests/functional2/testlib/utils.py @@ -1,11 +1,11 @@ import builtins -import os import types import typing from pathlib import Path from types import UnionType from typing import Any, Literal, get_args, get_origin +from functional2.testlib.environ import environ from functional2.testlib.fixtures.file_helper import ( CopyFile, CopyTree, @@ -141,9 +141,9 @@ def get_global_asset(name: str) -> Fileish: return CopyTemplate( functional2_base_folder / "testlib" / "global_assets" / "config.nix.template", { - "system": os.environ.get("system"), # noqa: SIM112 # system is actually lowercase here + "system": environ.get("system"), # Either just the build shell or entire global path if we are darwin - "path": os.environ.get("BUILD_TEST_SHELL") or os.environ.get("PATH"), + "path": environ.get("BUILD_TEST_SHELL") or environ.get("PATH"), }, ) return CopyFile(functional2_base_folder / "testlib" / "global_assets" / name)