From f6763d5663b0ce19d5bfe3109df06769374f0aff Mon Sep 17 00:00:00 2001 From: Dave Mihalcik Date: Mon, 9 Nov 2026 10:17:00 -0500 Subject: [PATCH] chore(ci): select suites by tier marker, not file name Every CI step named its test files, so each new suite edited the same lines of xtest.yml or check.yml (see #703, #706, #705) and conflicted with every other PR doing the same. Worse, a forgotten entry looks exactly like a pass: test_root_signature.py and test_audit_logs_integration.py are on main and run in no workflow at all. Each test module now declares one tier -- units, harness, core, legacy, multikas, benchmark, zip64 -- with pytestmark, *_units.py files are units by name, and each step selects with -m. conftest.py fails collection on any test without exactly one tier. check.yml's -m units step collects every module, so an untiered suite fails every PR rather than going unrun. Behaviour changes: - test_root_signature.py (core) and test_audit_logs_integration.py (multikas) now run in CI for the first time. - test_audit_logs.py runs offline in check.yml (units) instead of in the platform-backed helper step; it never needed the platform. - test_audit_logs_on_tampered_file xfails its audit assertion pending #709: it has failed since it was written, unseen because nothing ran it. The decrypt-rejection check stays blocking, and an audit pass fails the test so the xfail gets removed. Signed-off-by: Dave Mihalcik --- .github/workflows/check.yml | 10 +++----- .github/workflows/xtest.yml | 14 +++++------ xtest/AGENTS.md | 23 +++++++++++++++++ xtest/conftest.py | 37 +++++++++++++++++++++++++++- xtest/pyproject.toml | 8 ++++++ xtest/test_abac.py | 2 ++ xtest/test_audit_logs.py | 2 ++ xtest/test_audit_logs_integration.py | 11 +++++++-- xtest/test_bench_arms.py | 2 ++ xtest/test_bench_config.py | 2 ++ xtest/test_bench_measure.py | 11 ++++++--- xtest/test_bench_report.py | 2 ++ xtest/test_bench_runner.py | 2 ++ xtest/test_bench_stats.py | 2 ++ xtest/test_dpop.py | 2 ++ xtest/test_legacy.py | 2 ++ xtest/test_policytypes.py | 2 ++ xtest/test_pqc.py | 2 ++ xtest/test_root_signature.py | 2 ++ xtest/test_sdk_commands.py | 2 ++ xtest/test_self.py | 4 +++ xtest/test_tdf_assertions.py | 2 ++ xtest/test_tdfs.py | 2 ++ xtest/test_zip64.py | 2 +- xtest/test_zip64_units.py | 4 +-- xtest/test_zip_conformance.py | 6 ++++- 26 files changed, 136 insertions(+), 24 deletions(-) diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index 66046f1f2..43a352c4d 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -44,17 +44,15 @@ jobs: # No platform and no SDK builds required, so the part that has to be # *correct* is checked on every PR rather than only when the nightly runs. # + # -m units still collects every module, so conftest.py's tier check + # fails this step for any test the xtest.yml steps would not select. + # # --frozen --no-build: resolve nothing and build nothing, so a # dependency cannot slip in an unlocked version or a setup script on a # runner that already has everything installed from the step above. - name: Test xtest offline harnesses run: >- - uv run --frozen --no-build pytest --no-header -q - test_bench_stats.py test_bench_measure.py test_bench_runner.py - test_bench_arms.py test_bench_config.py test_bench_report.py - test_sdk_commands.py test_tdfs_units.py test_encryption_units.py - test_sizes_units.py test_zip64_units.py test_assertions_units.py - test_zip_conformance_units.py + uv run --frozen --no-build pytest --no-header -q -m units working-directory: xtest - name: Lint and test otdf-local run: | diff --git a/.github/workflows/xtest.yml b/.github/workflows/xtest.yml index 93158d326..e179f465c 100644 --- a/.github/workflows/xtest.yml +++ b/.github/workflows/xtest.yml @@ -713,7 +713,7 @@ jobs: - name: Validate xtest helper library (tests of the test harness and its utilities) if: ${{ !inputs }} run: |- - uv run pytest --html=test-results/helper-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" test_self.py test_audit_logs.py + uv run pytest --html=test-results/helper-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" -m harness working-directory: otdftests/xtest env: PLATFORM_TAG: ${{ matrix.platform-tag }} @@ -732,7 +732,7 @@ jobs: - name: Run legacy decryption tests run: |- skip_flag=$([[ "$SKIP_RELEASED_PAIRS" == "true" ]] && echo "--skip-released-pairs" || echo "") - uv run pytest -n auto --dist worksteal --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-decrypt "${ENCRYPT_SDK}" -ra -v --focus "$FOCUS_SDK" $skip_flag test_legacy.py + uv run pytest -n auto --dist worksteal --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-decrypt "${ENCRYPT_SDK}" -ra -v --focus "$FOCUS_SDK" $skip_flag -m legacy working-directory: otdftests/xtest env: PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" @@ -743,7 +743,7 @@ jobs: if: ${{ env.FOCUS_SDK == 'all' }} run: |- skip_flag=$([[ "$SKIP_RELEASED_PAIRS" == "true" ]] && echo "--skip-released-pairs" || echo "") - uv run pytest -n auto --dist loadscope --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" -ra -v $skip_flag test_tdfs.py test_tdf_assertions.py test_policytypes.py test_zip_conformance.py + uv run pytest -n auto --dist loadscope --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" -ra -v $skip_flag -m core working-directory: otdftests/xtest env: PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" @@ -755,7 +755,7 @@ jobs: if: ${{ env.FOCUS_SDK != 'all' }} run: |- skip_flag=$([[ "$SKIP_RELEASED_PAIRS" == "true" ]] && echo "--skip-released-pairs" || echo "") - uv run pytest -n auto --dist loadscope --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" -ra -v --focus "$FOCUS_SDK" $skip_flag test_tdfs.py test_tdf_assertions.py test_policytypes.py test_zip_conformance.py + uv run pytest -n auto --dist loadscope --html=test-results/sdk-${FOCUS_SDK}-${PLATFORM_TAG}.html --self-contained-html --sdks-encrypt "${ENCRYPT_SDK}" -ra -v --focus "$FOCUS_SDK" $skip_flag -m core working-directory: otdftests/xtest env: PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" @@ -888,7 +888,7 @@ jobs: --sdks-encrypt "${ENCRYPT_SDK}" \ --focus "$FOCUS_SDK" \ $skip_flag \ - test_abac.py test_pqc.py test_dpop.py + -m multikas working-directory: otdftests/xtest env: PLATFORM_DIR: "../../${{ steps.run-platform.outputs.platform-working-dir }}" @@ -1173,7 +1173,7 @@ jobs: --bench-out test-results/benchmarks \ --html "test-results/bench-${BENCH_SDK}.html" \ --self-contained-html \ - test_benchmarks.py + -m benchmark working-directory: otdftests/xtest env: BENCH_SDK: ${{ matrix.sdk }} @@ -1489,7 +1489,7 @@ jobs: --junitxml "test-results/zip64-${ZIP64_SDK}.xml" \ --html "test-results/zip64-${ZIP64_SDK}.html" \ --self-contained-html \ - test_zip64.py + -m zip64 working-directory: otdftests/xtest env: ZIP64_SDK: ${{ matrix.sdk }} diff --git a/xtest/AGENTS.md b/xtest/AGENTS.md index fc844f069..f319a12ba 100644 --- a/xtest/AGENTS.md +++ b/xtest/AGENTS.md @@ -19,6 +19,29 @@ fixture system. | `sdk/{go,java,js}/dist//` | SDK CLI builds. Installed by `otdf-sdk-mgr install` (see `../otdf-sdk-mgr/AGENTS.md`). | | `test.env` | Default endpoint and client-credential env vars. Source with `set -a && source test.env && set +a`. | +## Test Tiers + +CI selects tests by marker, not by file name, so adding a module never means +editing a workflow. Every test needs exactly one tier; collection fails +otherwise, because an untiered test would match no CI step and silently never +run. Declare it once per module: + +```python +pytestmark = pytest.mark.core +``` + +| Tier | Needs | Run by | +|------|-------|--------| +| `units` | Nothing: no platform, no SDK builds. Implied by a `*_units.py` file name. | `check.yml`, every PR | +| `harness` | Platform; tests the harness itself. | `xtest.yml` helper step | +| `core` | Platform + SDKs, platform's own KAS. | `xtest.yml` standard step | +| `legacy` | Golden TDFs and their keys. Decrypt-only. | `xtest.yml` legacy step | +| `multikas` | KAS alpha..delta and km1..km3. | `xtest.yml` ABAC step | +| `benchmark`, `zip64` | `--bench` / `--sizes medium`. | nightly jobs | + +Pick the tier by what the test needs, not by topic: `test_pqc.py` is +`multikas` because of its KAS fixtures, not because PQC is about ABAC. + ## Custom pytest Options (defined in `conftest.py`) | Option | Purpose | diff --git a/xtest/conftest.py b/xtest/conftest.py index a4d2fe88b..1c8d85946 100644 --- a/xtest/conftest.py +++ b/xtest/conftest.py @@ -425,7 +425,7 @@ def _parametrize_bench_cells(metafunc: pytest.Metafunc): """Fan the benchmark module out over its cells. Without --bench there is nothing to fan out over, and the items are - dropped wholesale in :func:`pytest_collection_modifyitems` rather than + dropped wholesale in :func:`_drop_unrequested` rather than parametrized here. Parametrizing over an empty list would *not* collect zero items: pytest's default ``empty_parameter_set_mark`` turns an empty set into one skipped item per test, so every ordinary run would carry @@ -541,9 +541,44 @@ def _item_exercises_zip64_window(item: pytest.Item, session_sizes: list[str]) -> return any(sizes.exercises_zip64_window(size) for size in session_sizes) +# CI steps select tests by tier (``-m core``), not by file name, so a new +# module is picked up by declaring ``pytestmark = pytest.mark.``. The +# tiers are registered in pyproject.toml. +TIERS = frozenset( + {"units", "harness", "core", "legacy", "multikas", "benchmark", "zip64"} +) + + +def _require_one_tier(items: list[pytest.Item]) -> None: + """Fail collection on any test without exactly one tier. + + An untiered test is matched by no CI step's ``-m`` and would never run, + which looks exactly like passing. ``*_units.py`` is ``units`` by name. + """ + bad: dict[str, int] = {} + for item in items: + if item.path.name.endswith("_units.py"): + item.add_marker(pytest.mark.units) + n = len({m.name for m in item.iter_markers()} & TIERS) + if n != 1: + bad.setdefault(item.nodeid.split("::")[0], n) + if bad: + found = ", ".join(f"{mod} ({n})" for mod, n in sorted(bad.items())) + raise pytest.UsageError( + f"need exactly one tier marker of {sorted(TIERS)}; found: {found}" + ) + + +# tryfirst: the tier check must see every item, before -m deselects any. +@pytest.hookimpl(tryfirst=True) def pytest_collection_modifyitems( config: pytest.Config, items: list[pytest.Item] ) -> None: + _require_one_tier(items) + _drop_unrequested(config, items) + + +def _drop_unrequested(config: pytest.Config, items: list[pytest.Item]) -> None: """Drop cells the session did not ask for. Two groups, deselected rather than skipped for the same reason: neither a diff --git a/xtest/pyproject.toml b/xtest/pyproject.toml index bac2eca83..3329a7e81 100644 --- a/xtest/pyproject.toml +++ b/xtest/pyproject.toml @@ -90,6 +90,14 @@ python_files = ["test_*.py"] python_functions = ["test_*"] addopts = "-ra -v" markers = [ + # Tiers: each test carries exactly one (conftest.py enforces it), and CI + # selects by tier with -m rather than by file name. benchmark and zip64 + # below are tiers too. + "units: offline harness tests; no platform, no SDK builds (implied by *_units.py)", + "harness: harness self-tests against a running platform", + "core: cross-SDK tests against the platform's own KAS", + "legacy: golden TDF decryption; needs the golden keys loaded", + "multikas: needs the alpha..delta and km1..km3 KAS instances", "benchmark: paired A/B performance cell; only collected under --bench", "no_audit_logs: opt this test out of the default audit-log assertions", "zip64: multi-GiB ZIP64 boundary cell; only collected when --sizes reaches 2**31", diff --git a/xtest/test_abac.py b/xtest/test_abac.py index 3fa40a794..66841f6be 100644 --- a/xtest/test_abac.py +++ b/xtest/test_abac.py @@ -21,6 +21,8 @@ from otdfctl import OpentdfCommandLineTool from test_policytypes import skip_rts_as_needed +pytestmark = pytest.mark.multikas + rewrap_403_pattern = ( "tdf: rewrap request 403|403 for \\[https?://[^\\]]+\\]; rewrap permission denied" ) diff --git a/xtest/test_audit_logs.py b/xtest/test_audit_logs.py index 2147ad3be..3b6132e1f 100644 --- a/xtest/test_audit_logs.py +++ b/xtest/test_audit_logs.py @@ -20,6 +20,8 @@ ParsedAuditEvent, # noqa: F401 - Used in TestParsedAuditEvent class ) +pytestmark = pytest.mark.units + class TestLogEntry: """Tests for the LogEntry class.""" diff --git a/xtest/test_audit_logs_integration.py b/xtest/test_audit_logs_integration.py index 06ddbccdb..1ced6f364 100644 --- a/xtest/test_audit_logs_integration.py +++ b/xtest/test_audit_logs_integration.py @@ -29,6 +29,8 @@ from audit_logs import AuditLogAsserter from otdfctl import OpentdfCommandLineTool +pytestmark = pytest.mark.multikas + logger = logging.getLogger(__name__) @@ -408,8 +410,13 @@ def tamper_policy_binding(manifest: tdfs.Manifest) -> tdfs.Manifest: except subprocess.CalledProcessError: pass # Expected - # Verify rewrap error was audited - audit_logs.assert_rewrap_error(min_count=1, since_mark=mark) + # Verify rewrap error was audited. Only this part is expected to fail + # (opentdf/tests#709): the decrypt rejection above must stay blocking. + try: + audit_logs.assert_rewrap_error(min_count=1, since_mark=mark) + except AssertionError: + pytest.xfail("no rewrap error audit event; see opentdf/tests#709") + pytest.fail("rewrap error now audited: drop the xfail (opentdf/tests#709)") @pytest.mark.slow def test_audit_under_sequential_load( diff --git a/xtest/test_bench_arms.py b/xtest/test_bench_arms.py index 6b9931b72..6d0a9e5f0 100644 --- a/xtest/test_bench_arms.py +++ b/xtest/test_bench_arms.py @@ -33,6 +33,8 @@ ) from perf.runner import BenchConfig +pytestmark = pytest.mark.units + def install(root: Path, sdk: str, *versions: str) -> None: """Lay down a stub build tree, as ``otdf-sdk-mgr install`` would.""" diff --git a/xtest/test_bench_config.py b/xtest/test_bench_config.py index 56c1ace18..85c995ac7 100644 --- a/xtest/test_bench_config.py +++ b/xtest/test_bench_config.py @@ -31,6 +31,8 @@ validate_sdk_name, ) +pytestmark = pytest.mark.units + class TestArmRequest: def test_str_with_spec(self): diff --git a/xtest/test_bench_measure.py b/xtest/test_bench_measure.py index d7990df80..802d51c3d 100644 --- a/xtest/test_bench_measure.py +++ b/xtest/test_bench_measure.py @@ -19,10 +19,13 @@ from perf import measure from perf.measure import MeasurementError, Sample -pytestmark = pytest.mark.skipif( - not hasattr(os, "wait4"), - reason="per-process rusage requires os.wait4", -) +pytestmark = [ + pytest.mark.units, + pytest.mark.skipif( + not hasattr(os, "wait4"), + reason="per-process rusage requires os.wait4", + ), +] def python_child(body: str) -> list[str]: diff --git a/xtest/test_bench_report.py b/xtest/test_bench_report.py index bba99f7ee..89c038684 100644 --- a/xtest/test_bench_report.py +++ b/xtest/test_bench_report.py @@ -11,6 +11,8 @@ from perf.report import _p_cell from perf.stats import Verdict +pytestmark = pytest.mark.units + def _comparison( ratio: float, diff --git a/xtest/test_bench_runner.py b/xtest/test_bench_runner.py index 13d4c3b7d..1ffce8c7c 100644 --- a/xtest/test_bench_runner.py +++ b/xtest/test_bench_runner.py @@ -30,6 +30,8 @@ run_cell, ) +pytestmark = pytest.mark.units + BASELINE_WALL_S = 1.0 BASELINE_RSS = 100_000_000 BASELINE_CPU = 0.8 diff --git a/xtest/test_bench_stats.py b/xtest/test_bench_stats.py index 1ff29a7c1..55e03ee58 100644 --- a/xtest/test_bench_stats.py +++ b/xtest/test_bench_stats.py @@ -19,6 +19,8 @@ from perf import stats from perf.stats import Verdict +pytestmark = pytest.mark.units + # Typical CI-runner dispersion for a CLI invocation: roughly +/-8% round to # round. Large enough to be realistic, small enough that 30 rounds can resolve # a 15% effect. diff --git a/xtest/test_dpop.py b/xtest/test_dpop.py index 18844774a..a5e1864a1 100644 --- a/xtest/test_dpop.py +++ b/xtest/test_dpop.py @@ -36,6 +36,8 @@ from audit_logs import AuditLogAsserter from fixtures.encryption import EncryptFactory +pytestmark = pytest.mark.multikas + def _token_endpoint() -> str: if endpoint := os.getenv("TOKENENDPOINT"): diff --git a/xtest/test_legacy.py b/xtest/test_legacy.py index 4a221ff17..621e6185d 100644 --- a/xtest/test_legacy.py +++ b/xtest/test_legacy.py @@ -5,6 +5,8 @@ import tdfs +pytestmark = pytest.mark.legacy + def get_golden_file(golden_file_name: str) -> Path: xtest_dir = Path(__file__).parent diff --git a/xtest/test_policytypes.py b/xtest/test_policytypes.py index 79b14117a..9204e978c 100644 --- a/xtest/test_policytypes.py +++ b/xtest/test_policytypes.py @@ -9,6 +9,8 @@ from abac import Attribute from fixtures.encryption import EncryptFactory +pytestmark = pytest.mark.core + def skip_rts_as_needed( encrypt_sdk: tdfs.SDK, diff --git a/xtest/test_pqc.py b/xtest/test_pqc.py index b214c5f4a..c2a66c68d 100644 --- a/xtest/test_pqc.py +++ b/xtest/test_pqc.py @@ -16,6 +16,8 @@ from fixtures.encryption import EncryptFactory from tdfs import KeyAccessObject +pytestmark = pytest.mark.multikas + # Rewrap session-key algorithms, as passed to SDK.decrypt(session_key_algorithm=...) # / the CLI wrappers, mapped to the features that must be present to try them. # Every case also needs the platform's "audit-session-key-type". RSA is every SDK's diff --git a/xtest/test_root_signature.py b/xtest/test_root_signature.py index 457b55ac7..da5388a92 100644 --- a/xtest/test_root_signature.py +++ b/xtest/test_root_signature.py @@ -42,6 +42,8 @@ from abac import Attribute from fixtures.encryption import EncryptFactory +pytestmark = pytest.mark.core + ManifestChange = Callable[[tdfs.Manifest], tdfs.Manifest] PayloadChange = Callable[[bytes], bytes] diff --git a/xtest/test_sdk_commands.py b/xtest/test_sdk_commands.py index c0fc2b12f..6e1cd0eca 100644 --- a/xtest/test_sdk_commands.py +++ b/xtest/test_sdk_commands.py @@ -17,6 +17,8 @@ import tdfs +pytestmark = pytest.mark.units + @pytest.fixture def sdk(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> tdfs.SDK: diff --git a/xtest/test_self.py b/xtest/test_self.py index 992a75402..5cb95fc1b 100644 --- a/xtest/test_self.py +++ b/xtest/test_self.py @@ -1,10 +1,14 @@ import random import string +import pytest + import abac from audit_logs import AuditLogAsserter from otdfctl import OpentdfCommandLineTool +pytestmark = pytest.mark.harness + otdfctl = OpentdfCommandLineTool() diff --git a/xtest/test_tdf_assertions.py b/xtest/test_tdf_assertions.py index 9219a02b5..eba995498 100644 --- a/xtest/test_tdf_assertions.py +++ b/xtest/test_tdf_assertions.py @@ -10,6 +10,8 @@ from fixtures.encryption import EncryptFactory from test_tdfs import assert_tamper_error, looks_like_422 +pytestmark = pytest.mark.core + #### MANIFEST VALIDITY TESTS diff --git a/xtest/test_tdfs.py b/xtest/test_tdfs.py index 47bed5f8b..731a5c00d 100644 --- a/xtest/test_tdfs.py +++ b/xtest/test_tdfs.py @@ -13,6 +13,8 @@ from audit_logs import AuditLogAsserter from fixtures.encryption import EncryptFactory +pytestmark = pytest.mark.core + dspx1153Fails = [] try: diff --git a/xtest/test_zip64.py b/xtest/test_zip64.py index 583852ead..b37a4f219 100644 --- a/xtest/test_zip64.py +++ b/xtest/test_zip64.py @@ -3,7 +3,7 @@ Every test here is marked ``zip64`` and is deselected unless the session asks for a payload size that can reach ``2**31`` -- see the size table in ``sizes.py`` for why 2.1 GiB and not something rounder, and -``pytest_collection_modifyitems`` in ``conftest.py`` for the deselection. +``_drop_unrequested`` in ``conftest.py`` for the deselection. Run it with:: diff --git a/xtest/test_zip64_units.py b/xtest/test_zip64_units.py index a935e294d..20303ccad 100644 --- a/xtest/test_zip64_units.py +++ b/xtest/test_zip64_units.py @@ -189,7 +189,7 @@ def test_item_without_size_uses_session_selection(self): class _FakeConfig: - """The three things ``pytest_collection_modifyitems`` asks of a config. + """The three things ``_drop_unrequested`` asks of a config. A real ``pytest.Config`` would need a real session; the hook only reads two options, caches the resolved sizes in the stash, and calls one hook. @@ -224,7 +224,7 @@ def _fake_item(name: str, *, marker: str | None = None, size: str | None = None) def _run_filter(config: _FakeConfig, items: list[pytest.Item]) -> list[str]: - conftest.pytest_collection_modifyitems(cast(pytest.Config, config), items) + conftest._drop_unrequested(cast(pytest.Config, config), items) return [i.name for i in items] diff --git a/xtest/test_zip_conformance.py b/xtest/test_zip_conformance.py index a1ea444fe..174c2d4e0 100644 --- a/xtest/test_zip_conformance.py +++ b/xtest/test_zip_conformance.py @@ -102,7 +102,11 @@ # ``no_audit_logs``: nothing here requests the ``audit_logs`` fixture -- this # module tests container encoding, not KAS rewrap events. -pytestmark = [pytest.mark.zip_conformance, pytest.mark.no_audit_logs] +pytestmark = [ + pytest.mark.core, + pytest.mark.zip_conformance, + pytest.mark.no_audit_logs, +] #: An extended-timestamp extra-field record (header id 0x5455), the shape #: real writers actually put ahead of a ZIP64 record in the wild.