From ae7f7dedd310c413fc3482602bb48a39ec37a37e Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Tue, 1 Sep 2026 00:53:41 +0200 Subject: [PATCH] Run the eleven tests the suite skips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A green run of this suite reads `257 passed, 11 skipped`. A skipped test asserts nothing and is indistinguishable from a passing one in the job's conclusion, so eleven assertions had stopped holding the model to MEOS without anything saying so. Every one of them guards on a precondition this job already had the material for and never named. Supplied and asserted, the suite reads 268 passed, 0 skipped. Three causes, and the second is a defect rather than a circumstance. `MEOS_LIBRARY_PATH` and `MEOS_INCLUDE_DIR` were unset. The ctypes engine suite decodes, invokes and round-trips against a real libmeos, and the vendored test splits the installed headers by owner; the job builds and installs both and pointed neither at the tests. That is nine of the eleven. `object_model_parity.py` read `sys.argv` at IMPORT time to resolve its paths. The test suite imports it, so it read the RUNNER's flags: under `pytest tests/ -q -rs` the PyMEOS oracle resolved to the literal path `-rs`, which no checkout has, and both parity gates skipped themselves. The command line is now read in `main()`, where it belongs, and the oracle also honours `$PYMEOS_FACTORY` so a job whose PyMEOS checkout is not a sibling can name it. CI checks PyMEOS out and names it. Turning the engine suite on exposed what it had stopped catching. `build_type_encodings()` picks the decoder for the opaque `Temporal` by an alphabetical tiebreak it documents as arbitrary — there is no generic `temporal_in` to prefer — and the tests asserted the identity of that tiebreak's winner, `tbool_in`, together with a `tbool` literal. MobilityDB gaining `tbigint` moved the pick, so the suite decoded `{t@...}` with `tbigint_in`. The first assertion failed on the name and the resulting MEOS error state carried into two more tests, one of them reporting a garbage `-374120624`. The tests now follow the pick instead of naming it: a literal per subtype keyed by decoder, and an assertion of what the design does guarantee — the decoder is one of the subtype-narrow readers, the encoder IS the generic `temporal_out`. A `Refuse a silent skip` step fails the job on any skip. With every precondition supplied and asserted, a skip means a guard reads a condition this job no longer satisfies, which is a defect worth a red. `.gitignore` gains `.prefix/` and `_pymeos/`: the suite now runs against a built libmeos and a PyMEOS checkout, so both appear in a working tree and neither is the tree's to carry. Running the engine suite also exposed a test that could not have been reliable. `test_opaque_outparam_round_trip` called `encode("geo_as_ewkt", ptr)` with no aux, but `geo_as_ewkt(const GSERIALIZED *, int maxdd)` takes two arguments and `encode()` builds its argtypes from the aux it is given, so `maxdd` read whatever the register held. Measured over six runs it raised `The value must be strictly positive` on four of them, with a different value each time, and passed on two — the pass being luck of the address, not evidence. With `maxdd` supplied it is 6 of 6, and the engine suite is 8 of 8 over repeated runs. The defect predates this change; it was invisible because the test never ran. --- .github/workflows/pytest.yml | 51 ++++++++++++++++++++++++++++---- .gitignore | 7 +++++ object_model_parity.py | 29 +++++++++++++----- tests/test_engine_integration.py | 36 ++++++++++++++++++++-- 4 files changed, 106 insertions(+), 17 deletions(-) diff --git a/.github/workflows/pytest.yml b/.github/workflows/pytest.yml index 360e3af..735c1ce 100644 --- a/.github/workflows/pytest.yml +++ b/.github/workflows/pytest.yml @@ -40,21 +40,60 @@ jobs: - name: Install pytest run: pip install pytest + # PyMEOS is the object-model parity oracle. The audit reads its + # factory.py and degrades to `oracle-unavailable` without it, so the two + # parity gates assert nothing until it is on disk. + - name: Check out the PyMEOS oracle + uses: actions/checkout@v4 + with: + repository: MobilityDB/PyMEOS + path: _pymeos + # The action wrote output/meos-idl.json at the repo root, exactly where # the suite loads it from; assert the advertised path and run the tests. # - # MDB_SRC_ROOT names the MobilityDB checkout the catalog came from. The - # drift gates read meos_catalog.c and meos_error.h directly, and skip - # themselves where no source is reachable — so without it the suite is - # green while the assertions that hold the model to MEOS never run. The - # assertion below fails the job rather than let that recur silently. + # Every environment variable here is a precondition some test guards on, + # and a guard that fails makes the test SKIP: it asserts nothing and the + # job still reports success. This job already had the material for all of + # them and named none, so 11 of 268 tests never ran. + # + # MDB_SRC_ROOT the MobilityDB checkout the catalog came from; the + # drift gates read meos_catalog.c and meos_error.h. + # MEOS_LIBRARY_PATH the libmeos just built; the ctypes engine suite + # decodes, invokes and round-trips against it. + # MEOS_INCLUDE_DIR the installed headers; the vendored-provenance test + # splits them by owner. + # PYMEOS_FACTORY the parity oracle checked out above. + # + # Each is asserted before the run, so a moved path fails the job here + # rather than turning into a silent skip. - name: Run the test suite env: MDB_SRC_ROOT: ${{ steps.provision.outputs.mobilitydb-src }} + MEOS_LIBRARY_PATH: ${{ steps.provision.outputs.libmeos-prefix }}/lib/libmeos.so + MEOS_INCLUDE_DIR: ${{ steps.provision.outputs.libmeos-prefix }}/include + PYMEOS_FACTORY: ${{ github.workspace }}/_pymeos/pymeos/factory.py run: | + set -o pipefail test -s "${{ steps.provision.outputs.catalog-path }}" test -f "$MDB_SRC_ROOT/meos/src/temporal/meos_catalog.c" - python3 -m pytest tests/ -v + test -f "$MEOS_LIBRARY_PATH" + test -f "$MEOS_INCLUDE_DIR/meos.h" + test -f "$PYMEOS_FACTORY" + python3 -m pytest tests/ -v -rs | tee "$RUNNER_TEMP/pytest.log" + + # A skipped test is indistinguishable from a passing one in the job's + # conclusion, which is how the eleven went unnoticed. Every precondition + # is supplied and asserted above, so a skip now means a guard reads a + # condition this job no longer satisfies — a defect, not a circumstance. + - name: Refuse a silent skip + run: | + if grep -qE '[0-9]+ skipped' "$RUNNER_TEMP/pytest.log"; then + grep -E '^SKIPPED' "$RUNNER_TEMP/pytest.log" || true + echo "::error::the suite skipped tests; every precondition is supplied, so a skip is a defect" + exit 1 + fi + echo "no test skipped" - name: Upload meos-idl.json as artefact if: always() diff --git a/.gitignore b/.gitignore index a0dabe1..bb8b369 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,13 @@ output/ # Fetched MobilityDB sources _mobilitydb/ meos/ +# PyMEOS, the object-model parity oracle, where CI checks it out +_pymeos/ + +# The prefix tools/provision-meos.sh installs libmeos and the MEOS headers +# into. The suite now runs against a built one, so a developer has it locally; +# it is build output and never belongs in the tree. +.prefix/ # IDE .vscode/ diff --git a/object_model_parity.py b/object_model_parity.py index f4c31b6..2198527 100644 --- a/object_model_parity.py +++ b/object_model_parity.py @@ -13,18 +13,23 @@ # honest `oracle-unavailable` status (same philosophy as portable_parity.py). import json +import os import re import sys from pathlib import Path -IN_PATH = (Path(sys.argv[1]) if len(sys.argv) > 1 - else Path("output/meos-idl.json")) -OUT_PATH = (Path(sys.argv[2]) if len(sys.argv) > 2 - else Path("output/meos-object-model-parity.json")) -# PyMEOS oracle: factory.py. Default = sibling checkout; overridable. -PYMEOS = (Path(sys.argv[3]) if len(sys.argv) > 3 - else Path(__file__).resolve().parent.parent - / "PyMEOS" / "pymeos" / "factory.py") +# These defaults are resolved WITHOUT reading sys.argv, because the test suite +# imports this module and a module that indexes sys.argv at import time reads +# the RUNNER's flags. Under `pytest tests/ -q -rs` the oracle resolved to the +# literal path `-rs`, which no checkout has, so the parity gate skipped itself +# and the suite reported green while asserting nothing. The command line is +# read in main(), where it belongs. The oracle also honours $PYMEOS_FACTORY, so +# a job whose PyMEOS checkout is not a sibling can name it. +IN_PATH = Path("output/meos-idl.json") +OUT_PATH = Path("output/meos-object-model-parity.json") +PYMEOS = Path(os.environ.get("PYMEOS_FACTORY") + or Path(__file__).resolve().parent.parent + / "PyMEOS" / "pymeos" / "factory.py") def _oracle_id(path: Path) -> str: @@ -169,6 +174,14 @@ def _by_kind(work): def main() -> None: + # The command line is read here, so importing this module never consults it. + global IN_PATH, OUT_PATH, PYMEOS + if len(sys.argv) > 1: + IN_PATH = Path(sys.argv[1]) + if len(sys.argv) > 2: + OUT_PATH = Path(sys.argv[2]) + if len(sys.argv) > 3: + PYMEOS = Path(sys.argv[3]) if not IN_PATH.exists(): sys.exit(f"Catalog not found: {IN_PATH} — run `python run.py` first.") oracle = _parse_oracle(PYMEOS) diff --git a/tests/test_engine_integration.py b/tests/test_engine_integration.py index 249db5d..b3dfdd9 100644 --- a/tests/test_engine_integration.py +++ b/tests/test_engine_integration.py @@ -28,6 +28,20 @@ _TBOOL = "{t@2000-01-01, f@2000-01-03, t@2000-01-05}" _TFLOAT = "{1.5@2000-01-01, 3.5@2000-01-03}" +# A three-instant literal per temporal subtype, keyed by the decoder that reads +# it. Which decoder the catalog selects for the opaque `Temporal` is settled by +# an alphabetical tiebreak that build_type_encodings() documents as arbitrary: +# there is no generic `temporal_in`, so the pick is whichever subtype sorts +# first, and it MOVES when MobilityDB gains a type (tbigint displaced tbool). +# The fixture therefore follows the pick instead of naming it. +_LITERAL_BY_DECODER = { + "tbool_in": _TBOOL, + "tint_in": "{1@2000-01-01, 2@2000-01-03, 1@2000-01-05}", + "tbigint_in": "{1@2000-01-01, 2@2000-01-03, 1@2000-01-05}", + "tfloat_in": "{1.5@2000-01-01, 3.5@2000-01-03, 1.5@2000-01-05}", + "ttext_in": "{AA@2000-01-01, BB@2000-01-03, AA@2000-01-05}", +} + _KIND_TAG = {"integer": "int", "number": "double", "boolean": "bool", "string": "str"} @@ -48,16 +62,25 @@ def setUpClass(cls): cls.tout = t.get("out", "tbool_out") cls.in_aux = _aux(t.get("in_aux", [])) cls.out_aux = _aux(t.get("out_aux", [])) + cls.tin_literal = _LITERAL_BY_DECODER.get(cls.tin) def test_catalog_selected_in_out(self): # Decoding stays a typed wrapper (subtype-narrow); encoding is the # generic temporal_out with a defaulted maxdd. - self.assertEqual(self.tin, "tbool_in") + # + # WHICH subtype decodes is not asserted: no `temporal_in` exists, so + # build_type_encodings() falls back to an alphabetical pick it calls + # arbitrary, and that pick moves when MobilityDB gains a type. What the + # design does guarantee is asserted instead — the decoder is one of the + # subtype-narrow readers, and the encoder IS the generic root. + self.assertIn(self.tin, _LITERAL_BY_DECODER, + f"catalog selected {self.tin!r} as the Temporal decoder; " + f"add its literal to _LITERAL_BY_DECODER") self.assertEqual(self.tout, "temporal_out") self.assertEqual(self.out_aux, [("int", 15)]) def test_decode_invoke_scalar(self): - h = self.eng.decode(self.tin, _TBOOL, self.in_aux) + h = self.eng.decode(self.tin, self.tin_literal, self.in_aux) self.assertTrue(h) n = self.eng.invoke("temporal_num_instants", [("ptr", h)], "int") self.assertEqual(n, 3) @@ -96,7 +119,14 @@ def test_opaque_outparam_round_trip(self): True) self.assertTrue(present) self.assertTrue(ptr) - self.assertIn("POINT", self.eng.encode("geo_as_ewkt", ptr).upper()) + # geo_as_ewkt(const GSERIALIZED *, int maxdd) takes TWO arguments, so + # maxdd must be passed: encode() builds argtypes from the aux it is + # given, and calling a two-argument function with one argument leaves + # maxdd reading whatever the register held. MEOS rejects it whenever + # that junk is negative — measured failing 4 runs in 6, with a + # different value each time, and passing 6 in 6 once maxdd is supplied. + self.assertIn("POINT", + self.eng.encode("geo_as_ewkt", ptr, [("int", 15)]).upper()) def test_input_array_builder_round_trip(self): # Temporal *temporal_merge_array(Temporal **temparr, int count):