From cd0519f3d6ae96a4c367cceb7b07ebad353e1384 Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 09:47:30 +0000 Subject: [PATCH 1/6] task template: restore test files before tests, and run the changed package's tests folder An agent could rewrite a test's expected output to hide a regression (rich#3546: a rich/text.py change broke tests/test_card.py; the agent edited tests/_card_render.py). test.sh now discards agent edits to test files with the measurement setup, and pytest_runner.py runs the tests folders of the changed packages within a time limit: a test that fails twice with the agent's code and passes at base is a regression. --- src/datasmith/github/templates/final.md.j2 | 2 +- .../template/tests/pytest_runner.py | 147 +++++++++++++++--- .../harbor_adapter/template/tests/test.sh | 8 +- tests/docker/test_pytest_runner_wide.py | 97 ++++++++++++ tests/docker/test_test_sh_restore.py | 75 +++++++++ 5 files changed, 307 insertions(+), 22 deletions(-) create mode 100644 tests/docker/test_pytest_runner_wide.py create mode 100644 tests/docker/test_test_sh_restore.py diff --git a/src/datasmith/github/templates/final.md.j2 b/src/datasmith/github/templates/final.md.j2 index 90bbcb61..311deb00 100644 --- a/src/datasmith/github/templates/final.md.j2 +++ b/src/datasmith/github/templates/final.md.j2 @@ -46,7 +46,7 @@ The micromamba environment includes **Pytest** for testing and **Airspeed Veloci * **Target the hot paths** shown by profiling. * **Evidence-driven**: justify changes with benchmark/profile data. * **Correctness first**: never trade correctness for speed. -* **Leave the measurement setup alone**: the ASV benchmark files, `asv.conf*`, `conftest.py` and `.github/` are restored to their original state before your change is measured, so edits there are thrown away. Speed up the library code instead. +* **Leave the measurement setup alone**: the ASV benchmark files, `asv.conf*`, `conftest.py`, `.github/` and the test files are restored to their original state before your change is measured and tested, so edits there are thrown away. Speed up the library code instead. {% if repo_description %} **Repository Description** diff --git a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py index f6031bd9..5362a4b1 100644 --- a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py +++ b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py @@ -392,6 +392,80 @@ def run_base_and_diff(selected_tests, extra_args, repo_root, agent_results, base return out +WIDE_MAX_FILES = 40 + + +def _agent_run_limited(test_paths, extra_args, repo_root, timeout): + """pytest in a fresh process with a time limit: ({nodeid: outcome} of the tests that finished, rootdir, timed_out).""" + import tempfile as _tempfile + + self_path = os.path.abspath(__file__) + stream = _tempfile.mktemp(suffix="_fcwide.jsonl") + script = ( + "import sys, importlib.util; sys.path[0] = %r; " + "spec = importlib.util.spec_from_file_location('fc_pytest_runner', %r); " + "P = importlib.util.module_from_spec(spec); spec.loader.exec_module(P); " + "P.run_pytest_and_collect(%r, extra_args=%r, cwd=%r, stream=%r)" + ) % (os.path.dirname(self_path), self_path, list(test_paths), extra_args or "", repo_root, stream) + timed_out = False + try: + subprocess.run([sys.executable, "-c", script], cwd=repo_root, stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, timeout=max(1, int(timeout))) + except subprocess.TimeoutExpired: + timed_out = True + tests, rootdir = [], repo_root + try: + with open(stream) as fh: + for line in fh: + try: + rec = json.loads(line) + except ValueError: + continue + if "rootdir" in rec: + rootdir = rec["rootdir"] + else: + tests.append(rec) + os.remove(stream) + except OSError: + pass + return _final_outcomes({"tests": tests}), rootdir, timed_out + + +def run_wide_check(changed_files, ran_ids, extra_args, repo_root, budget, base_ref=None): + """The tests folders of the changed packages, beyond the tests mapped by name, within ``budget`` seconds. + + A test that fails twice with the agent's code and passes at base is a regression; only files with such + failures run at base. (rich#3546: a rich/text.py change broke tests/test_card.py, which no name maps to.)""" + dirs = package_tests_fallback(changed_files, repo_root) + out = {"ran": False, "dirs": dirs, "budget_s": int(budget), "regressed": []} + if not dirs or budget < 60: + out["reason"] = "no tests folder" if not dirs else "no time left" + return out + t0 = time.time() + first, rootdir, timed_out = _agent_run_limited(dirs, extra_args, repo_root, budget) + out.update(timed_out=timed_out, n_agent_tests=len(first)) + + def path_of(nodeid): + return os.path.relpath(os.path.join(rootdir, nodeid.split("::")[0]), repo_root).replace("\\", "/") + + failing = sorted(n for n, o in first.items() if o in ("failed", "error") and n not in ran_ids) + files = sorted({path_of(n) for n in failing})[:WIDE_MAX_FILES] + failing = [n for n in failing if path_of(n) in files] + if failing: + again, _, _ = _agent_run_limited(files, extra_args, repo_root, (budget - (time.time() - t0)) / 2) + failing = [n for n in failing if again.get(n) in ("failed", "error")] + files = sorted({path_of(n) for n in failing}) + out["n_agent_failing"] = len(failing) + if failing: + agent = {"tests": [{"nodeid": n, "when": "call", "outcome": "failed"} for n in failing]} + base = run_base_and_diff(files, extra_args, repo_root, agent, base_ref=base_ref) + out["base"] = {k: v for k, v in base.items() if k != "regressed"} + out["regressed"] = base["regressed"] + out["ran"] = bool(first) + out["duration_s"] = round(time.time() - t0, 1) + return out + + # --------------------------- Pytest Integration --------------------------- @@ -403,7 +477,8 @@ class _ResultsPlugin(object): 'pytest_warning_captured') at runtime to avoid Pluggy validation errors. """ - def __init__(self): + def __init__(self, stream=None): + self._stream = stream self.results = { "start_time": time.time(), "duration": None, @@ -432,6 +507,10 @@ def pytest_sessionstart(self, session): self.results["pytest_version"] = getattr(pytest, "__version__", None) except Exception: self.results["pytest_version"] = None + if self._stream: + # Node ids are relative to pytest's rootdir, which need not be the repo root. + with open(self._stream, "a") as fh: + fh.write(json.dumps({"rootdir": str(session.config.rootdir)}) + "\n") def pytest_runtest_logreport(self, report): if report.when not in ("setup", "call", "teardown"): @@ -466,6 +545,10 @@ def pytest_runtest_logreport(self, report): entry["longrepr"] = None self.results["tests"].append(entry) + if self._stream: + # One line per result, so a run stopped at its time limit still leaves the tests that finished. + with open(self._stream, "a") as fh: + fh.write(json.dumps({"nodeid": entry["nodeid"], "when": entry["when"], "outcome": outcome}) + "\n") if ( report.when == "call" @@ -555,7 +638,7 @@ def _warning_captured(warning_message, when, item, location): setattr(plugin, "pytest_warning_captured", _warning_captured) -def run_pytest_and_collect(test_paths, extra_args=None, cwd=None): +def run_pytest_and_collect(test_paths, extra_args=None, cwd=None, stream=None): import pytest if isinstance(extra_args, str): @@ -571,7 +654,7 @@ def run_pytest_and_collect(test_paths, extra_args=None, cwd=None): if os.path.exists(ap): unique_paths.append(ap) - plugin = _ResultsPlugin() + plugin = _ResultsPlugin(stream) _attach_warning_hook(plugin, pytest) # As `python -m pytest` from the repo: the repo's package must win over another copy of it in site-packages. @@ -927,7 +1010,27 @@ def main(args) -> dict: return out +def write_results(output): + # Must match parser.py's LOG_DIR lookup; /logs/test_results.json is a best-effort legacy copy. + logs_root = Path( + os.environ.get("T_BENCH_TASK_LOGS_PATH") + or os.environ.get("T_BENCH_CONTAINER_LOGS_PATH") + or "/logs/artifacts" + ) + _payload = json.dumps(output, sort_keys=True) + logs_root.mkdir(parents=True, exist_ok=True) + (logs_root / "test_results.json").write_text(_payload) + _legacy = Path("/logs/test_results.json") + if _legacy != logs_root / "test_results.json": + try: + _legacy.parent.mkdir(parents=True, exist_ok=True) + _legacy.write_text(_payload) + except Exception: # noqa: BLE001 -- best-effort; the parser path above is the critical one + pass + + if __name__ == "__main__": + _t_start = time.time() args = parse_args() # The retries below mutate extra_args; fallback tests use the originals. _orig_extra = args.extra_args @@ -971,20 +1074,26 @@ def main(args) -> dict: output["regression"] = {"ran": False, "error": "%s: %s" % (type(e).__name__, e)} output["fc_runner"] = "regression-v1+template" + # Written now too: if test.sh stops the runner at FC_PYTEST_TIMEOUT during the wider check, these results stand. + write_results(output) - # Must match parser.py's LOG_DIR lookup; /logs/test_results.json is a best-effort legacy copy. - logs_root = Path( - os.environ.get("T_BENCH_TASK_LOGS_PATH") - or os.environ.get("T_BENCH_CONTAINER_LOGS_PATH") - or "/logs/artifacts" - ) - _payload = json.dumps(output, sort_keys=True) - logs_root.mkdir(parents=True, exist_ok=True) - (logs_root / "test_results.json").write_text(_payload) - _legacy = Path("/logs/test_results.json") - if _legacy != logs_root / "test_results.json": - try: - _legacy.parent.mkdir(parents=True, exist_ok=True) - _legacy.write_text(_payload) - except Exception: # noqa: BLE001 -- best-effort; the parser path above is the critical one - pass + # Wider check, in what is left of FC_PYTEST_TIMEOUT minus a margin. + try: + left = float(os.environ.get("FC_PYTEST_TIMEOUT") or 1800) - (time.time() - _t_start) - 180 + wide = run_wide_check( + output.get("changed_files") or [], set(_final_outcomes(output.get("results"))), + args.extra_args if output.get("selected_tests") else _orig_extra, + output.get("repo_root") or detect_repo_root(), min(float(os.environ.get("FC_WIDE_TESTS_SEC") or 600), left), + base_ref=output.get("base_ref") or args.base_ref, + ) + output["regression_wide"] = wide + reg = output["regression"] + if wide["ran"]: + reg["regressed"] = sorted(set(reg.get("regressed") or []) | set(wide["regressed"])) + reg["n_regressed"] = len(reg["regressed"]) + # A failed base run of the mapped tests still counts as not run. + reg["ran"] = bool(reg.get("ran")) or not reg.get("base_error") + except Exception as e: # noqa: BLE001 + output["regression_wide"] = {"ran": False, "error": "%s: %s" % (type(e).__name__, e)} + + write_results(output) diff --git a/src/datasmith/harbor_adapter/template/tests/test.sh b/src/datasmith/harbor_adapter/template/tests/test.sh index 4f3d49fe..70091f59 100644 --- a/src/datasmith/harbor_adapter/template/tests/test.sh +++ b/src/datasmith/harbor_adapter/template/tests/test.sh @@ -82,8 +82,9 @@ PYEOF echo "[$(ts)] [test] patch: files=${patch_files} +${patch_added}/-${patch_removed}" # ── Restore the measurement setup ──────────────────────────────────────── -# Agent edits to benchmarks, ASV config, conftest.py or .github/ are discarded; patch.diff above keeps them. -# PATTERNS is a copy of PROTECTED_PATCH_PATTERNS in fcrl/grading/gates.py (formulacode-verified-rl). +# Agent edits to benchmarks, ASV config, conftest.py, .github/ and test files are discarded; patch.diff above keeps them. +# The first five PATTERNS are PROTECTED_PATCH_PATTERNS in fcrl/grading/gates.py (formulacode-verified-rl); the test +# files are restored so pytest runs the original tests against the agent's code. python - "${FC_BASE}" "${FC_INDEX:-}" "${LOG_DIR}/protected_restore.json" <<'PYEOF' || echo "[$(ts)] [test] WARNING: measurement setup restore failed" >&2 import json, os, re, subprocess, sys base, index, out = sys.argv[1:4] @@ -93,6 +94,9 @@ PATTERNS = ( r"(^|/)\.snapshots/", r"(^|/)conftest\.py$", r"(^|/)\.github/", + r"(^|/)tests?/", + r"(^|/)test_[^/]*\.py$", + r"(^|/)[^/]*_test\.py$", ) def changed(): env = dict(os.environ, GIT_INDEX_FILE=index) if index else None diff --git a/tests/docker/test_pytest_runner_wide.py b/tests/docker/test_pytest_runner_wide.py new file mode 100644 index 00000000..d487857e --- /dev/null +++ b/tests/docker/test_pytest_runner_wide.py @@ -0,0 +1,97 @@ +"""run_wide_check: tests of the changed package that no name maps to (rich#3546: text.py broke test_card.py).""" + +import importlib.util +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +_RUNNER = Path(__file__).parents[2] / "src" / "datasmith" / "harbor_adapter" / "template" / "tests" / "pytest_runner.py" + + +@pytest.fixture +def runner(): + spec = importlib.util.spec_from_file_location("_fc_template_pytest_runner_wide", _RUNNER) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def git(repo, *args): + return subprocess.run(["git", *args], cwd=repo, check=True, capture_output=True, text=True).stdout.strip() + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + (tmp_path / "pkg").mkdir() + (tmp_path / "pkg" / "__init__.py").write_text("") + (tmp_path / "pkg" / "text.py").write_text("def pad(s, n):\n return s + ' ' * (n - len(s))\n") + (tmp_path / "tests").mkdir() + (tmp_path / "tests" / "test_text.py").write_text("from pkg.text import pad\n\ndef test_pad():\n assert len(pad('a', 3)) == 3\n") + (tmp_path / "tests" / "test_card.py").write_text( + "from pkg.text import pad\n\ndef test_card():\n assert pad('ab', 4) == 'ab '\n\n" + "def test_broken_at_base():\n assert False\n" + ) + git(tmp_path, "init", "-q") + git(tmp_path, "add", "-A") + git(tmp_path, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "base") + return tmp_path + + +def test_a_test_no_name_maps_to_is_a_regression(runner, repo: Path) -> None: + (repo / "pkg" / "text.py").write_text("def pad(s, n):\n return s.rjust(n)\n") + out = runner.run_wide_check(["pkg/text.py"], set(), "", str(repo), 120) + assert out["ran"] is True and out["dirs"] == ["tests"] + # test_broken_at_base fails with the agent too, but also at base, so it is not counted. + assert out["regressed"] == ["tests/test_card.py::test_card"] + assert "rjust" in (repo / "pkg" / "text.py").read_text() + + +def test_tests_already_run_by_name_are_not_counted_twice(runner, repo: Path) -> None: + (repo / "pkg" / "text.py").write_text("def pad(s, n):\n return s.rjust(n)\n") + out = runner.run_wide_check(["pkg/text.py"], {"tests/test_card.py::test_card"}, "", str(repo), 120) + assert out["regressed"] == [] + + +def test_no_change_has_no_regression(runner, repo: Path) -> None: + out = runner.run_wide_check(["pkg/text.py"], set(), "", str(repo), 120) + assert out["ran"] is True and out["regressed"] == [] and out["n_agent_failing"] == 1 + + +def test_too_little_time_skips_the_check(runner, repo: Path) -> None: + out = runner.run_wide_check(["pkg/text.py"], set(), "", str(repo), 30) + assert out["ran"] is False and out["reason"] == "no time left" + + +def test_a_run_stopped_at_its_limit_keeps_the_finished_tests(runner, repo: Path) -> None: + (repo / "tests" / "test_slow.py").write_text( + "import time\n\ndef test_fast():\n pass\n\ndef test_slow():\n time.sleep(60)\n" + ) + outcomes, rootdir, timed_out = runner._agent_run_limited(["tests/test_slow.py"], "", str(repo), 5) + assert timed_out is True and rootdir == str(repo) + assert outcomes == {"tests/test_slow.py::test_fast": "passed"} + + +def test_node_ids_relative_to_a_tests_rootdir_map_to_repo_paths(runner, repo: Path) -> None: + # rich: pytest's rootdir is tests/ (its config is there), so node ids have no tests/ prefix. + (repo / "tests" / "pytest.ini").write_text("[pytest]\n") + (repo / "pkg" / "text.py").write_text("def pad(s, n):\n return s.rjust(n)\n") + out = runner.run_wide_check(["pkg/text.py"], set(), "", str(repo), 120) + assert out["regressed"] == ["test_card.py::test_card"] + + +def test_cli_adds_wide_regressions_to_the_regression_result(repo: Path, tmp_path_factory) -> None: + base = git(repo, "rev-parse", "HEAD") + (repo / "pkg" / "text.py").write_text("def pad(s, n):\n return s.rjust(n)\n") + logs = tmp_path_factory.mktemp("logs") + env = dict(os.environ, T_BENCH_TASK_LOGS_PATH=str(logs), FC_PYTEST_TIMEOUT="600") + subprocess.run([sys.executable, str(_RUNNER), "--base", base], cwd=repo, env=env, check=True, capture_output=True) + out = json.loads((logs / "test_results.json").read_text()) + assert out["selected_tests"] == ["tests/test_text.py"] + assert out["regression"]["ran"] is True + assert out["regression"]["regressed"] == ["tests/test_card.py::test_card"] + assert out["regression"]["n_regressed"] == 1 + assert out["regression_wide"]["ran"] is True diff --git a/tests/docker/test_test_sh_restore.py b/tests/docker/test_test_sh_restore.py new file mode 100644 index 00000000..2236edad --- /dev/null +++ b/tests/docker/test_test_sh_restore.py @@ -0,0 +1,75 @@ +"""The measurement setup restore in test.sh: protected paths and test files go back to the starting tree.""" + +import json +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +TEST_SH = Path(__file__).parents[2] / "src/datasmith/harbor_adapter/template/tests/test.sh" + + +def restore_script(sha_file: Path) -> str: + text = TEST_SH.read_text() + body = re.search(r'protected_restore\.json" <<\'PYEOF\'.*?\n(.*?)\nPYEOF', text, re.S).group(1) + return body.replace("/opt/fc_baseline_sha", str(sha_file)) + + +def git(repo, *args): + return subprocess.run(["git", *args], cwd=repo, check=True, capture_output=True, text=True).stdout.strip() + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + r = tmp_path / "repo" + for rel, body in { + "pkg/text.py": "x = 1\n", + "tests/_card_render.py": "expected = 'a'\n", + "tests/test_card.py": "def test_card():\n pass\n", + "pkg/tests/test_m.py": "def test_m():\n pass\n", + "benchmarks/bench.py": "def time_x():\n pass\n", + }.items(): + (r / rel).parent.mkdir(parents=True, exist_ok=True) + (r / rel).write_text(body) + git(r, "init", "-q") + git(r, "add", "-A") + git(r, "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", "base") + return r + + +def run(repo: Path, tmp_path: Path) -> dict: + sha = tmp_path / "fc_baseline_sha" + sha.write_text(git(repo, "rev-parse", "HEAD")) + out, index = tmp_path / "protected_restore.json", tmp_path / "index" + # test.sh passes a copy of the index (FC_INDEX), so files the agent added show up in the diff. + index.write_bytes((repo / ".git/index").read_bytes()) + subprocess.run([sys.executable, "-c", restore_script(sha), git(repo, "rev-parse", "HEAD"), str(index), str(out)], + cwd=repo, check=True, capture_output=True) + return json.loads(out.read_text()) + + +def test_edited_and_added_test_files_are_discarded_and_library_edits_kept(repo: Path, tmp_path: Path) -> None: + (repo / "pkg/text.py").write_text("x = 2\n") + (repo / "tests/_card_render.py").write_text("expected = 'b'\n") + (repo / "pkg/tests/test_m.py").write_text("def test_m():\n assert True\n") + (repo / "tests/test_new.py").write_text("def test_new():\n pass\n") + (repo / "pkg/util_test.py").write_text("def test_u():\n pass\n") + (repo / "benchmarks/bench.py").write_text("def time_x():\n return 1\n") + rec = run(repo, tmp_path) + assert rec["ran"] is True and rec["remaining"] == [] + assert rec["discarded"] == sorted([ + "benchmarks/bench.py", "pkg/tests/test_m.py", "pkg/util_test.py", "tests/_card_render.py", "tests/test_new.py", + ]) + assert (repo / "tests/_card_render.py").read_text() == "expected = 'a'\n" + assert not (repo / "tests/test_new.py").exists() and not (repo / "pkg/util_test.py").exists() + assert (repo / "pkg/text.py").read_text() == "x = 2\n" + + +def test_library_files_with_test_like_names_inside_the_package_are_kept(repo: Path, tmp_path: Path) -> None: + (repo / "pkg/testing.py").write_text("y = 1\n") + (repo / "pkg/_test_utils.py").write_text("z = 1\n") + rec = run(repo, tmp_path) + assert rec["discarded"] == [] + assert (repo / "pkg/testing.py").exists() and (repo / "pkg/_test_utils.py").exists() From 94a383832cff6529de78915bb9a7cd5066c51919 Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 10:05:54 +0000 Subject: [PATCH 2/6] pytest runner: a test file that stops importing is a regression; collection errors no longer stop the run dascore#415: the restored tests import a name the oracle patch removed, so pytest stopped at collection and the runner reported 0 tests run with no regressions. --- .../template/tests/pytest_runner.py | 23 +++++++++---- tests/docker/test_pytest_runner_regression.py | 32 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py index 5362a4b1..85a1d9af 100644 --- a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py +++ b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py @@ -283,8 +283,12 @@ def fallback_tests(changed_files, repo_root): def _final_outcomes(results): - """{nodeid: final outcome}; the 'call' outcome wins, else a setup/teardown error.""" + """{nodeid: final outcome}; the 'call' outcome wins, else a setup/teardown error. A test file that does not + collect is ``{file: "error"}``.""" out = {} + for e in (results or {}).get("errors") or []: + if e.get("nodeid"): + out[e["nodeid"]] = "error" for t in (results or {}).get("tests", []): nid = t.get("nodeid") if not nid: @@ -341,7 +345,8 @@ def run_base_and_diff(selected_tests, extra_args, repo_root, agent_results, base "spec = importlib.util.spec_from_file_location('fc_pytest_runner', %r); " "P = importlib.util.module_from_spec(spec); spec.loader.exec_module(P); " "r = P.run_pytest_and_collect(%r, extra_args=%r, cwd=%r); " - "open(%r, 'w').write(json.dumps(r.get('tests', [])))" + "errs = [dict(e, when='collect', outcome='error') for e in r.get('errors', []) if e.get('nodeid')]; " + "open(%r, 'w').write(json.dumps(r.get('tests', []) + errs))" ) % (os.path.dirname(self_path), self_path, list(selected_tests), base_args, repo_root, tf) # Own pycache: a base file with the agent file's size and mtime would load the agent bytecode. _old_pyc = os.environ.get("PYTHONPYCACHEPREFIX") @@ -374,9 +379,12 @@ def run_base_and_diff(selected_tests, extra_args, repo_root, agent_results, base base_error = (base_error or "") + "; patched rebuild after stash pop failed" if moved: _run(["git", "reset", "-q", "--soft", head], cwd=repo_root) + # A file that does not collect with the agent's code fails as a whole if any of its tests passes at base. + base_files = {n.split("::")[0] for n, o in base_oc.items() if o == "passed" and "::" in n} regressed = sorted( n for n, o in agent_oc.items() - if o in ("failed", "error") and base_oc.get(n) == "passed" + if o in ("failed", "error") + and (base_oc.get(n) == "passed" or ("::" not in n and n in base_files and base_oc.get(n) != "error")) ) out = { "ran": bool(stashed and selected_tests and base_oc), @@ -589,6 +597,9 @@ def pytest_collectreport(self, report): entry["longrepr"] = None self.results["errors"].append(entry) self.results["summary"]["error"] += 1 + if self._stream and entry["nodeid"]: + with open(self._stream, "a") as fh: + fh.write(json.dumps({"nodeid": entry["nodeid"], "when": "collect", "outcome": "error"}) + "\n") def _attach_warning_hook(plugin, pytest_module): @@ -663,6 +674,9 @@ def run_pytest_and_collect(test_paths, extra_args=None, cwd=None, stream=None): sys.path.insert(0, repo_dir) args = unique_paths + list(extra_args) + # By default one module that does not import stops the whole run, so no test result is left. + if "--continue-on-collection-errors" not in args: + args.append("--continue-on-collection-errors") # TMPDIR is a host-mounted folder whose pytest-of- can belong to another uid, which errors every test at setup. if not any(a == "--basetemp" or a.startswith("--basetemp=") for a in args): import tempfile as _tempfile @@ -1055,9 +1069,6 @@ def write_results(output): extra = args.extra_args if selected else _orig_extra if not selected: fb, strategy = fallback_tests(output.get("changed_files") or [], repo_root) - if strategy == "compiled_fallback": - # One test file that cannot import (e.g. needs a download) must not stop the others. - extra = (extra + " --continue-on-collection-errors").strip() if fb: selected = fb agent_res = run_pytest_and_collect(selected, extra_args=extra, cwd=repo_root) diff --git a/tests/docker/test_pytest_runner_regression.py b/tests/docker/test_pytest_runner_regression.py index 43824cc3..156c23fc 100644 --- a/tests/docker/test_pytest_runner_regression.py +++ b/tests/docker/test_pytest_runner_regression.py @@ -135,3 +135,35 @@ def test_repo_package_wins_over_a_copy_in_site_packages(tmp_path_factory) -> Non proc = subprocess.run([sys.executable, "-c", script], cwd=repo, env=env, capture_output=True, text=True) summary = json.loads(proc.stdout.strip().splitlines()[-1]) assert summary["passed"] == 1, proc.stdout + proc.stderr + + + +def commit_all(repo: Path, msg: str) -> None: + subprocess.run(["git", "add", "-A"], cwd=repo, check=True) + subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", msg], cwd=repo, check=True) + + +def test_a_test_file_that_no_longer_imports_is_a_regression(runner, repo: Path) -> None: + # dascore#415: the restored tests import a name the patch removed, and pytest stopped before any test ran. + m = (repo / "test_m.py").read_text() + subprocess.run(["git", "checkout", "--", "test_m.py"], cwd=repo, check=True) + (repo / "m.py").write_text("def f():\n return 1\n") + (repo / "test_imp.py").write_text("from m import f\n\ndef test_f():\n assert f() == 1\n") + commit_all(repo, "m") + (repo / "m.py").write_text("def g():\n return 1\n") + (repo / "test_m.py").write_text(m) + agent = runner.run_pytest_and_collect(["test_imp.py", "test_m.py"], cwd=str(repo)) + assert [t["nodeid"] for t in agent["tests"] if t["when"] == "call"] == ["test_m.py::test_a"], "the other file still runs" + out = runner.run_base_and_diff(["test_imp.py", "test_m.py"], "", str(repo), agent) + assert out["regressed"] == ["test_imp.py", "test_m.py::test_a"] + + +def test_a_test_file_that_does_not_import_at_base_either_is_not_a_regression(runner, repo: Path) -> None: + m = (repo / "test_m.py").read_text() + subprocess.run(["git", "checkout", "--", "test_m.py"], cwd=repo, check=True) + (repo / "test_bad.py").write_text("import a_module_that_does_not_exist\n") + commit_all(repo, "bad") + (repo / "test_m.py").write_text(m) + agent = dict(AGENT, errors=[{"nodeid": "test_bad.py", "longrepr": "ModuleNotFoundError"}]) + out = runner.run_base_and_diff(["test_bad.py", "test_m.py"], "", str(repo), agent) + assert out["regressed"] == ["test_m.py::test_a"] From a3fc7fc0ce5c0569e2f6b2e2d67778d649d4fa06 Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 10:12:16 +0000 Subject: [PATCH 3/6] pytest runner: keep the end of the wider run's output when it gives no results --- .../template/tests/pytest_runner.py | 26 ++++++++++++------- tests/docker/test_pytest_runner_wide.py | 2 +- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py index 85a1d9af..24391a94 100644 --- a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py +++ b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py @@ -404,7 +404,8 @@ def run_base_and_diff(selected_tests, extra_args, repo_root, agent_results, base def _agent_run_limited(test_paths, extra_args, repo_root, timeout): - """pytest in a fresh process with a time limit: ({nodeid: outcome} of the tests that finished, rootdir, timed_out).""" + """pytest in a fresh process with a time limit: ({nodeid: outcome} of the tests that finished, rootdir, timed_out, + the end of its output).""" import tempfile as _tempfile self_path = os.path.abspath(__file__) @@ -415,12 +416,15 @@ def _agent_run_limited(test_paths, extra_args, repo_root, timeout): "P = importlib.util.module_from_spec(spec); spec.loader.exec_module(P); " "P.run_pytest_and_collect(%r, extra_args=%r, cwd=%r, stream=%r)" ) % (os.path.dirname(self_path), self_path, list(test_paths), extra_args or "", repo_root, stream) - timed_out = False - try: - subprocess.run([sys.executable, "-c", script], cwd=repo_root, stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, timeout=max(1, int(timeout))) - except subprocess.TimeoutExpired: - timed_out = True + timed_out, err = False, "" + with _tempfile.TemporaryFile() as fh: + try: + subprocess.run([sys.executable, "-c", script], cwd=repo_root, stdout=fh, stderr=subprocess.STDOUT, + timeout=max(1, int(timeout))) + except subprocess.TimeoutExpired: + timed_out = True + fh.seek(0) + err = fh.read().decode("utf-8", "replace")[-1500:] tests, rootdir = [], repo_root try: with open(stream) as fh: @@ -436,7 +440,7 @@ def _agent_run_limited(test_paths, extra_args, repo_root, timeout): os.remove(stream) except OSError: pass - return _final_outcomes({"tests": tests}), rootdir, timed_out + return _final_outcomes({"tests": tests}), rootdir, timed_out, err def run_wide_check(changed_files, ran_ids, extra_args, repo_root, budget, base_ref=None): @@ -450,8 +454,10 @@ def run_wide_check(changed_files, ran_ids, extra_args, repo_root, budget, base_r out["reason"] = "no tests folder" if not dirs else "no time left" return out t0 = time.time() - first, rootdir, timed_out = _agent_run_limited(dirs, extra_args, repo_root, budget) + first, rootdir, timed_out, tail = _agent_run_limited(dirs, extra_args, repo_root, budget) out.update(timed_out=timed_out, n_agent_tests=len(first)) + if not first: + out["output_tail"] = tail def path_of(nodeid): return os.path.relpath(os.path.join(rootdir, nodeid.split("::")[0]), repo_root).replace("\\", "/") @@ -460,7 +466,7 @@ def path_of(nodeid): files = sorted({path_of(n) for n in failing})[:WIDE_MAX_FILES] failing = [n for n in failing if path_of(n) in files] if failing: - again, _, _ = _agent_run_limited(files, extra_args, repo_root, (budget - (time.time() - t0)) / 2) + again, _, _, _ = _agent_run_limited(files, extra_args, repo_root, (budget - (time.time() - t0)) / 2) failing = [n for n in failing if again.get(n) in ("failed", "error")] files = sorted({path_of(n) for n in failing}) out["n_agent_failing"] = len(failing) diff --git a/tests/docker/test_pytest_runner_wide.py b/tests/docker/test_pytest_runner_wide.py index d487857e..923e3354 100644 --- a/tests/docker/test_pytest_runner_wide.py +++ b/tests/docker/test_pytest_runner_wide.py @@ -70,7 +70,7 @@ def test_a_run_stopped_at_its_limit_keeps_the_finished_tests(runner, repo: Path) (repo / "tests" / "test_slow.py").write_text( "import time\n\ndef test_fast():\n pass\n\ndef test_slow():\n time.sleep(60)\n" ) - outcomes, rootdir, timed_out = runner._agent_run_limited(["tests/test_slow.py"], "", str(repo), 5) + outcomes, rootdir, timed_out, _ = runner._agent_run_limited(["tests/test_slow.py"], "", str(repo), 5) assert timed_out is True and rootdir == str(repo) assert outcomes == {"tests/test_slow.py::test_fast": "passed"} From 336fda513b25914c872f9e5bbc24e90a68e0a699 Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 10:14:26 +0000 Subject: [PATCH 4/6] pytest runner: the wider check also runs the tests folder above a changed compiled source --- .../harbor_adapter/template/tests/pytest_runner.py | 9 ++++++++- tests/docker/test_pytest_runner_wide.py | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py index 24391a94..e8091b55 100644 --- a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py +++ b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py @@ -448,7 +448,14 @@ def run_wide_check(changed_files, ran_ids, extra_args, repo_root, budget, base_r A test that fails twice with the agent's code and passes at base is a regression; only files with such failures run at base. (rich#3546: a rich/text.py change broke tests/test_card.py, which no name maps to.)""" - dirs = package_tests_fallback(changed_files, repo_root) + dirs = [] + for path in changed_files: + if is_test_file(path) or not (python_file(path) or _COMPILED_SOURCE.search(path)): + continue + found = _nearest_tests_dir(path, repo_root) + if found and found not in dirs: + dirs.append(found) + dirs = dirs[:4] out = {"ran": False, "dirs": dirs, "budget_s": int(budget), "regressed": []} if not dirs or budget < 60: out["reason"] = "no tests folder" if not dirs else "no time left" diff --git a/tests/docker/test_pytest_runner_wide.py b/tests/docker/test_pytest_runner_wide.py index 923e3354..a2137108 100644 --- a/tests/docker/test_pytest_runner_wide.py +++ b/tests/docker/test_pytest_runner_wide.py @@ -95,3 +95,10 @@ def test_cli_adds_wide_regressions_to_the_regression_result(repo: Path, tmp_path assert out["regression"]["regressed"] == ["tests/test_card.py::test_card"] assert out["regression"]["n_regressed"] == 1 assert out["regression_wide"]["ran"] is True + + +def test_a_compiled_source_change_runs_the_tests_folder_above_it(runner, repo: Path) -> None: + # bottleneck: the patch changes only C templates, which no .py rule covered. + (repo / "pkg" / "fast.c").write_text("int x;\n") + out = runner.run_wide_check(["pkg/fast.c"], set(), "", str(repo), 120) + assert out["dirs"] == ["tests"] and out["ran"] is True From e92ffab5b1915016fae311fcfdd107703c89f45e Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 10:48:58 +0000 Subject: [PATCH 5/6] pytest runner: pytest 9 has no config.rootdir; use rootpath --- src/datasmith/harbor_adapter/template/tests/pytest_runner.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py index e8091b55..c0029186 100644 --- a/src/datasmith/harbor_adapter/template/tests/pytest_runner.py +++ b/src/datasmith/harbor_adapter/template/tests/pytest_runner.py @@ -531,7 +531,9 @@ def pytest_sessionstart(self, session): if self._stream: # Node ids are relative to pytest's rootdir, which need not be the repo root. with open(self._stream, "a") as fh: - fh.write(json.dumps({"rootdir": str(session.config.rootdir)}) + "\n") + # rootdir is gone in pytest 9; rootpath is pytest >= 6.1. + root = getattr(session.config, "rootpath", None) or session.config.rootdir + fh.write(json.dumps({"rootdir": str(root)}) + "\n") def pytest_runtest_logreport(self, report): if report.when not in ("setup", "call", "teardown"): From d3b18e6ac815b2d370b08984172b47a7e21c135c Mon Sep 17 00:00:00 2001 From: Arjun Sharma Date: Mon, 19 Oct 2026 10:56:40 +0000 Subject: [PATCH 6/6] tests: ruff format --- tests/docker/test_pytest_runner_regression.py | 13 +++++++++---- tests/docker/test_pytest_runner_wide.py | 4 +++- tests/docker/test_test_sh_restore.py | 14 +++++++++++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/tests/docker/test_pytest_runner_regression.py b/tests/docker/test_pytest_runner_regression.py index 156c23fc..3e26d2b1 100644 --- a/tests/docker/test_pytest_runner_regression.py +++ b/tests/docker/test_pytest_runner_regression.py @@ -53,7 +53,9 @@ def test_new_test_file_from_the_agent_is_left_out_of_the_base_run(runner, repo: assert (repo / "test_new.py").exists() -def test_tmp_path_works_when_the_temp_root_belongs_to_another_user(runner, repo: Path, tmp_path_factory, monkeypatch) -> None: +def test_tmp_path_works_when_the_temp_root_belongs_to_another_user( + runner, repo: Path, tmp_path_factory, monkeypatch +) -> None: # memray/xarray: TMPDIR's pytest-of- was owned by another uid, so every test errored at setup. root = tmp_path_factory.mktemp("tmproot") monkeypatch.setenv("TMPDIR", str(root)) @@ -80,7 +82,9 @@ def git(*args): def test_no_base_results_is_not_ran(runner, repo: Path, monkeypatch) -> None: real = runner._run - monkeypatch.setattr(runner, "_run", lambda cmd, cwd=None: (1, "", "boom") if cmd[0] == sys.executable else real(cmd, cwd)) + monkeypatch.setattr( + runner, "_run", lambda cmd, cwd=None: (1, "", "boom") if cmd[0] == sys.executable else real(cmd, cwd) + ) out = runner.run_base_and_diff(["test_m.py"], "", str(repo), AGENT) assert out["ran"] is False assert out["stashed"] is True @@ -137,7 +141,6 @@ def test_repo_package_wins_over_a_copy_in_site_packages(tmp_path_factory) -> Non assert summary["passed"] == 1, proc.stdout + proc.stderr - def commit_all(repo: Path, msg: str) -> None: subprocess.run(["git", "add", "-A"], cwd=repo, check=True) subprocess.run(["git", "-c", "user.name=t", "-c", "user.email=t@t", "commit", "-qm", msg], cwd=repo, check=True) @@ -153,7 +156,9 @@ def test_a_test_file_that_no_longer_imports_is_a_regression(runner, repo: Path) (repo / "m.py").write_text("def g():\n return 1\n") (repo / "test_m.py").write_text(m) agent = runner.run_pytest_and_collect(["test_imp.py", "test_m.py"], cwd=str(repo)) - assert [t["nodeid"] for t in agent["tests"] if t["when"] == "call"] == ["test_m.py::test_a"], "the other file still runs" + assert [t["nodeid"] for t in agent["tests"] if t["when"] == "call"] == ["test_m.py::test_a"], ( + "the other file still runs" + ) out = runner.run_base_and_diff(["test_imp.py", "test_m.py"], "", str(repo), agent) assert out["regressed"] == ["test_imp.py", "test_m.py::test_a"] diff --git a/tests/docker/test_pytest_runner_wide.py b/tests/docker/test_pytest_runner_wide.py index a2137108..6a507cf5 100644 --- a/tests/docker/test_pytest_runner_wide.py +++ b/tests/docker/test_pytest_runner_wide.py @@ -30,7 +30,9 @@ def repo(tmp_path: Path) -> Path: (tmp_path / "pkg" / "__init__.py").write_text("") (tmp_path / "pkg" / "text.py").write_text("def pad(s, n):\n return s + ' ' * (n - len(s))\n") (tmp_path / "tests").mkdir() - (tmp_path / "tests" / "test_text.py").write_text("from pkg.text import pad\n\ndef test_pad():\n assert len(pad('a', 3)) == 3\n") + (tmp_path / "tests" / "test_text.py").write_text( + "from pkg.text import pad\n\ndef test_pad():\n assert len(pad('a', 3)) == 3\n" + ) (tmp_path / "tests" / "test_card.py").write_text( "from pkg.text import pad\n\ndef test_card():\n assert pad('ab', 4) == 'ab '\n\n" "def test_broken_at_base():\n assert False\n" diff --git a/tests/docker/test_test_sh_restore.py b/tests/docker/test_test_sh_restore.py index 2236edad..e8b4fc11 100644 --- a/tests/docker/test_test_sh_restore.py +++ b/tests/docker/test_test_sh_restore.py @@ -45,8 +45,12 @@ def run(repo: Path, tmp_path: Path) -> dict: out, index = tmp_path / "protected_restore.json", tmp_path / "index" # test.sh passes a copy of the index (FC_INDEX), so files the agent added show up in the diff. index.write_bytes((repo / ".git/index").read_bytes()) - subprocess.run([sys.executable, "-c", restore_script(sha), git(repo, "rev-parse", "HEAD"), str(index), str(out)], - cwd=repo, check=True, capture_output=True) + subprocess.run( + [sys.executable, "-c", restore_script(sha), git(repo, "rev-parse", "HEAD"), str(index), str(out)], + cwd=repo, + check=True, + capture_output=True, + ) return json.loads(out.read_text()) @@ -60,7 +64,11 @@ def test_edited_and_added_test_files_are_discarded_and_library_edits_kept(repo: rec = run(repo, tmp_path) assert rec["ran"] is True and rec["remaining"] == [] assert rec["discarded"] == sorted([ - "benchmarks/bench.py", "pkg/tests/test_m.py", "pkg/util_test.py", "tests/_card_render.py", "tests/test_new.py", + "benchmarks/bench.py", + "pkg/tests/test_m.py", + "pkg/util_test.py", + "tests/_card_render.py", + "tests/test_new.py", ]) assert (repo / "tests/_card_render.py").read_text() == "expected = 'a'\n" assert not (repo / "tests/test_new.py").exists() and not (repo / "pkg/util_test.py").exists()