diff --git a/.github/scripts/check_all_skip.py b/.github/scripts/check_all_skip.py index 578bff4e9..ef14d8faf 100644 --- a/.github/scripts/check_all_skip.py +++ b/.github/scripts/check_all_skip.py @@ -15,6 +15,20 @@ from pathlib import Path +class CollectionError(Exception): + """Raised when pytest exits with an unexpected return code on a test file.""" + + +def _write_github_output(reason: str, detail: str = "") -> None: + github_output = os.environ.get("GITHUB_OUTPUT", "") + if not github_output: + return + with open(github_output, "a") as f: + f.write(f"failure_reason={reason}\n") + if detail: + f.write(f"failure_detail={detail}\n") + + def resolve_base_ref(base_ref: str) -> str: """Resolve base_ref to a revision that exists in this checkout. @@ -78,7 +92,7 @@ def get_test_outcomes(test_files: list[str]) -> dict[str, dict]: f"(collection error or crash) — cannot judge skip status.\n" f"{proc.stdout[-2000:]}{proc.stderr[-2000:]}" ) - sys.exit(1) + raise CollectionError(filepath) # Parse stdout for summary lines like "4 passed, 2 skipped, 1 failed" # and individual test outcomes like "test_name SKIPPED" @@ -273,7 +287,11 @@ def main() -> int: return 0 # Get test outcomes - results = get_test_outcomes(test_files) + try: + results = get_test_outcomes(test_files) + except CollectionError as e: + _write_github_output("collection_error", str(e)) + return 1 # Get PR body for escape hatch pr_body = get_pr_body() @@ -372,6 +390,10 @@ def main() -> int: parts.append(f"{zero_collected_files} file(s) yielded no collected tests") if setup_error_files > 0: parts.append(f"{setup_error_files} file(s) had setup/teardown errors") + if unwaived_all_skip > 0: + _write_github_output("all_skip") + else: + _write_github_output("other_failure") print(f"\n::error:: {', '.join(parts)} — see above for details") return 1 diff --git a/.github/workflows/distrust-green-gate.yml b/.github/workflows/distrust-green-gate.yml index 5dd113867..c4c527e6a 100644 --- a/.github/workflows/distrust-green-gate.yml +++ b/.github/workflows/distrust-green-gate.yml @@ -43,15 +43,27 @@ jobs: with: github-token: ${{ secrets.GITHUB_TOKEN }} script: | - const body = [ - "**Distrust Green Gate**: this PR adds or modifies test file(s) where ALL", - "tests skip (e.g. `pytest.importorskip` on a module not yet available), so", - "CI reports GREEN while asserting nothing. See the check-all-skip job log", - "for the file and the guard that caused it.", - "", - "Either implement the guarded code, or waive deliberately with a", - "`Tests-Skipped-Intentionally: , ` trailer in the PR body.", - ].join("\n"); + const reason = `${{ steps.all-skip.outputs.failure_reason }}`; + let body; + if (reason === 'collection_error') { + body = [ + "**Distrust Green Gate**: this PR adds or modifies a test file that", + "fails to collect (crash or import error during collection). See the", + "check-all-skip job log for the file and error detail.", + "", + "Fix the collection error before requesting review.", + ].join("\n"); + } else { + body = [ + "**Distrust Green Gate**: this PR adds or modifies test file(s) where ALL", + "tests skip (e.g. `pytest.importorskip` on a module not yet available), so", + "CI reports GREEN while asserting nothing. See the check-all-skip job log", + "for the file and the guard that caused it.", + "", + "Either implement the guarded code, or waive deliberately with a", + "`Tests-Skipped-Intentionally: , ` trailer in the PR body.", + ].join("\n"); + } await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, diff --git a/changelog.d/tsk-ivbdjs-distrust-green-gate-misreport.md b/changelog.d/tsk-ivbdjs-distrust-green-gate-misreport.md new file mode 100644 index 000000000..45f00dbc3 --- /dev/null +++ b/changelog.d/tsk-ivbdjs-distrust-green-gate-misreport.md @@ -0,0 +1,3 @@ +### Fixed + +- Distrust Green Gate now reports collection errors separately from all-skip violations and no longer offers the `Tests-Skipped-Intentionally` waiver for files that crash on collection. \ No newline at end of file diff --git a/tests/scripts/test_check_all_skip.py b/tests/scripts/test_check_all_skip.py index e061e1689..f3887f67a 100644 --- a/tests/scripts/test_check_all_skip.py +++ b/tests/scripts/test_check_all_skip.py @@ -400,3 +400,105 @@ def test_rc5_no_tests_ran_still_reports_collection_message( captured = capsys.readouterr() assert rc == 1 assert "collection yielded 0 of 1 defined tests" in captured.out + + +class TestFailureReasonOutput: + """The script must write failure_reason to GITHUB_OUTPUT so the workflow can choose the right comment.""" + + def test_collection_error_writes_reason_and_no_waiver( + self, check_mod, tmp_path: Path, capsys: pytest.CaptureFixture + ) -> None: + github_output = tmp_path / "github_output.txt" + test_file = tmp_path / "test_crash.py" + test_file.write_text( + "def test_a():\n assert True\n" + ) + with patch.object(check_mod, "resolve_base_ref", return_value="origin/dev"): + with patch.object( + check_mod, + "get_test_outcomes", + side_effect=check_mod.CollectionError(str(test_file)), + ): + with patch.object(check_mod, "find_changed_test_files", return_value=[str(test_file)]): + with patch.object(check_mod, "get_pr_body", return_value=""): + env = {"BASE_REF": "origin/dev", "GITHUB_OUTPUT": str(github_output)} + with patch.object(check_mod.os, "environ", env): + rc = check_mod.main() + captured = capsys.readouterr() + assert rc == 1 + output_text = github_output.read_text() + assert "failure_reason=collection_error" in output_text + assert "Tests-Skipped-Intentionally" not in captured.out + + def test_all_skip_writes_reason_and_contains_waiver( + self, check_mod, tmp_path: Path, capsys: pytest.CaptureFixture + ) -> None: + github_output = tmp_path / "github_output.txt" + results = { + "tests/test_foo.py": { + "total": 3, + "skipped": 3, + "passed": 0, + "failed": 0, + "errors": 0, + "returncode": 0, + "tail": "", + "import_guards": [], + "defined_tests": 3, + } + } + with patch.object(check_mod, "resolve_base_ref", return_value="origin/dev"): + with patch.object(check_mod, "get_test_outcomes", return_value=results): + with patch.object(check_mod, "find_changed_test_files", return_value=["tests/test_foo.py"]): + with patch.object(check_mod, "get_pr_body", return_value=""): + env = {"BASE_REF": "origin/dev", "GITHUB_OUTPUT": str(github_output)} + with patch.object(check_mod.os, "environ", env): + rc = check_mod.main() + captured = capsys.readouterr() + assert rc == 1 + output_text = github_output.read_text() + assert "failure_reason=all_skip" in output_text + + def test_collection_error_and_all_skip_produce_different_reasons( + self, check_mod, tmp_path: Path + ) -> None: + collection_output = tmp_path / "collection_output.txt" + allskip_output = tmp_path / "allskip_output.txt" + + test_file = tmp_path / "test_crash.py" + test_file.write_text("def test_a():\n assert True\n") + + collection_results = {} # not used, side_effect raises + allskip_results = { + "tests/test_foo.py": { + "total": 3, "skipped": 3, "passed": 0, "failed": 0, + "errors": 0, "returncode": 0, "tail": "", + "import_guards": [], "defined_tests": 3, + } + } + + with patch.object(check_mod, "resolve_base_ref", return_value="origin/dev"): + with patch.object( + check_mod, + "get_test_outcomes", + side_effect=check_mod.CollectionError(str(test_file)), + ): + with patch.object(check_mod, "find_changed_test_files", return_value=[str(test_file)]): + with patch.object(check_mod, "get_pr_body", return_value=""): + env = {"BASE_REF": "origin/dev", "GITHUB_OUTPUT": str(collection_output)} + with patch.object(check_mod.os, "environ", env): + check_mod.main() + + with patch.object(check_mod, "resolve_base_ref", return_value="origin/dev"): + with patch.object(check_mod, "get_test_outcomes", return_value=allskip_results): + with patch.object(check_mod, "find_changed_test_files", return_value=["tests/test_foo.py"]): + with patch.object(check_mod, "get_pr_body", return_value=""): + env = {"BASE_REF": "origin/dev", "GITHUB_OUTPUT": str(allskip_output)} + with patch.object(check_mod.os, "environ", env): + check_mod.main() + + collection_reason = collection_output.read_text() + allskip_reason = allskip_output.read_text() + assert collection_reason != allskip_reason + assert "failure_reason=collection_error" in collection_reason + assert "failure_reason=all_skip" in allskip_reason