Merge pull request #328 from TomCruiseTorpedo/fix/collection-skip-fails-strict-run

# Conflicts:
#	CHANGELOG.md

Author: fabio-rovai

CHANGELOG.md

@@ -12,6 +12,18 @@ All notable changes to Open Ontologies are documented here.
 - Python: keep anonymous blank-node identities stable when diffing identical RDF text in `OntologyEngine.diff` and `kgcl_diff`; identical inputs remain parsed and validated.
 - Python: serialize RDF triple-term values in SELECT, CONSTRUCT and DESCRIBE query results instead of raising `AttributeError`; ordinary scalar values remain unchanged.
 - Python: accept reported blank-node graph names when selecting asserted graphs for Horn reasoning; preserve ordinary graph-name forms and inferred-graph refusal.
+- **A module that skipped at collection did not fail the strict Python leg.**
+  The entry that added `python/tests/conftest.py` says collection-time skips
+  "are caught too, through `pytest_collectreport`". They were seen and not
+  failed: the hook rewrote `report.outcome`, and that never reached the exit
+  status. MEASURED on pytest 9.0.3 and 9.1.1, under `OO_REQUIRE_FIXTURES=1`,
+  with one passing test and one module that skips at collection:
+  `1 passed, 1 skipped`, exit 0. Four files in the strict leg skip that way —
+  `test_shacl.py`, `test_shacl_vacuous.py`, `test_align.py` and
+  `test_vocab_check.py` — so the leg stayed green if their extras went missing.
+  The hook now records the skip, and `pytest_sessionfinish` fails the run.
+  `python/tests/test_conftest_collection_skip.py` holds it, with a control run
+  without the variable.
 
 - **`owl-dl` classifies Pizza exactly as HermiT does, and says when it is not
   complete.** On the canonical `pizza.owl` it returned no subsumptions and 143

docs/ci-gates.md

@@ -207,8 +207,10 @@ property is that CI cannot skip in silence, not that a laptop cannot skip at all
 The Python half is still by hand:
 
 ```bash
-# every Python file that can skip: 10 hits, of which conftest.py is the
-# mechanism and not a test, so 9, and the table has 9 rows
+# every Python file that can skip: 11 hits. conftest.py is the mechanism and
+# not a test, and test_conftest_collection_skip.py tests it: it writes a
+# skipping module into a temp directory and cannot skip itself. So 9, and the
+# table has 9 rows
 grep -rln 'pytest.skip\|importorskip\|skipif' python/tests/*.py
 # every leg CI makes strict
 grep -rn 'OO_REQUIRE_FIXTURES=1' .github/workflows/

python/tests/conftest.py

@@ -84,12 +84,45 @@ def pytest_runtest_makereport(item, call):
     return report
 
 
+_COLLECTION_SKIPS = []
+
+
 def pytest_collectreport(report):
     """A module-level `importorskip` skips at COLLECTION, not at run time.
 
     `pytest_runtest_makereport` never sees those, so three of this suite's files
     — the two pyshacl ones and the hnswlib one — would have stayed exempt from a
     rule written only there.
+
+    This hook can SEE such a skip and cannot FAIL it. Until this was changed it
+    set `report.outcome = "failed"` here, through `_fail`, and that never
+    reached the exit status. MEASURED on pytest 9.0.3 and 9.1.1, with one
+    passing test and one module that skips at collection, under
+    OO_REQUIRE_FIXTURES=1: `1 passed, 1 skipped`, exit 0. Marked `tryfirst`, the
+    summary said `1 error` and the exit was still 0. Run with `-rs` or `-ra`,
+    the terminal reporter crashed on the rewritten report. So the skip is
+    recorded here, and `pytest_sessionfinish` fails the run.
+    `test_conftest_collection_skip.py` holds this.
     """
     if report.skipped:
-        _fail(report, report.nodeid, _reason(report))
+        reason = _reason(report)
+        print(f"{MARKER}: {report.nodeid}: {reason}", flush=True)
+        _COLLECTION_SKIPS.append(f"{report.nodeid}: {reason}")
+
+
+@pytest.hookimpl(trylast=True)
+def pytest_sessionfinish(session, exitstatus):
+    """Where a skip at collection becomes a failure. See `pytest_collectreport`."""
+    if not (STRICT and _COLLECTION_SKIPS):
+        return
+    print(
+        f"\n{MARKER}: OO_REQUIRE_FIXTURES=1, so this is a failure, not a skip. "
+        f"{len(_COLLECTION_SKIPS)} module(s) skipped at collection and none of "
+        f"their tests ran:",
+        flush=True,
+    )
+    for line in _COLLECTION_SKIPS:
+        print(f"  {line}", flush=True)
+    # A status that already says something went wrong is left as it is.
+    if session.exitstatus == pytest.ExitCode.OK:
+        session.exitstatus = pytest.ExitCode.TESTS_FAILED

python/tests/test_conftest_collection_skip.py

@@ -0,0 +1,64 @@
+"""`OO_REQUIRE_FIXTURES=1` fails a run in which a module skipped at COLLECTION.
+
+A module-level skip happens before any test item exists, so only
+`pytest_collectreport` sees it. `conftest.py` used to rewrite the report there,
+and that did not reach the exit status: one passing test plus one such module
+gave `1 passed, 1 skipped` and exit 0 under the variable. Four files in the
+strict leg of the CI `python` job skip this way (`test_shacl.py`,
+`test_shacl_vacuous.py`, `test_align.py`, `test_vocab_check.py`), so that leg
+stayed green if their extras went missing.
+
+Each test runs pytest in a subprocess, on a throwaway directory that holds a
+COPY of this suite's own `conftest.py`. The hook under test is the shipped one,
+not a restatement of it.
+"""
+
+import os
+import shutil
+import subprocess
+import sys
+from pathlib import Path
+
+CONFTEST = Path(__file__).with_name("conftest.py")
+
+PASSES = "def test_passes():\n    assert True\n"
+
+SKIPS_AT_COLLECTION = (
+    "import pytest\n"
+    "pytest.importorskip('a_module_that_is_installed_nowhere')\n"
+    "\n"
+    "def test_never_runs():\n"
+    "    assert True\n"
+)
+
+
+def _run(tmp_path, strict):
+    shutil.copy(CONFTEST, tmp_path / "conftest.py")
+    (tmp_path / "test_passes.py").write_text(PASSES)
+    (tmp_path / "test_skips_at_collection.py").write_text(SKIPS_AT_COLLECTION)
+    env = dict(os.environ)
+    env.pop("OO_REQUIRE_FIXTURES", None)
+    env.pop("PYTEST_ADDOPTS", None)
+    if strict:
+        env["OO_REQUIRE_FIXTURES"] = "1"
+    return subprocess.run(
+        [sys.executable, "-m", "pytest", "-q", "-p", "no:cacheprovider",
+         "--rootdir", str(tmp_path), str(tmp_path)],
+        cwd=tmp_path,
+        env=env,
+        capture_output=True,
+        text=True,
+    )
+
+
+def test_a_skip_at_collection_fails_the_strict_run(tmp_path):
+    r = _run(tmp_path, strict=True)
+    assert r.returncode == 1, r.stdout + r.stderr
+    assert "test_skips_at_collection.py" in r.stdout, r.stdout
+
+
+def test_the_same_skip_passes_without_the_variable(tmp_path):
+    """The control. A conftest that failed EVERY run would pass the test above."""
+    r = _run(tmp_path, strict=False)
+    assert r.returncode == 0, r.stdout + r.stderr
+    assert "1 passed, 1 skipped" in r.stdout, r.stdout