diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e82d65fd..d950b7355 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -101,10 +101,19 @@ jobs: done python-tests: - name: Python tests (pytest) + name: Python tests (pytest ${{ matrix.shard }}) # Keep the namespace/AppArmor setup tied to the audited Ubuntu release. runs-on: ubuntu-24.04 # Make Python test validation authoritative for the configured scope. + strategy: + # Report every failing section in one run instead of cancelling the rest + # the moment one shard goes red. + fail-fast: false + matrix: + # Shards partition the suite by test file, so the four together run + # every test exactly once. tests/_shards.py owns the partition and + # tests/test_shards.py pins this list to its DEFAULT_SHARD_COUNT. + shard: ["1/4", "2/4", "3/4", "4/4"] steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -210,5 +219,8 @@ jobs: print("Runtime bubblewrap PID/mount namespace probe passed.") PY - - run: python -m pytest -q -rs + - name: pytest (shard ${{ matrix.shard }}) if: steps.docs-check.outputs.docs_only != 'true' + env: + PYTEST_SHARD: ${{ matrix.shard }} + run: python -m pytest -q -rs --shard "$PYTEST_SHARD" diff --git a/tests/README.md b/tests/README.md index 1c12cf15a..62e1036cf 100644 --- a/tests/README.md +++ b/tests/README.md @@ -93,6 +93,39 @@ fast lane; the test stays runnable directly, e.g.: ./venv/bin/python -m pytest -m slow ``` +## Parallel shards (`--shard N/M`) + +CI no longer runs the whole suite as one workload. The `python-tests` job is a +four-way matrix, and each job runs one section: + +```bash +./venv/bin/python -m pytest -q --shard 1/4 +``` + +`tests/_shards.py` owns the partition and `tests/conftest.py` applies it. The +unit of a shard is a **test file**, so tests that share module state stay +together, and assignment is a total function of the file path - every file +lands in exactly one shard, and the four shards together run every test exactly +once. The partition is deliberately *not* built on the `area_*` markers: those +do not partition the suite, because a file may carry a hand-applied `area_*` +mark on top of the one derived from its filename. + +Sharding deselects; it does not narrow collection. Every test module is still +imported, in the same order, in every shard, so the import-time stubbing in +`conftest.py` behaves identically whether the suite runs whole or in sections. +Only the deselected tests' call phase is skipped. + +Balance comes from the `slow` marker: a `slow` item is weighted far above an +ordinary one, and files are packed heaviest-first into the lightest shard. The +plan depends only on the collected file set, so every parallel job computes the +same one from the same commit. As more tests earn a `slow` mark from duration +evidence, the sections even out further - no duration table to keep current. + +`--shard 1/1` is a no-op, and a selector that is malformed or out of range ends +the run with a usage error rather than quietly testing a subset. If you change +the shard count, change `DEFAULT_SHARD_COUNT` and the `ci.yml` matrix together; +`tests/test_shards.py` fails when they drift apart. + ## Order-sensitivity reporting (report-only) `tests/run_order_report.py` runs pytest with the collected test items shuffled diff --git a/tests/_shards.py b/tests/_shards.py new file mode 100644 index 000000000..f21f82380 --- /dev/null +++ b/tests/_shards.py @@ -0,0 +1,156 @@ +"""Deterministic, balanced partition of the test suite into parallel shards. + +The suite runs ~7.8k tests in a single pytest workload. This module splits that +workload into N sections that CI runs as parallel jobs, so wall-clock time is +bounded by the slowest section rather than by the whole suite. + +Two properties matter more than speed, and both are structural here rather than +checked after the fact: + +* **Exhaustive and disjoint.** Shard assignment is a total function of the test + *file*, so every test file lands in exactly one shard. No test can be dropped + by a marker typo, and none runs twice. This is deliberately not built on the + ``area_*`` taxonomy markers: those are not a partition in practice, because a + test may also carry a hand-applied ``area_*`` mark on top of the one + ``tests/conftest.py`` derives from its filename (``test_hwfit_container_ + visibility_warning.py`` carries three). +* **Whole files stay together.** Tests in one file share module state and are + written to run in file order, so a file is the smallest unit a shard can hold. + +Balance uses the existing ``slow`` marker as its weight signal rather than a +committed duration table that would go stale silently. Packing is greedy +longest-processing-time-first, which is deterministic for a given file set - +every parallel job computes the identical plan from the same commit. + +This module imports nothing from the application or from pytest - only the +standard library - so the planner is directly unit-testable. The pytest wiring +lives in ``tests/conftest.py``. See ``tests/README.md``. +""" +from __future__ import annotations + +import re +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from pathlib import Path + +# Number of sections CI runs in parallel. Kept here as documentation of the +# intended default; the shard count actually used comes from the --shard value. +DEFAULT_SHARD_COUNT = 4 + +# Relative cost of one test item. Measured on dev at 2026-10-02 over a full +# `pytest -q --durations=25` run: the five `slow`-marked items average 9.0 s +# each and the remaining 7789 items average 0.018 s, a ratio of roughly 500. +# Exact values do not matter - only that a `slow` item outweighs a whole +# ordinary file, so the packer spreads the slow ones across sections first. +DEFAULT_ITEM_WEIGHT = 1.0 +SLOW_ITEM_WEIGHT = 500.0 + +_SHARD_SPEC_PATTERN = re.compile(r"\A(\d+)/(\d+)\Z") + + +class ShardSpecError(ValueError): + """Raised when a ``--shard`` value is not a usable ``N/M`` selector.""" + + +@dataclass(frozen=True) +class ShardSpec: + """A one-based shard selector: shard ``index`` of ``count``.""" + + index: int + count: int + + @property + def selects_everything(self) -> bool: + """True when the selector is a no-op (``1/1``) and nothing is deselected.""" + return self.count == 1 + + def __str__(self) -> str: + return f"{self.index}/{self.count}" + + +def parse_shard_spec(value: str) -> ShardSpec: + """Parse ``"N/M"`` into a :class:`ShardSpec`. + + Rejects anything that would silently run the wrong subset: a malformed + value, a zero or negative part, or an index past the shard count. Surrounding + whitespace is tolerated because CI passes the value through a shell variable. + """ + match = _SHARD_SPEC_PATTERN.match(value.strip()) + if match is None: + raise ShardSpecError( + f"invalid shard {value!r}: expected N/M, e.g. 1/{DEFAULT_SHARD_COUNT}" + ) + index, count = int(match.group(1)), int(match.group(2)) + if count < 1: + raise ShardSpecError(f"invalid shard {value!r}: shard count must be >= 1") + if not 1 <= index <= count: + raise ShardSpecError( + f"invalid shard {value!r}: shard index must be between 1 and {count}" + ) + return ShardSpec(index=index, count=count) + + +def item_weight(is_slow: bool) -> float: + """Weight of a single test item, by whether it carries the ``slow`` marker.""" + return SLOW_ITEM_WEIGHT if is_slow else DEFAULT_ITEM_WEIGHT + + +def accumulate_file_weights(entries: Iterable[tuple[str, bool]]) -> dict[str, float]: + """Sum per-item weights into a per-file total. + + ``entries`` yields ``(file_key, is_slow)`` for each collected test item, so a + file's weight reflects both how many tests it holds and how slow they are. + """ + weights: dict[str, float] = {} + for file_key, is_slow in entries: + weights[file_key] = weights.get(file_key, 0.0) + item_weight(is_slow) + return weights + + +def plan_shards( + file_weights: Mapping[str, float], count: int +) -> tuple[frozenset[str], ...]: + """Partition the files of ``file_weights`` into ``count`` balanced shards. + + Greedy longest-processing-time-first: heaviest file first, each one placed in + the lightest shard so far. Ties break on the file key and then on the lowest + shard index, so the plan depends only on the input and is identical in every + parallel job. Returns one frozenset per shard, in shard order; shards may be + empty when there are fewer files than shards. + """ + if count < 1: + raise ShardSpecError(f"shard count must be >= 1, got {count}") + buckets: list[set[str]] = [set() for _ in range(count)] + loads = [0.0] * count + # Heaviest first, with the file key as a deterministic tie-break. + ordered = sorted(file_weights.items(), key=lambda item: (-item[1], item[0])) + for file_key, weight in ordered: + target = min(range(count), key=lambda index: (loads[index], index)) + buckets[target].add(file_key) + loads[target] += weight + return tuple(frozenset(bucket) for bucket in buckets) + + +def shard_loads( + file_weights: Mapping[str, float], plan: tuple[frozenset[str], ...] +) -> tuple[float, ...]: + """Total weight of each shard in ``plan`` - the balance the packer achieved.""" + return tuple( + sum(file_weights[file_key] for file_key in bucket) for bucket in plan + ) + + +def relative_file_key(path: str | Path, root: str | Path | None = None) -> str: + """Stable per-file key: the posix path relative to ``root`` when possible. + + Falls back to the absolute posix path when ``path`` lies outside ``root`` or + the relationship cannot be resolved, which keeps the key defined for every + collected item rather than dropping one from the plan. + """ + resolved = Path(path) + if root is not None: + try: + return resolved.resolve().relative_to(Path(root).resolve()).as_posix() + except (OSError, ValueError): + pass + return resolved.as_posix() diff --git a/tests/conftest.py b/tests/conftest.py index cdc3129d9..4f3eb2c07 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -62,6 +62,37 @@ if "src.database" not in sys.modules: # collection, which breaks session import in subsequent tests). import core.models # noqa: E402 +def pytest_addoption(parser): + """Add ``--shard N/M`` so CI can run the suite as parallel sections.""" + group = parser.getgroup("sharding", "parallel test sharding") + group.addoption( + "--shard", + action="store", + default=None, + metavar="N/M", + help=( + "run only shard N of M (1-based), e.g. --shard 1/4. Shards partition " + "the suite by test file, so together they run every test exactly " + "once. See tests/_shards.py." + ), + ) + + +def _shard_spec(config): + """Parse the ``--shard`` option into a ShardSpec, or None when unset.""" + from tests._shards import ShardSpecError, parse_shard_spec + + value = config.getoption("shard") + if value is None: + return None + try: + return parse_shard_spec(value) + except ShardSpecError as error: + # UsageError fails the run immediately rather than silently running a + # subset nobody asked for - a dropped shard is invisible in a green CI. + raise pytest.UsageError(str(error)) from error + + def pytest_configure(config): """Register the dynamic taxonomy ``sub_*`` markers before collection. @@ -79,15 +110,24 @@ def pytest_configure(config): if marker_name.startswith("sub_"): config.addinivalue_line("markers", f"{marker_name}: taxonomy sub-area marker") + # Validate --shard before collection so a bad selector fails the run up + # front instead of after a few minutes of collecting. + _shard_spec(config) + def pytest_collection_modifyitems(config, items): - """Tag each collected test with its taxonomy ``area_*`` and ``sub_*`` markers. + """Tag each collected test with its taxonomy markers, then apply ``--shard``. - Collection-time only: this adds markers and nothing else. It does not skip, - reorder, or deselect tests, mutate fixtures or the environment, or import any + Tagging is collection-time only: it adds markers and nothing else. It does + not skip, reorder, mutate fixtures or the environment, or import any production module. See ``tests/_taxonomy.py`` for the classification rules. + + Sharding deselects the test files that belong to another shard. It runs + after collection, so every test module is still imported, in the same order, + in every shard - the import-time stubbing above behaves identically whether + the suite runs whole or as one section of it. Only the deselected tests' + call phase is skipped. See ``tests/_shards.py`` for the partition. """ - import pytest from tests._taxonomy import markers_for_path for item in items: @@ -95,6 +135,30 @@ def pytest_collection_modifyitems(config, items): for marker_name in markers_for_path(path): item.add_marker(getattr(pytest.mark, marker_name)) + spec = _shard_spec(config) + if spec is None or spec.selects_everything: + return + + from tests._shards import accumulate_file_weights, plan_shards, relative_file_key + + root = getattr(config, "rootpath", None) + keys = [ + relative_file_key(getattr(item, "path", None) or item.fspath, root) + for item in items + ] + weights = accumulate_file_weights( + (key, item.get_closest_marker("slow") is not None) + for key, item in zip(keys, items) + ) + selected_files = plan_shards(weights, spec.count)[spec.index - 1] + + selected, deselected = [], [] + for key, item in zip(keys, items): + (selected if key in selected_files else deselected).append(item) + if deselected: + config.hook.pytest_deselected(items=deselected) + items[:] = selected + @pytest.fixture(scope="session", autouse=True) def _serve_test_static(): diff --git a/tests/test_shards.py b/tests/test_shards.py new file mode 100644 index 000000000..2e36e0092 --- /dev/null +++ b/tests/test_shards.py @@ -0,0 +1,214 @@ +"""Unit tests for tests/_shards.py - the parallel shard planner. + +These pin the partition guarantees directly, without running pytest collection: +every test file lands in exactly one shard, the plan is identical in every +parallel job, and a bad ``--shard`` value is rejected rather than quietly +running a subset. They import only the module under test (a test-support +module, not production code) and touch no filesystem. +""" +from pathlib import Path + +import pytest + +from tests._shards import ( + DEFAULT_ITEM_WEIGHT, + DEFAULT_SHARD_COUNT, + SLOW_ITEM_WEIGHT, + ShardSpec, + ShardSpecError, + accumulate_file_weights, + item_weight, + parse_shard_spec, + plan_shards, + relative_file_key, + shard_loads, +) + + +def even_weights(count, weight=1.0): + """``count`` file keys of equal weight, named so sort order is stable.""" + return {f"tests/test_{index:03d}.py": weight for index in range(count)} + + +# --- parse_shard_spec -------------------------------------------------------- + +def test_parse_accepts_a_simple_selector(): + assert parse_shard_spec("2/4") == ShardSpec(index=2, count=4) + + +def test_parse_tolerates_surrounding_whitespace_from_a_shell_variable(): + assert parse_shard_spec(" 3/4\n") == ShardSpec(index=3, count=4) + + +@pytest.mark.parametrize("value", ["", "abc", "1", "1/", "/4", "1/4/4", "1-4", "1 / 4"]) +def test_parse_rejects_malformed_selectors(value): + with pytest.raises(ShardSpecError): + parse_shard_spec(value) + + +@pytest.mark.parametrize("value", ["0/4", "5/4", "-1/4", "1/0"]) +def test_parse_rejects_out_of_range_selectors(value): + with pytest.raises(ShardSpecError): + parse_shard_spec(value) + + +def test_parse_error_names_the_offending_value(): + with pytest.raises(ShardSpecError, match="9/4"): + parse_shard_spec("9/4") + + +def test_single_shard_selects_everything_and_larger_counts_do_not(): + assert parse_shard_spec("1/1").selects_everything is True + assert parse_shard_spec("1/2").selects_everything is False + + +def test_spec_renders_as_the_selector_it_came_from(): + assert str(parse_shard_spec("3/4")) == "3/4" + + +# --- weights ----------------------------------------------------------------- + +def test_a_slow_item_outweighs_an_ordinary_one(): + assert item_weight(is_slow=True) == SLOW_ITEM_WEIGHT + assert item_weight(is_slow=False) == DEFAULT_ITEM_WEIGHT + assert SLOW_ITEM_WEIGHT > DEFAULT_ITEM_WEIGHT + + +def test_file_weight_sums_the_items_in_that_file(): + weights = accumulate_file_weights([ + ("tests/test_a.py", False), + ("tests/test_a.py", False), + ("tests/test_b.py", True), + ]) + assert weights == { + "tests/test_a.py": 2 * DEFAULT_ITEM_WEIGHT, + "tests/test_b.py": SLOW_ITEM_WEIGHT, + } + + +def test_file_weights_of_an_empty_collection_are_empty(): + assert accumulate_file_weights([]) == {} + + +# --- plan_shards: the partition guarantees ----------------------------------- + +@pytest.mark.parametrize("count", [1, 2, 3, 4, 5, 8]) +def test_every_file_lands_in_exactly_one_shard(count): + weights = even_weights(37) + plan = plan_shards(weights, count) + + assert len(plan) == count + placements = [key for bucket in plan for key in bucket] + assert sorted(placements) == sorted(weights) + assert len(placements) == len(set(placements)) + + +def test_a_single_shard_holds_the_whole_suite(): + weights = even_weights(10) + assert plan_shards(weights, 1) == (frozenset(weights),) + + +def test_the_plan_is_identical_for_the_same_input(): + weights = even_weights(50) + assert plan_shards(weights, 4) == plan_shards(weights, 4) + + +def test_the_plan_does_not_depend_on_file_insertion_order(): + keys = list(even_weights(20)) + forward = plan_shards({key: 1.0 for key in keys}, 4) + reversed_order = plan_shards({key: 1.0 for key in reversed(keys)}, 4) + assert forward == reversed_order + + +def test_equal_weights_are_spread_evenly(): + weights = even_weights(40) + plan = plan_shards(weights, 4) + assert [len(bucket) for bucket in plan] == [10, 10, 10, 10] + + +def test_shards_may_be_empty_when_files_are_scarcer_than_shards(): + plan = plan_shards(even_weights(2), 4) + assert sorted(len(bucket) for bucket in plan) == [0, 0, 1, 1] + + +def test_an_empty_suite_still_yields_the_requested_number_of_shards(): + assert plan_shards({}, 3) == (frozenset(), frozenset(), frozenset()) + + +@pytest.mark.parametrize("count", [0, -1]) +def test_plan_rejects_a_nonsensical_shard_count(count): + with pytest.raises(ShardSpecError): + plan_shards(even_weights(4), count) + + +# --- plan_shards: balance ---------------------------------------------------- + +def test_a_heavy_file_is_offset_by_giving_its_shard_fewer_others(): + # The real shape of the suite: one file of `slow` tests worth about a + # quarter of the total, and a long tail of ordinary files. + weights = {"tests/test_heavy.py": 100.0, **even_weights(300)} + plan = plan_shards(weights, 4) + + assert shard_loads(weights, plan) == (100.0, 100.0, 100.0, 100.0) + heavy_shard = next(i for i, b in enumerate(plan) if "tests/test_heavy.py" in b) + assert len(plan[heavy_shard]) == 1 + + +def test_a_file_heavier_than_an_even_share_sets_the_floor_alone(): + # A shard cannot be lighter than its heaviest file, so the packer stops + # adding to that shard rather than balancing the others against it. + weights = {"tests/test_heavy.py": 300.0, **even_weights(300)} + plan = plan_shards(weights, 4) + loads = shard_loads(weights, plan) + + assert max(loads) == 300.0 + heavy_shard = next(i for i, b in enumerate(plan) if "tests/test_heavy.py" in b) + assert plan[heavy_shard] == frozenset({"tests/test_heavy.py"}) + others = [load for i, load in enumerate(loads) if i != heavy_shard] + assert max(others) - min(others) <= 1.0 + + +def test_the_heaviest_files_are_placed_in_different_shards(): + weights = {f"tests/test_slow_{index}.py": 500.0 for index in range(4)} + weights.update(even_weights(100)) + plan = plan_shards(weights, 4) + + for index in range(4): + holders = [bucket for bucket in plan if f"tests/test_slow_{index}.py" in bucket] + assert len(holders) == 1 + assert all( + sum(1 for key in bucket if key.startswith("tests/test_slow_")) == 1 + for bucket in plan + ) + + +def test_shard_loads_account_for_every_file(): + weights = {"tests/test_a.py": 2.0, "tests/test_b.py": 3.0, "tests/test_c.py": 5.0} + assert sum(shard_loads(weights, plan_shards(weights, 2))) == 10.0 + + +# --- relative_file_key ------------------------------------------------------- + +def test_key_is_relative_to_the_repository_root(): + assert relative_file_key("/repo/tests/test_a.py", "/repo") == "tests/test_a.py" + + +def test_key_falls_back_to_the_full_path_outside_the_root(): + assert relative_file_key("/elsewhere/test_a.py", "/repo") == "/elsewhere/test_a.py" + + +def test_key_without_a_root_is_the_path_as_given(): + assert relative_file_key("tests/test_a.py") == "tests/test_a.py" + + +# --- the default the CI matrix is written against ---------------------------- + +def test_default_shard_count_matches_the_ci_matrix(): + """A matrix that drifts from the default silently stops running a shard.""" + workflow = Path(__file__).resolve().parents[1] / ".github" / "workflows" / "ci.yml" + text = workflow.read_text(encoding="utf-8") + for index in range(1, DEFAULT_SHARD_COUNT + 1): + assert f'"{index}/{DEFAULT_SHARD_COUNT}"' in text, ( + f"ci.yml does not run shard {index}/{DEFAULT_SHARD_COUNT}" + ) + assert f'"{DEFAULT_SHARD_COUNT + 1}/' not in text