diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9a3fb927c..6aa601e264 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,6 +73,16 @@ jobs: with: slice_count: 12 + # macOS + Windows lanes. The main `tests` lane above is Linux-only, and + # the OS-marked tests it collects are skipped there by design (see the + # `_OS_MARKS` comment in tests/conftest.py) — this is where they run. + # Same `python` lane gate: if no Python changed, neither runs. + tests-os: + name: OS-specific tests + needs: detect + if: needs.detect.outputs.python == 'true' + uses: ./.github/workflows/tests-os.yml + lint: name: Python lints needs: detect @@ -192,6 +202,7 @@ jobs: needs: - detect - tests + - tests-os - lint - js-tests - installer-tests diff --git a/.github/workflows/tests-os.yml b/.github/workflows/tests-os.yml new file mode 100644 index 0000000000..921f092d15 --- /dev/null +++ b/.github/workflows/tests-os.yml @@ -0,0 +1,147 @@ +name: OS-specific tests + +# Runs the tests that can only be trusted on their own host OS. +# +# The main Python suite (.github/workflows/tests.yml) runs on +# ubuntu-latest and covers everything that is either platform-agnostic or +# genuinely Linux-specific. Tests whose subject is macOS- or +# Windows-specific behaviour carry a marker (see the ``_OS_MARKS`` block +# comment in tests/conftest.py) and are SKIPPED on Linux, because faking +# ``sys.platform`` on a Linux runner selects the branch under test without +# reproducing any of the OS behaviour that branch exists for. This workflow +# is where those markers actually execute: +# +# macos → ``-m macos_only`` on macos-latest +# windows → ``-m windows_only`` on windows-latest +# +# Deliberately NOT sliced. The marked set is small (tens of tests, not +# thousands), so one plain ``pytest`` process per OS is both faster and far +# less machinery than the LPT-sliced per-file runner the Linux lane needs. +# If either lane grows past its timeout, that is the signal to reach for +# scripts/run_tests.sh --slice here too. +# +# Each lane FAILS when it selects zero tests (pytest exit code 5). Without +# that guard, a renamed marker or a bad selector would report a green job +# that ran nothing — the exact silent-coverage-loss failure this workflow +# exists to prevent. + +on: + workflow_call: + +permissions: + contents: read + +concurrency: + group: tests-os-${{ github.ref }} + cancel-in-progress: true + +jobs: + os-tests: + name: ${{ matrix.name }} + runs-on: ${{ matrix.runner }} + timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + include: + - name: macOS-only tests + runner: macos-latest + marker: macos_only + - name: Windows-only tests + runner: windows-latest + marker: windows_only + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Install uv + uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # 8.2.0 + with: + # Pinned for the same reason as the Linux lane: unpinned, setup-uv + # resolves "latest" by fetching a manifest on every job and a + # transient fetch failure fails the whole job. + version: "0.9.28" + enable-cache: true + cache-dependency-glob: | + pyproject.toml + uv.lock + + - name: Set up Python 3.11 + run: uv python install 3.11 + + - name: Install dependencies + # Same extras as the Linux test lane so an OS-marked test can import + # anything its Linux siblings can. ``[all]`` is deliberately + # Windows/macOS-installable (see the policy comment on the extra in + # pyproject.toml — matrix/python-olm was removed from it precisely + # because it could not build here). + uses: ./.github/actions/retry + with: + command: uv sync --locked --python 3.11 --extra all --extra dev --extra anthropic --extra mistral --extra fal --extra modal --extra daytona --extra hindsight --extra parallel-web + + - name: Minimize uv cache + run: uv cache prune --ci + + - name: Run ${{ matrix.marker }} tests + # Two-step selection: + # + # 1. scripts/ci/list_os_marked_tests.py narrows WHICH FILES are + # imported. ``-m`` filters after collection, and collection + # imports every module under tests/ — on this host that would + # drag ~900 unrelated test modules through import, where a + # single unrelated ImportError would fail a job whose own + # subject is fine. The helper exits non-zero if the marker + # matches no file at all. + # 2. ``-m`` decides WHICH TESTS run, and stays authoritative. + # Passing it on the command line REPLACES pyproject's + # ``-m 'not integration'`` addopts (same option, last wins) — + # hence repeating ``not integration``, or the integration + # suite would return through the side door. + # + # ``--timeout-method`` needs no override: tests/conftest.py's + # pytest_configure already downgrades the signal-based timer on + # Windows, which has no SIGALRM. + shell: bash + run: | + set -uo pipefail + + LIST="${RUNNER_TEMP:-.}/selected-tests.txt" + + # Process substitution would hide the helper's exit status, so write + # to a file and check it explicitly. + if ! uv run --no-sync python scripts/ci/list_os_marked_tests.py \ + "${{ matrix.marker }}" > "$LIST"; then + echo "::error::could not enumerate ${{ matrix.marker }} test files" + exit 1 + fi + if [ ! -s "$LIST" ]; then + echo "::error::empty ${{ matrix.marker }} file list" + exit 1 + fi + + # Deliberately NOT `mapfile`: that is a bash 4 builtin and the macOS + # runner's /bin/bash is 3.2. Word-splitting is safe here because the + # helper emits repo-relative test paths, which contain no spaces. + # shellcheck disable=SC2046 + set -- $(cat "$LIST") + echo "selected $# file(s) for ${{ matrix.marker }}:" + cat "$LIST" + + uv run --no-sync python -m pytest \ + "$@" \ + -m "${{ matrix.marker }} and not integration" \ + -v --tb=short + status=$? + if [ "$status" -eq 5 ]; then + echo "::error::No tests matched -m ${{ matrix.marker }}. Either the" \ + "marker was renamed/dropped or selection is broken — this job" \ + "must never pass without running its OS's tests." + exit 1 + fi + exit "$status" + env: + # Belt-and-suspenders with tests/conftest.py's env blanking: no + # test may reach a real provider API. + OPENROUTER_API_KEY: "" + OPENAI_API_KEY: "" + NOUS_API_KEY: "" diff --git a/scripts/ci/list_os_marked_tests.py b/scripts/ci/list_os_marked_tests.py new file mode 100644 index 0000000000..bfbc0a60c1 --- /dev/null +++ b/scripts/ci/list_os_marked_tests.py @@ -0,0 +1,115 @@ +#!/usr/bin/env python3 +"""List the test files that carry a given OS marker. + +Used by ``.github/workflows/tests-os.yml`` to scope what the macOS and +Windows lanes import. + +Why scope at all, when ``pytest -m macos_only`` already selects correctly? +Because ``-m`` filters AFTER collection, and collection IMPORTS every test +module under ``tests/``. On the Linux lane that is fine (it runs them all +anyway), but on the macOS/Windows lanes it would drag ~900 unrelated modules +through import on a host they were never expected to import on — one +unrelated ImportError would fail a job whose actual subject passed. Narrowing +the paths keeps each lane's failure signal about its own tests. + +``-m`` is still passed by the workflow and remains the authoritative +selector: this script only decides which files get imported, never which +tests run. Over-selecting here is harmless (``-m`` drops the extras); the +failure mode to care about is UNDER-selecting, which is why the workflow +fails the job when zero tests end up selected. + +Usage: + python scripts/ci/list_os_marked_tests.py macos_only [tests_root] + +Prints one path per line (POSIX separators, repo-relative), sorted. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +_VALID_MARKERS = ("linux_only", "macos_only", "windows_only") + + +def find_marked_files(marker: str, root: Path) -> list[Path]: + """Return every ``test_*.py`` under *root* that references *marker*. + + Matches the marker as a whole word so ``macos_only`` doesn't pick up a + hypothetical ``macos_only_extra``. Catches both the decorator form + (``@pytest.mark.macos_only``, on a function or a class) and the + module-level ``pytestmark`` form. + """ + pattern = re.compile(rf"\b{re.escape(marker)}\b") + hits: list[Path] = [] + for path in sorted(root.rglob("test_*.py")): + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + if pattern.search(text): + hits.append(path) + return hits + + +def main(argv: list[str]) -> int: + if len(argv) < 2: + print(__doc__, file=sys.stderr) + return 2 + marker = argv[1] + if marker not in _VALID_MARKERS: + print( + f"error: unknown marker {marker!r} (expected one of " + f"{', '.join(_VALID_MARKERS)})", + file=sys.stderr, + ) + return 2 + + repo_root = Path(__file__).resolve().parents[2] + root = Path(argv[2]) if len(argv) > 2 else repo_root / "tests" + if not root.exists(): + print(f"error: no such directory: {root}", file=sys.stderr) + return 2 + + files = find_marked_files(marker, root) + if not files: + print( + f"error: no test file references @pytest.mark.{marker} — the marker " + "was probably renamed or dropped. Refusing to emit an empty list, " + "which would let the OS lane pass without running anything.", + file=sys.stderr, + ) + return 1 + + lines: list[str] = [] + for path in files: + # POSIX separators so the output is safe to paste into a bash + # command line on the Windows runner (Git Bash accepts them). + # + # Relative to the repo root when the path is inside it (the CI case — + # pytest is invoked from the repo root). A root outside the repo is a + # test/manual invocation; emit it as-is rather than raising, since + # ``relative_to`` refuses non-descendant paths. + try: + rel = path.resolve().relative_to(repo_root) + except ValueError: + lines.append(path.as_posix()) + else: + lines.append(rel.as_posix()) + + # Write bytes with explicit LF rather than print(), which on Windows + # translates "\n" to "\r\n" in text mode. The consumer reads this list with + # ``$(cat ...)`` in bash, and word splitting uses IFS (space/tab/newline) — + # a CR is NOT a separator, so it stays glued to each path and pytest then + # fails with "file or directory not found: tests/...py" for a path that + # looks correct in the log because the CR is invisible. Emitting bytes makes + # the output identical on every host instead of depending on the platform's + # newline translation. + sys.stdout.buffer.write(b"".join(line.encode("utf-8") + b"\n" for line in lines)) + sys.stdout.buffer.flush() + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/scripts/run_tests_parallel.py b/scripts/run_tests_parallel.py index 4fd1da44ce..96bec3c56a 100755 --- a/scripts/run_tests_parallel.py +++ b/scripts/run_tests_parallel.py @@ -44,6 +44,7 @@ from __future__ import annotations import argparse import json import os +import re import subprocess import sys import threading @@ -136,6 +137,41 @@ def _split_pathspec(value: str) -> List[str]: i += 1 return [p for p in parts if p.strip()] +# Host-OS gating (see the ``_OS_MARKS`` block in tests/conftest.py): tests +# marked for another host are collected and SKIPPED by the conftest hook — +# this runner never executes them, by construction. The summary calls that +# out explicitly so a local run isn't misread as covering macOS/Windows +# behaviour, and names the CI lane where those tests actually execute. +_OS_MARKERS = { + "linux_only": ("linux", "the main Linux CI lane"), + "macos_only": ("darwin", "the tests-os CI lane (macos-latest)"), + "windows_only": ("win32", "the tests-os CI lane (windows-latest)"), +} + + +def _off_host_marker_files(files: List[Path]) -> dict[str, int]: + """Count discovered files referencing each marker for an OS we are not on. + + Whole-word text match, same approach as scripts/ci/list_os_marked_tests.py: + over-counting a prose mention is harmless here (the note is informational); + what matters is never reporting 0 while gated tests exist. + """ + off_host = { + marker: re.compile(rf"\b{marker}\b") + for marker, (host_prefix, _) in _OS_MARKERS.items() + if not sys.platform.startswith(host_prefix) + } + counts = {marker: 0 for marker in off_host} + for path in files: + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + for marker, pattern in off_host.items(): + if pattern.search(text): + counts[marker] += 1 + return {marker: n for marker, n in counts.items() if n} + def _approximately_count_tests( files: List[Path], repo_root: Path @@ -421,8 +457,6 @@ def _parse_pytest_summary(output: str) -> dict[str, int]: Returns a dict with keys ``passed``, ``failed``, ``skipped``, ``errors``, ``xfailed``, ``xpassed`` (only keys found in the output are present). """ - import re - result: dict[str, int] = {} # Walk backwards from the end — the summary line is always near the tail. for line in reversed(output.splitlines()): @@ -1005,6 +1039,7 @@ def main() -> int: fail_count = 0 tests_passed = 0 tests_failed = 0 + tests_skipped = 0 # Every collected outcome, not just pass/fail: a legitimately all-skipped # (platform-gated) file reports "2 skipped" and must NOT trip the # nothing-ran guard, whereas a file that died before collection reports @@ -1013,7 +1048,7 @@ def main() -> int: lock = threading.Lock() def _on_done(file: Path, started_at: float, fut: "Future[Tuple[Path, int, str, Dict[str, int], float]]") -> None: - nonlocal files_done, tests_done, pass_count, fail_count, tests_passed, tests_failed + nonlocal files_done, tests_done, pass_count, fail_count, tests_passed, tests_failed, tests_skipped nonlocal tests_collected n_tests = test_counts.get(file, 0) try: @@ -1038,6 +1073,7 @@ def main() -> int: # Accumulate test-level counts from parsed summary. tests_passed += summary.get("passed", 0) tests_failed += summary.get("failed", 0) + tests_skipped += summary.get("skipped", 0) tests_collected += sum( summary.get(k, 0) for k in ("passed", "failed", "skipped", "errors", "xfailed", "xpassed") @@ -1078,7 +1114,22 @@ def main() -> int: elapsed = time.monotonic() - started print() pct = min(100, (tests_done / approx_total_tests * 100)) if approx_total_tests else 0 - print(f"=== Summary: {len(files)} files, {tests_passed} tests passed, {tests_failed} failed ({pct:.0f}% complete) in {elapsed:.1f}s ({args.jobs} workers) ===") + skipped_note = f", {tests_skipped} skipped" if tests_skipped else "" + print(f"=== Summary: {len(files)} files, {tests_passed} tests passed, {tests_failed} failed{skipped_note} ({pct:.0f}% complete) in {elapsed:.1f}s ({args.jobs} workers) ===") + + # Host-OS gating note: tests marked for another OS were skipped by the + # conftest hook, not run. Say so explicitly — a green local run on Linux + # proves nothing about the macos_only/windows_only tests, and the reader + # should know where they DO run rather than misreading skips as coverage. + off_host = _off_host_marker_files(files) + if off_host: + print() + for marker, n in sorted(off_host.items()): + _, lane = _OS_MARKERS[marker] + print( + f" note: {marker} tests (in {n} file{'s' if n != 1 else ''}) were " + f"SKIPPED on this host ({sys.platform}); they run on {lane}." + ) # Zero tests collected across the WHOLE run is NOT a pass. Per-file rc=5 # is deliberately tolerated above (platform-gated files), but if NOTHING diff --git a/tests/ci/test_list_os_marked_tests.py b/tests/ci/test_list_os_marked_tests.py new file mode 100644 index 0000000000..9b44a7596c --- /dev/null +++ b/tests/ci/test_list_os_marked_tests.py @@ -0,0 +1,136 @@ +"""Tests for ``scripts/ci/list_os_marked_tests.py``. + +The helper decides which files the macOS / Windows CI lanes import. Its +failure modes matter more than its happy path: if it silently returned an +empty list, the OS lane would run zero tests and still report green — the +exact silent-coverage-loss the lanes exist to prevent. So the contracts under +test are "finds real markers", "refuses to emit nothing", and "rejects an +unknown marker". +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +SCRIPT = REPO_ROOT / "scripts" / "ci" / "list_os_marked_tests.py" + + +def _run(*args: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(SCRIPT), *args], + capture_output=True, + text=True, + timeout=120, + cwd=REPO_ROOT, + ) + + +def _write(root: Path, relpath: str, body: str) -> Path: + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body, encoding="utf-8") + return path + + +@pytest.mark.parametrize("marker", ["linux_only", "macos_only", "windows_only"]) +def test_finds_decorator_and_pytestmark_forms(tmp_path, marker): + """Both the decorator form and module-level ``pytestmark`` are detected.""" + _write( + tmp_path, + "test_decorated.py", + f"import pytest\n\n\n@pytest.mark.{marker}\ndef test_x():\n pass\n", + ) + _write( + tmp_path, + "nested/test_module_level.py", + f"import pytest\n\npytestmark = pytest.mark.{marker}\n\n\ndef test_y():\n pass\n", + ) + # A file with no marker at all must not be selected. + _write(tmp_path, "test_plain.py", "def test_z():\n pass\n") + + result = _run(marker, str(tmp_path)) + + assert result.returncode == 0, result.stderr + listed = result.stdout.split() + assert any(p.endswith("test_decorated.py") for p in listed) + assert any(p.endswith("test_module_level.py") for p in listed) + assert not any(p.endswith("test_plain.py") for p in listed) + + +def test_marker_matched_as_whole_word(tmp_path): + """``macos_only`` must not match a longer identifier that contains it.""" + _write( + tmp_path, + "test_lookalike.py", + "import pytest\n\n\n@pytest.mark.macos_only_extra\ndef test_x():\n pass\n", + ) + + result = _run("macos_only", str(tmp_path)) + + # No genuine match: the helper must fail rather than emit nothing. + assert result.returncode == 1 + assert "macos_only" in result.stderr + + +def test_exits_nonzero_when_no_file_carries_the_marker(tmp_path): + """The load-bearing guard: an empty result is an error, never a silent pass.""" + _write(tmp_path, "test_plain.py", "def test_z():\n pass\n") + + result = _run("windows_only", str(tmp_path)) + + assert result.returncode == 1 + assert result.stdout.strip() == "" + assert "renamed or dropped" in result.stderr + + +def test_rejects_unknown_marker(tmp_path): + result = _run("bsd_only", str(tmp_path)) + + assert result.returncode == 2 + assert "unknown marker" in result.stderr + + +def test_rejects_missing_root(): + result = _run("macos_only", "/nonexistent/path/for/this/test") + + assert result.returncode == 2 + assert "no such directory" in result.stderr + + +def test_emits_repo_relative_posix_paths(): + """Output feeds a bash command line on the Windows runner, so separators + must be POSIX and paths repo-relative. + + Asserted against the real ``tests/`` tree, which is the only case CI + exercises — an out-of-repo root can't be made repo-relative and is + emitted absolute instead. + """ + result = _run("windows_only") + + assert result.returncode == 0, result.stderr + listed = result.stdout.split() + assert listed + for line in listed: + assert "\\" not in line + assert not Path(line).is_absolute() + + +def test_real_tree_selects_files_for_every_marker(): + """Against the actual ``tests/`` tree each marker resolves to real files. + + This is the invariant the CI lanes depend on — not a snapshot of which + files those are, only that each marker is in use and every listed path + exists. + """ + for marker in ("linux_only", "macos_only", "windows_only"): + result = _run(marker) + assert result.returncode == 0, f"{marker}: {result.stderr}" + listed = result.stdout.split() + assert listed, f"{marker} selected no files" + for rel in listed: + assert (REPO_ROOT / rel).is_file(), f"{marker} listed missing {rel}"