From 0360f8af64e2bd23ce15202e92f211adbbd7edd6 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Mon, 27 Jul 2026 16:56:03 -0700 Subject: [PATCH 1/5] improve hierarchy building for combined axis labels via ids --- .../_internal/pytest_plugin/steps.py | 130 +++++++++++++--- .../_tests/pytest_plugin/test_hierarchy.py | 144 +++++++++++++++++- 2 files changed, 246 insertions(+), 28 deletions(-) diff --git a/python/lib/sift_client/_internal/pytest_plugin/steps.py b/python/lib/sift_client/_internal/pytest_plugin/steps.py index 0dfcb816c..f5b8822f5 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/steps.py +++ b/python/lib/sift_client/_internal/pytest_plugin/steps.py @@ -16,7 +16,7 @@ import inspect import logging import warnings -from typing import TYPE_CHECKING, Any, List, Optional, Tuple +from typing import TYPE_CHECKING, Any, List, Optional, Sequence, Tuple import pytest @@ -110,7 +110,8 @@ def _signal_introspection_degraded(detail: str) -> None: HierarchyParent = Tuple[HierarchyKey, str, Optional[str]] # (identity, name, docstring) ParametrizeParent = Tuple[ParametrizeKey, str] # (registry key, frame name) # Scope-promoted params (anything broader than function scope) stashed per item. -# Each entry is ``(scope, param_name, label)``; in callspec application order. +# Each entry is ``(scope, param_name, label)``; in callspec application order, one +# per axis. ``param_name`` is the axis's first argname (see ``_param_axes``). ScopedParams = Tuple[Tuple[str, str, str], ...] # A gated-in leaf's parents: its rendered hierarchy identities and parametrize keys. LeafParents = Tuple[List[HierarchyKey], List[ParametrizeKey]] @@ -277,14 +278,14 @@ def _id_from_spec(ids: Any, index: int, value: Any) -> str | None: def _explicit_param_id(item: pytest.Item, name: str, value: Any) -> str | None: - """The author-supplied pytest ID for param ``name`` on ``item``, else None. + """The author-supplied pytest ID for single-argname axis ``name``, else None. Honours an explicit ``ids=`` (list or callable factory) declared on the fixture (``@pytest.fixture(params=..., ids=...)``) or on the ``@pytest.mark.parametrize`` axis, matching the friendly labels pytest puts - in the node ID. Combined axes (``"a,b"``) are skipped: their single shared - ID can't be attributed to one of the two frames the report renders, so those - fall back to ``name=value``. + in the node ID. Combined axes (``"a,b"``) are handled by + ``_combined_axis_label``: they render as one frame, so their shared ID needs + no attribution and is adopted directly. """ callspec = getattr(item, "callspec", None) if callspec is None: @@ -309,15 +310,101 @@ def _explicit_param_id(item: pytest.Item, name: str, value: Any) -> str | None: def _param_label(item: pytest.Item, name: str, value: Any) -> str: - """Display label for one param axis: its explicit pytest ID, else ``name=value``.""" + """Display label for one single-argname axis: its explicit pytest ID, else ``name=value``.""" return _explicit_param_id(item, name, value) or f"{name}={value!r}" +def _mark_for_param(item: pytest.Item, name: str) -> pytest.Mark | None: + """The ``parametrize`` mark covering param ``name``, or None if it is fixture-based.""" + for mark in item.iter_markers("parametrize"): + if name in _mark_argnames(mark): + return mark + return None + + +def _combined_axis_label(item: pytest.Item, mark: pytest.Mark, names: Sequence[str]) -> str: + """Display label for one multi-argname axis (``@parametrize("a,b", ...)``). + + A combined axis is a single axis in pytest's model: one tuple per case, one + ID per tuple, one bracket segment in the node ID. It renders as one step, so + the author's shared ``ids=`` entry labels it directly. + + A callable ``ids=`` factory is applied per value and the results joined with + ``-``, mirroring how pytest builds the segment for a combined axis. If any + value yields no ID the whole factory result is discarded rather than + part-labelling the frame. + + With no usable ``ids=``, fall back to the tuple's ``name=value`` pairs joined + with ``, ``. Pytest's auto-generated IDs are deliberately not adopted here, + for the same reason ``_id_from_spec`` rejects them: they are noisier than the + structured pairs for non-trivial values. + """ + callspec = item.callspec # type: ignore[attr-defined] + ids = mark.kwargs.get("ids") + index = callspec.indices.get(names[0]) + if ids is not None and index is not None: + if callable(ids): + parts: list[str] = [] + for name in names: + try: + resolved = ids(callspec.params[name]) + except Exception: + resolved = None + if resolved is None: + break + parts.append(str(resolved)) + else: + return "-".join(parts) + elif index < len(ids): + return str(ids[index]) + return ", ".join(f"{name}={callspec.params[name]!r}" for name in names) + + +def _param_axes(item: pytest.Item) -> tuple[tuple[tuple[str, ...], str, str], ...]: + """The item's parametrize axes as ``(argnames, scope, label)``, application order. + + One entry per AXIS, not per argname: a combined ``@parametrize("a,b", ...)`` + contributes a single entry covering both names, because that is what pytest + treats as one axis (one ID, one node-ID segment) and what the report should + render as one step. Ordering follows ``callspec.params``, with each axis + placed at its first argname's position. + + ``scope`` and the fixture-based ``ids=`` lookup are resolved from the axis's + first argname; pytest requires every argname in a combined axis to share a + scope, so the first is representative. + """ + callspec = getattr(item, "callspec", None) + if callspec is None or not callspec.params: + return () + axes: list[tuple[tuple[str, ...], str, str]] = [] + seen: set[str] = set() + for name in callspec.params: + if name in seen: + continue + mark = _mark_for_param(item, name) + # Restrict to argnames this callspec actually carries: a mark can name a + # param that a later ``pytest_generate_tests`` hook removed. + names = ( + tuple(n for n in _mark_argnames(mark) if n in callspec.params) + if mark is not None + else (name,) + ) + if len(names) > 1 and mark is not None: + label = _combined_axis_label(item, mark, names) + else: + names = (name,) + label = _param_label(item, name, callspec.params[name]) + seen.update(names) + axes.append((names, _param_scope(item, names[0]), label)) + return tuple(axes) + + def build_scoped_params(item: pytest.Item) -> ScopedParams: """Scope-promoted params for ``item`` (anything broader than function scope). Each entry is ``(scope, name, label)`` in ``callspec.params`` application - order. ``resolved_parents`` buckets these by scope and places them at their + order, one per axis (see ``_param_axes``); ``name`` is the axis's first + argname. ``resolved_parents`` buckets these by scope and places them at their scope's hierarchy level. Function-scoped axes are excluded; they stay inner, handled by ``build_parametrize_path``. """ @@ -325,13 +412,11 @@ def build_scoped_params(item: pytest.Item) -> ScopedParams: if callspec is None or not callspec.params: return () try: - out: list[tuple[str, str, str]] = [] - for name, value in callspec.params.items(): - scope = _param_scope(item, name) - if scope == "function": - continue - out.append((scope, name, _param_label(item, name, value))) - return tuple(out) + return tuple( + (scope, names[0], label) + for names, scope, label in _param_axes(item) + if scope != "function" + ) except Exception as exc: # Degrade to "nothing promoted": every axis stays function-scoped via # build_parametrize_path, so the leaf still renders, just flat. @@ -344,12 +429,15 @@ def build_parametrize_path(item: pytest.Item) -> ParametrizePath: Pytest stores ``callspec.params`` with the BOTTOM decorator's axis first; the Sift step tree treats the TOP decorator as outermost, so we reverse. + One frame per AXIS (see ``_param_axes``), so a combined + ``@parametrize("a,b", ...)`` is one frame rather than one per argname. Only function-scoped axes appear here; higher-scoped params are promoted into the hierarchy by ``resolved_parents``. Each axis is labelled by its explicit pytest ID when the author supplied one, otherwise by ``name=value`` - (see ``_param_label``). The first frame is always ``originalname`` so the - leaf step (``path[-1]`` in ``report.step_impl``) is the bare function name - when a test has no function-scoped params of its own. + (see ``_param_label`` and ``_combined_axis_label``). The first frame is + always ``originalname`` so the leaf step (``path[-1]`` in + ``report.step_impl``) is the bare function name when a test has no + function-scoped params of its own. """ callspec = getattr(item, "callspec", None) if callspec is None or not callspec.params: @@ -357,10 +445,10 @@ def build_parametrize_path(item: pytest.Item) -> ParametrizePath: originalname = getattr(item, "originalname", item.name) frames: list[str] = [originalname] try: - for name, value in reversed(callspec.params.items()): - if _param_scope(item, name) != "function": + for _names, scope, label in reversed(_param_axes(item)): + if scope != "function": continue - frames.append(_param_label(item, name, value)) + frames.append(label) except Exception as exc: # Mirror the build_scoped_params degradation: with scope resolution # broken nothing is promoted, so render every axis here under the bare diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py index 55454de56..4ea69b41d 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py @@ -1644,11 +1644,11 @@ def test_auto(v): assert "v=2" in by_name -def test_combined_axis_ids_fall_back_to_name_value( +def test_combined_axis_renders_one_frame_with_its_id( pytester: pytest.Pytester, out_dir: Path ) -> None: - """A combined ``"a,b"`` axis can't attribute its shared ID, so each frame uses - ``name=value`` rather than mislabelling both with the same combined ID. + """A combined ``"a,b"`` axis is ONE axis in pytest, so it is one step labelled + with the shared ID, not one step per argname. """ pytester.makepyfile( test_comb=dedent( @@ -1664,10 +1664,140 @@ def test_comb(a, b): result = pytester.runpytest_inprocess("-v") result.assert_outcomes(passed=1) by_name = _by_name(capture.load_steps(capture.run_jsonl(out_dir))) - # The shared "combined" ID is not adopted for either per-arg frame. - assert "combined" not in by_name - assert "a=1" in by_name - assert "b=2" in by_name + assert "combined" in by_name + # No per-argname frames beneath it. + assert "a=1" not in by_name + assert "b=2" not in by_name + + +def test_combined_axis_without_ids_joins_name_value_pairs( + pytester: pytest.Pytester, out_dir: Path +) -> None: + """With no ``ids=``, a combined axis still renders as ONE frame, labelled with + the tuple's ``name=value`` pairs rather than pytest's noisier auto-ID. + """ + pytester.makepyfile( + test_comb=dedent( + """ + import pytest + + @pytest.mark.parametrize("a,b", [(1, 2)]) + def test_comb(a, b): + pass + """ + ) + ) + result = pytester.runpytest_inprocess("-v") + result.assert_outcomes(passed=1) + by_name = _by_name(capture.load_steps(capture.run_jsonl(out_dir))) + assert "a=1, b=2" in by_name + assert "a=1" not in by_name + + +def test_callable_id_factory_on_combined_axis(pytester: pytest.Pytester, out_dir: Path) -> None: + """A callable ``ids=`` factory runs per value and the results join with ``-``, + matching how pytest builds a combined axis's node-ID segment. + """ + pytester.makepyfile( + test_comb=dedent( + """ + import pytest + + def label(value): + return f"v{value}" + + @pytest.mark.parametrize("a,b", [(1, 2)], ids=label) + def test_comb(a, b): + pass + """ + ) + ) + result = pytester.runpytest_inprocess("-v") + result.assert_outcomes(passed=1) + by_name = _by_name(capture.load_steps(capture.run_jsonl(out_dir))) + assert "v1-v2" in by_name + + +def test_combined_axis_stacked_with_single_axis(pytester: pytest.Pytester, out_dir: Path) -> None: + """A combined axis stacked under a single-name axis yields two frames, top + decorator outermost, each labelled by its own ID. + """ + pytester.makepyfile( + test_comb=dedent( + """ + import pytest + + @pytest.mark.parametrize("v", ["hi", "lo"]) + @pytest.mark.parametrize("a,b", [(1, 2)], ids=["combined"]) + def test_comb(v, a, b): + pass + """ + ) + ) + result = pytester.runpytest_inprocess("-v") + result.assert_outcomes(passed=2) + steps = capture.load_steps(capture.run_jsonl(out_dir)) + by_name = _by_name(steps) + assert "v='hi'" in by_name + assert "combined" in by_name + assert "a=1" not in by_name + # Top decorator outermost: every combined-axis frame nests under a ``v=`` frame. + assert len(by_name["combined"]) == 2 + for frame in by_name["combined"]: + ancestors = _ancestor_names(steps, frame) + assert "v='hi'" in ancestors or "v='lo'" in ancestors + + +def test_combined_axis_under_scoped_fixture_param(pytester: pytest.Pytester, out_dir: Path) -> None: + """A module-scoped fixture axis above a three-argname combined axis whose + tuples carry a function object. One frame per axis, each labelled by its own + ID, and no frame carrying an object repr. + """ + pytester.makepyfile( + test_axes=dedent( + """ + import pytest + + def handler_a(): pass + def handler_b(): pass + + CASES = [ + (handler_a, "signal.alpha", "ALPHA"), + (handler_b, "signal.beta", "BETA"), + ] + CASE_IDS = ["ALPHA", "BETA"] + + @pytest.fixture(scope="module", params=["mode one"], ids=["mode one"]) + def mode(request): + return request.param + + @pytest.mark.parametrize("handler,signal_name,case_name", CASES, ids=CASE_IDS) + def test_signal(handler, signal_name, case_name, mode, step): + with step.substep("record baseline"): + pass + """ + ) + ) + result = pytester.runpytest_inprocess("-v") + result.assert_outcomes(passed=2) + steps = capture.load_steps(capture.run_jsonl(out_dir)) + by_name = _by_name(steps) + assert "mode one" in by_name + assert "ALPHA" in by_name + assert "BETA" in by_name + # The other two argnames of the combined axis open no frames of their own. + assert not [n for n in by_name if n.startswith(("handler=", "signal_name="))] + # No label carries a function repr (heap address, unstable across runs). + assert not [n for n in by_name if " Date: Mon, 27 Jul 2026 17:52:23 -0700 Subject: [PATCH 2/5] fix teardown failure on last item --- .../_internal/pytest_plugin/report.py | 5 + .../pytest_plugin/_step_status_capture.py | 37 ++++++ .../_tests/pytest_plugin/test_pass_fail.py | 44 +++++++ python/lib/sift_client/pytest_plugin.py | 21 ++-- .../util/test_results/context_manager.py | 111 ++++++++++++++---- 5 files changed, 189 insertions(+), 29 deletions(-) diff --git a/python/lib/sift_client/_internal/pytest_plugin/report.py b/python/lib/sift_client/_internal/pytest_plugin/report.py index af28cfb84..4b8eb21c5 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/report.py +++ b/python/lib/sift_client/_internal/pytest_plugin/report.py @@ -474,6 +474,11 @@ def report_context_impl( replay_log_file=not (disabled or offline), metadata=report_metadata, audit_log=audit_log, + # pytest tears this session-scoped fixture down during the LAST item's + # teardown phase but reports that phase's outcome afterwards, so a + # teardown failure on the final test is unknown here. The plugin's + # ``pytest_sessionfinish`` calls ``finalize`` once it can't be. + defer_finalize=True, ) as context: report = context.report meta_kv = ",".join(f"{k}={v}" for k, v in (report.metadata or {}).items()) or "-" diff --git a/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py b/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py index 5f7314369..fe6bcc90e 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py +++ b/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py @@ -170,6 +170,11 @@ def final_error_message(name: str) -> str | None: return step.error_messages[-1] if step and step.error_messages else None +# Ordered ``(line index, status)`` writes, for asserting write order across +# entry kinds. See ``status_writes``. +StatusWrites = list[tuple[int, TestStatus]] + + def log_events(log_path: Path) -> list[tuple[str, str, TestStatus]]: """Ordered ``(request_type, step_name, status)`` tuples as they appear in the log. @@ -196,6 +201,38 @@ def log_events(log_path: Path) -> list[tuple[str, str, TestStatus]]: return events +def status_writes(log_path: Path, step_name: str) -> tuple[StatusWrites, StatusWrites]: + """``(report writes, FAILED writes on ``step_name``)`` as ``(line index, status)``. + + Line indices come from the same pass, so they are comparable across the two + lists and a test can assert the ORDER of a report write against a step write. + ``log_events`` can't express that, since it drops report entries. + """ + if not log_path.exists(): + return [], [] + report_writes: StatusWrites = [] + step_writes: StatusWrites = [] + step_ids: set[str] = set() + for idx, (request_type, response_id, json_str) in enumerate(_log_data_lines(log_path)): + # Skip the JSON decode for measurement entries, which dominate most logs. + if not request_type.endswith(("TestReport", "TestStep")): + continue + payload = json.loads(json_str) + if request_type.endswith("TestReport"): + raw = payload.get("testReport", {}).get("status") + if raw: + report_writes.append((idx, _status(raw))) + continue + test_step = payload.get("testStep", {}) + if request_type == "CreateTestStep" and test_step.get("name") == step_name and response_id: + step_ids.add(response_id) + elif request_type == "UpdateTestStep" and test_step.get("testStepId") in step_ids: + status = _status(test_step.get("status")) + if status == TestStatus.FAILED: + step_writes.append((idx, status)) + return report_writes, step_writes + + def load_steps(log_path: Path) -> list[dict]: """Load the offline log as a list of step records keyed by hierarchy fields. diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_pass_fail.py b/python/lib/sift_client/_tests/pytest_plugin/test_pass_fail.py index c1c0f79f1..86f8b01c2 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_pass_fail.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_pass_fail.py @@ -506,6 +506,50 @@ def test_x(bad_teardown): assert outer.statuses[-1] == TestStatus.FAILED +def test_teardown_failure_on_last_item_reaches_the_report(inner): + """A teardown failure on the FINAL collected item must reach the report. + + pytest tears down the session-scoped ``report_context`` fixture during the + last item's teardown phase and only *then* reports that phase's outcome. The + report status and the import-worker drain therefore have to be deferred to + ``pytest_sessionfinish``, or the failure is computed too late to matter and + its log entry is written after the worker has already stopped reading. + """ + _run( + inner, + """ + import pytest + + @pytest.fixture + def bad_teardown(): + yield + raise RuntimeError("teardown boom") + + def test_x(bad_teardown): + assert True + """, + ) + log_path = capture._active_log + assert log_path is not None + + outer = capture.test_step("test_x") + assert outer is not None + assert outer.statuses[-1] == TestStatus.FAILED + + report_writes, step_writes = capture.status_writes(log_path, "test_x") + + # The report's LAST status write must be FAILED. Before the fix the report + # was finalized as PASSED inside the fixture teardown, ahead of the failure. + assert report_writes, "report never wrote a status" + assert report_writes[-1][1] == TestStatus.FAILED + + # Ordering is the real guarantee: the report's terminal status has to be + # written AFTER the step's late FAILED update. If it precedes it, the drain + # preceded it too and the failure would never have been uploaded. + assert step_writes, "step never wrote a FAILED status" + assert report_writes[-1][0] > step_writes[-1][0] + + # --------------------------------------------------------------------------- # Collection-phase failures # --------------------------------------------------------------------------- diff --git a/python/lib/sift_client/pytest_plugin.py b/python/lib/sift_client/pytest_plugin.py index 36a501c87..3e2eda6d6 100644 --- a/python/lib/sift_client/pytest_plugin.py +++ b/python/lib/sift_client/pytest_plugin.py @@ -613,24 +613,29 @@ def pytest_keyboard_interrupt(excinfo: pytest.ExceptionInfo[BaseException]) -> N @pytest.hookimpl(hookwrapper=True) def pytest_sessionfinish(session: pytest.Session, exitstatus: int): - """Close any report-tree parents still open at session end (innermost first). + """Close any report-tree parents still open at session end, then finalize the report. - Normally a no-op: ``report_context_impl`` finalizes the parents inside the - ``ReportContext`` block so their updates reach the log before the import - worker drains, and most parents already closed early as their subtrees - finished. This is the idempotent backstop for anything still open. + Parent closing is normally a no-op: ``report_context_impl`` finalizes the + parents inside the ``ReportContext`` block, and most already closed early as + their subtrees finished. This is the idempotent backstop for anything still + open. - Runs as a hookwrapper so the drain happens *after* pytest's own + Runs as a hookwrapper so both steps happen *after* pytest's own ``pytest_sessionfinish`` (``SetupState.teardown_exact``), which finalizes the still-open leaf step and the session-scoped ``report_context`` fixture. On a session abort (``pytest.exit``) the leaf's fixture teardown is deferred to that point; finalizing parents before it would close them while their descendant is still unresolved, so the abort/failure would not roll up. The - ``yield`` lets that teardown run first, leaving this call the no-op backstop - it is meant to be. + ``yield`` lets that teardown run first. + + ``ReportContext.finalize`` runs here rather than in the context's ``__exit__`` + (see ``defer_finalize``) because this is the earliest point guaranteed to + follow the last item's teardown report. Both calls are idempotent. """ yield finalize_parents() + if REPORT_CONTEXT is not None: + REPORT_CONTEXT.finalize() def _verbosity(config: pytest.Config) -> int: diff --git a/python/lib/sift_client/util/test_results/context_manager.py b/python/lib/sift_client/util/test_results/context_manager.py index db21a317a..2fc849d6f 100644 --- a/python/lib/sift_client/util/test_results/context_manager.py +++ b/python/lib/sift_client/util/test_results/context_manager.py @@ -11,7 +11,7 @@ from contextlib import AbstractContextManager, contextmanager from datetime import datetime, timezone from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any import numpy as np @@ -149,6 +149,18 @@ def _is_session_exit(exc_value: BaseException | None) -> bool: return cls.__name__ == "Exit" and cls.__module__ == "_pytest.outcomes" +def _ancestor_paths(step_path: str) -> list[str]: + """Ancestor ``step_path``s of ``step_path``, innermost first. + + ``"1.2.3"`` gives ``["1.2", "1"]``. A root path gives ``[]``. + """ + paths = [] + while "." in step_path: + step_path = step_path.rsplit(".", 1)[0] + paths.append(step_path) + return paths + + class ReportContext(AbstractContextManager): """Context manager for a new TestReport. See usage example in __init__.py.""" @@ -192,6 +204,18 @@ class ReportContext(AbstractContextManager): # When set, the path of the DEBUG audit log. The replay worker is spawned # with ``--audit-log `` so its activity is traced too. audit_log: Path | None = None + # When True, ``__exit__`` finalizes nothing and the owner must call + # ``finalize`` itself. Set by the pytest plugin, because pytest reports a + # test's teardown-phase outcome AFTER tearing down the session-scoped fixture + # that owns this context. For the last item in a session a teardown failure + # is therefore only known once ``__exit__`` has already returned, so + # finalizing there would miss it and drain the import worker before its log + # entry is written. + defer_finalize: bool = False + _finalized: bool = False + # Whether the ``with`` block exited with an exception; folded into the + # report's status by ``finalize``. + _exit_failed: bool = False _import_proc: subprocess.Popen | None = None # Seconds to wait for the import worker subprocess to finish uploading # the JSONL backlog at session end before killing it. Tests substitute @@ -213,6 +237,7 @@ def __init__( replay_log_file: bool = True, metadata: dict[str, str | float | bool] | None = None, audit_log: str | Path | None = None, + defer_finalize: bool = False, ): """Initialize a new report context. @@ -240,6 +265,9 @@ def __init__( audit_log: When set, the path of a DEBUG audit log. The replay worker is spawned with ``--audit-log `` so its activity is traced to ``.replay.log``. + defer_finalize: When True, ``__exit__`` finalizes nothing and the + caller must call ``finalize`` once no further status changes can + arrive. See the attribute of the same name. """ self.client = client self.replay_log_file = replay_log_file @@ -254,6 +282,9 @@ def __init__( self.created_steps = [] self.created_measurements = [] self.replay_incomplete = False + self.defer_finalize = defer_finalize + self._finalized = False + self._exit_failed = False if log_file is True: session_dir = _make_session_dir() @@ -343,12 +374,31 @@ def __enter__(self): return self def __exit__(self, exc_type, exc_value, traceback): - update = { + self._exit_failed = exc_type is not None + if self.defer_finalize: + return True + return self.finalize() + + def finalize(self) -> bool: + """Roll the final status onto the report, then drain the import worker. + + Split out of ``__exit__`` so an owner expecting late failures can defer + both halves (see ``defer_finalize``). They must happen in this order, + since the status update is itself a log entry and draining first would + strand it. + + Idempotent, so a deferring owner can call it without tracking whether + ``__exit__`` already ran. + """ + if self._finalized: + return True + self._finalized = True + update: dict[str, Any] = { "end_time": datetime.now(timezone.utc), } if self.session_aborted: update["status"] = TestStatus.ABORTED - elif self.any_failures or exc_type: + elif self.any_failures or self._exit_failed: update["status"] = TestStatus.FAILED else: update["status"] = TestStatus.PASSED @@ -580,29 +630,48 @@ def record_measurement(self, measurement: TestMeasurement) -> None: self.created_measurements.append(measurement) def mark_step_failed_after_close(self, step: TestStep): - """Mark a step's parent as failed after the step has already been popped from the stack. + """Roll a failure up the ancestor chain after the step was already closed. Used by the pytest plugin when a teardown-phase report fires after the fixture's ``__exit__`` has already resolved and exited the step. + + Every ancestor is walked, not just the immediate parent. A teardown + failure can surface after ``finalize_parents`` has closed the whole chain + (for the last item in a session it always does), and those ancestors have + already been written out as PASSED. Marking ``open_step_results`` alone + would change nothing server-side, so each closed ancestor is re-updated + to FAILED. Ancestors still open are left to ``propagate_step_result``, + which picks up the ``open_step_results`` flag when they close. + + The report's own status is not touched here; it is derived from + ``any_failures`` in ``finalize``. """ self.any_failures = True - path_parts = step.step_path.split(".") - if len(path_parts) > 1: - parent_path = ".".join(path_parts[:-1]) - self.open_step_results[parent_path] = False - # Diagnostic: a teardown-phase failure fires after the step's own - # __exit__ has resolved, so it never runs through - # propagate_step_result. Log the parent roll-up here so it is traced - # like every other failure that reaches a parent. - log_event( - logger, - logging.DEBUG, - "step.propagate", - step_path=step.step_path, - status=step.status.name, - signal="teardown_fail", - parent=parent_path, - ) + ancestors = _ancestor_paths(step.step_path) + if not ancestors: + return + by_path = {s.step_path: s for s in self.created_steps} + for ancestor_path in ancestors: + self.open_step_results[ancestor_path] = False + ancestor = by_path.get(ancestor_path) + # Only a cleanly-closed ancestor needs the correction. One still open + # resolves through the normal path, and one already FAILED/ABORTED + # must not be downgraded. + if ancestor is not None and ancestor.status == TestStatus.PASSED: + ancestor.update({"status": TestStatus.FAILED}) + # Diagnostic: a teardown-phase failure fires after the step's own + # __exit__ has resolved, so it never runs through propagate_step_result. + # One line for the whole chain, so a deep hierarchy does not bury the + # single signal it represents. + log_event( + logger, + logging.DEBUG, + "step.propagate", + step_path=step.step_path, + status=step.status.name, + signal="teardown_fail", + parents=",".join(ancestors), + ) def propagate_step_result(self, step: TestStep, status: TestStatus) -> bool: """Propagate this step's final status to the parent step. From 57b1c2b9ff3dbffc11d4a62561c17535c8bd2052 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Mon, 27 Jul 2026 18:08:24 -0700 Subject: [PATCH 3/5] lint --- .../sift_client/_tests/pytest_plugin/_step_status_capture.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py b/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py index fe6bcc90e..53fa2672f 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py +++ b/python/lib/sift_client/_tests/pytest_plugin/_step_status_capture.py @@ -11,7 +11,7 @@ import json from dataclasses import dataclass, field -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, List, Tuple from sift_client._internal.low_level_wrappers._test_results_log import parse_log_data_lines from sift_client.sift_types.test_report import TestStatus @@ -172,7 +172,7 @@ def final_error_message(name: str) -> str | None: # Ordered ``(line index, status)`` writes, for asserting write order across # entry kinds. See ``status_writes``. -StatusWrites = list[tuple[int, TestStatus]] +StatusWrites = List[Tuple[int, TestStatus]] def log_events(log_path: Path) -> list[tuple[str, str, TestStatus]]: From 469beddd4bb3ed5235047fcc30fff0391155e661 Mon Sep 17 00:00:00 2001 From: Alex Luck Date: Mon, 27 Jul 2026 18:34:47 -0700 Subject: [PATCH 4/5] fix Python 3.8 type alias and order-dependent tests StatusWrites is a module-level alias, so it is evaluated at runtime and builtin generics are not subscriptable before 3.9. Spell it with typing.List/Tuple. Four new tests reused the inner module and function name test_comb. steps.parametrize_parents is a module-global keyed by nodeid and its reset fixture is opt-in, so identical keys collided across tests once the order was shuffled. Give each a unique name, and assert the ancestor chain over every leaf rather than a fixed index. --- .../_tests/pytest_plugin/test_hierarchy.py | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py index 4ea69b41d..6a63ddc5c 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py @@ -1651,12 +1651,12 @@ def test_combined_axis_renders_one_frame_with_its_id( with the shared ID, not one step per argname. """ pytester.makepyfile( - test_comb=dedent( + test_comb_ids=dedent( """ import pytest @pytest.mark.parametrize("a,b", [(1, 2)], ids=["combined"]) - def test_comb(a, b): + def test_comb_ids(a, b): pass """ ) @@ -1677,12 +1677,12 @@ def test_combined_axis_without_ids_joins_name_value_pairs( the tuple's ``name=value`` pairs rather than pytest's noisier auto-ID. """ pytester.makepyfile( - test_comb=dedent( + test_comb_noids=dedent( """ import pytest @pytest.mark.parametrize("a,b", [(1, 2)]) - def test_comb(a, b): + def test_comb_noids(a, b): pass """ ) @@ -1699,7 +1699,7 @@ def test_callable_id_factory_on_combined_axis(pytester: pytest.Pytester, out_dir matching how pytest builds a combined axis's node-ID segment. """ pytester.makepyfile( - test_comb=dedent( + test_comb_factory=dedent( """ import pytest @@ -1707,7 +1707,7 @@ def label(value): return f"v{value}" @pytest.mark.parametrize("a,b", [(1, 2)], ids=label) - def test_comb(a, b): + def test_comb_factory(a, b): pass """ ) @@ -1723,13 +1723,13 @@ def test_combined_axis_stacked_with_single_axis(pytester: pytest.Pytester, out_d decorator outermost, each labelled by its own ID. """ pytester.makepyfile( - test_comb=dedent( + test_comb_stacked=dedent( """ import pytest @pytest.mark.parametrize("v", ["hi", "lo"]) @pytest.mark.parametrize("a,b", [(1, 2)], ids=["combined"]) - def test_comb(v, a, b): + def test_comb_stacked(v, a, b): pass """ ) @@ -1789,15 +1789,15 @@ def test_signal(handler, signal_name, case_name, mode, step): assert not [n for n in by_name if n.startswith(("handler=", "signal_name="))] # No label carries a function repr (heap address, unstable across runs). assert not [n for n in by_name if " Date: Mon, 27 Jul 2026 19:25:34 -0700 Subject: [PATCH 5/5] fix regression and add warnings --- .../_internal/pytest_plugin/steps.py | 212 +++++++++++++----- .../_tests/pytest_plugin/test_hierarchy.py | 77 ++++++- 2 files changed, 226 insertions(+), 63 deletions(-) diff --git a/python/lib/sift_client/_internal/pytest_plugin/steps.py b/python/lib/sift_client/_internal/pytest_plugin/steps.py index f5b8822f5..8a2df9948 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/steps.py +++ b/python/lib/sift_client/_internal/pytest_plugin/steps.py @@ -35,16 +35,28 @@ # pytest internals (the fixture manager, ``callspec``). If a pytest version # moves or reshapes those, we degrade to function-scoped nesting with # ``name=value`` labels instead of failing the user's collection, and surface -# the loss once per session via ``_signal_introspection_degraded``. This latch -# keeps that to a single warning; ``reset_introspection_state`` clears it at the -# start of each session (see ``pytest_configure``). -_introspection_degraded = False +# the loss once per session rather than on every item. Keyed by signal name and +# mutated in place, so adding a signal needs no new module state and no +# ``global``; ``reset_introspection_state`` clears it at the start of each +# session (see ``pytest_configure``). +_warned_once: set = set() def reset_introspection_state() -> None: - """Clear the one-shot introspection-failure latch. Called at session start.""" - global _introspection_degraded - _introspection_degraded = False + """Clear the one-shot warning latches. Called at session start.""" + _warned_once.clear() + + +def _warn_once(key: str, message: str) -> bool: + """Warn the first time ``key`` is seen this session. True if this call warned.""" + if key in _warned_once: + return False + _warned_once.add(key) + # Local import avoids a circular import (pytest_plugin imports this module). + from sift_client.pytest_plugin import SiftPytestPluginWarning + + warnings.warn(message, SiftPytestPluginWarning, stacklevel=3) + return True def _signal_introspection_degraded(detail: str) -> None: @@ -54,22 +66,42 @@ def _signal_introspection_degraded(detail: str) -> None: ordinary "no fixturedef, it's a mark-based param" case. The report still renders; scope-promoted params just fall back to function-scoped nesting. """ - global _introspection_degraded - if _introspection_degraded: - return - _introspection_degraded = True - # Local import avoids a circular import (pytest_plugin imports this module). - from sift_client.pytest_plugin import SiftPytestPluginWarning - - warnings.warn( + if _warn_once( + "introspection_degraded", "Sift pytest plugin could not read pytest internals for scope-aware " f"parametrize placement ({detail}); parametrized fixtures will render " "flat (function-scoped) with name=value labels. The rest of the report " "is unaffected. This usually means an unsupported pytest version.", - SiftPytestPluginWarning, - stacklevel=2, + ): + log_event(logger, logging.WARNING, "parametrize.introspection_degraded", detail=detail) + + +def _log_id_unresolved(argnames: str, detail: str) -> None: + """Trace one dropped ``ids=`` to the audit log, without warning.""" + log_event(logger, logging.DEBUG, "parametrize.id_unresolved", argnames=argnames, detail=detail) + + +def _signal_id_unresolved(argnames: str, detail: str) -> None: + """Trace a dropped ``ids=`` and warn once that step labels changed. + + Fired only when the author supplied ``ids=`` and we failed to use it, never + for the ordinary "no ``ids=``" case where ``name=value`` is the intended + label. An unusable ``ids=`` is otherwise invisible, and the relabel it causes + changes step identity, so those steps stop grouping across runs. + """ + _log_id_unresolved(argnames, detail) + # Keyed by axis, not globally: a pytest version we misread breaks every axis + # at once, and naming each one is what tells the author the scope. Bounded by + # the number of parametrize decorators, unlike a per-item warning, which + # would scale with the matrix and bury the signal. + _warn_once( + f"id_unresolved:{argnames}", + f"Sift pytest plugin could not match the ids= entry for parametrize " + f"argnames '{argnames}' to a declared paramset ({detail}); that axis " + "falls back to name=value step labels. Those labels differ from the " + "pytest node ID and will not group with steps from other runs. The rest " + "of the report is unaffected.", ) - log_event(logger, logging.WARNING, "parametrize.introspection_degraded", detail=detail) if TYPE_CHECKING: @@ -222,6 +254,11 @@ def _mark_argnames(mark: pytest.Mark) -> list[str]: return list(argnames) +def _mark_argvalues(mark: pytest.Mark) -> Any: + """The argvalues a ``parametrize`` mark declares, or None if absent.""" + return mark.args[1] if len(mark.args) > 1 else None + + def _param_scope(item: pytest.Item, name: str) -> str: """The pytest scope governing param ``name`` on ``item``. @@ -254,27 +291,94 @@ def _param_scope(item: pytest.Item, name: str) -> str: return "function" -def _id_from_spec(ids: Any, index: int, value: Any) -> str | None: +def _values_equal(left: Any, right: Any) -> bool: + """``left == right`` as a plain bool, False when the values refuse comparison. + + Param values are arbitrary objects. A numpy array's ``==`` returns an array + whose truth value raises, and a custom ``__eq__`` can raise anything, so an + unusable comparison means "no match" rather than an error. + """ + if left is right: + return True + try: + return bool(left == right) + except Exception: + return False + + +def _paramset_index(argvalues: Any, current: tuple) -> int | None: + """Position of the paramset holding ``current`` in a declared ``argvalues``, else None. + + ``current`` is the axis's values in argname order, so a single-name axis + passes a 1-tuple. Entries are raw values, tuples (for a combined axis), or + ``pytest.param`` wrappers, whose ``.values`` is always a tuple. + + This replaces indexing by ``callspec.indices``, which does NOT mean "position + within this axis": on pytest 8.4 a stacked axis's index tracks the expanded + callspec instead, so ``ids[index]`` silently ran off the end (or picked a + neighbour's ID). Matching the value against what the author declared is + version-independent. Duplicate paramsets resolve to the first match, which is + also the ID pytest would disambiguate with a numeric suffix. + """ + try: + entries = list(argvalues) + except TypeError: + return None + for index, entry in enumerate(entries): + candidate = getattr(entry, "values", None) # pytest.param(...) + if candidate is None: + candidate = tuple(entry) if len(current) > 1 else (entry,) + if len(candidate) == len(current) and all( + _values_equal(a, b) for a, b in zip(candidate, current) + ): + return index + return None + + +def _id_from_spec(ids: Any, argvalues: Any, current: tuple, argnames: str) -> str | None: """Resolve one axis's author-supplied ``ids`` spec to a string, or None. - A list spec is indexed by the param's position; a callable spec (an ID - factory) is invoked with the param value, mirroring how pytest builds the - node ID. A callable that raises or returns ``None`` yields None, so the + A callable spec (an ID factory) is invoked per value and the results joined + with ``-``, mirroring how pytest builds the node-ID segment; if any value + yields no ID the whole factory result is discarded rather than part-labelling + the axis. A list spec is indexed by the paramset ``current`` came from (see + ``_paramset_index``). + + A callable that raises, or a list that cannot be indexed, yields None so the caller falls back to the structured ``name=value`` label. ``None`` here also covers "the author supplied no ``ids``"; those auto-generated IDs are noisier than ``name=value`` for non-trivial values, so we never adopt them. + + Every failure AFTER the author supplied an ``ids=`` is reported through + ``_signal_id_unresolved``, since the fallback is otherwise silent. """ if ids is None: return None if callable(ids): - try: - result = ids(value) - except Exception: - return None - return str(result) if result is not None else None - if index < len(ids): + parts = [] + for value in current: + # An ID factory that returns None or raises is the author's own doing + # (pytest falls back to its auto ID, and surfaces the error itself), + # so trace it but do not warn. + try: + resolved = ids(value) + except Exception as exc: + _log_id_unresolved(argnames, f"ids factory raised {exc!r}") + return None + if resolved is None: + _log_id_unresolved(argnames, "ids factory returned None") + return None + parts.append(str(resolved)) + return "-".join(parts) + index = _paramset_index(argvalues, current) + if index is None: + _signal_id_unresolved(argnames, "value not found in the declared argvalues") + return None + try: return str(ids[index]) - return None + except (IndexError, KeyError, TypeError) as exc: + _signal_id_unresolved(argnames, f"ids[{index}] not usable ({exc!r})") + return None def _explicit_param_id(item: pytest.Item, name: str, value: Any) -> str | None: @@ -287,23 +391,25 @@ def _explicit_param_id(item: pytest.Item, name: str, value: Any) -> str | None: ``_combined_axis_label``: they render as one frame, so their shared ID needs no attribution and is adopted directly. """ - callspec = getattr(item, "callspec", None) - if callspec is None: + if getattr(item, "callspec", None) is None: return None - index = callspec.indices.get(name) - if index is None: - return None - # Fixture params: the ``ids`` spec lives on the active FixtureDef. + current = (value,) + # Fixture params: the ``ids`` spec and the declared values live on the + # active FixtureDef. An indirect param has no ``params`` of its own, so it + # falls through to the mark below. defs = _fixturedefs(item, name) if defs: - resolved = _id_from_spec(getattr(defs[-1], "ids", None), index, value) + resolved = _id_from_spec( + getattr(defs[-1], "ids", None), getattr(defs[-1], "params", None), current, name + ) if resolved is not None: return resolved - # mark.parametrize: the ``ids`` spec lives in the marker kwargs. + # mark.parametrize: the ``ids`` spec is in the marker kwargs, the declared + # values in ``mark.args[1]``. for mark in item.iter_markers("parametrize"): names = _mark_argnames(mark) if len(names) == 1 and names[0] == name: - resolved = _id_from_spec(mark.kwargs.get("ids"), index, value) + resolved = _id_from_spec(mark.kwargs.get("ids"), _mark_argvalues(mark), current, name) if resolved is not None: return resolved return None @@ -329,35 +435,19 @@ def _combined_axis_label(item: pytest.Item, mark: pytest.Mark, names: Sequence[s ID per tuple, one bracket segment in the node ID. It renders as one step, so the author's shared ``ids=`` entry labels it directly. - A callable ``ids=`` factory is applied per value and the results joined with - ``-``, mirroring how pytest builds the segment for a combined axis. If any - value yields no ID the whole factory result is discarded rather than - part-labelling the frame. - With no usable ``ids=``, fall back to the tuple's ``name=value`` pairs joined with ``, ``. Pytest's auto-generated IDs are deliberately not adopted here, for the same reason ``_id_from_spec`` rejects them: they are noisier than the structured pairs for non-trivial values. """ callspec = item.callspec # type: ignore[attr-defined] - ids = mark.kwargs.get("ids") - index = callspec.indices.get(names[0]) - if ids is not None and index is not None: - if callable(ids): - parts: list[str] = [] - for name in names: - try: - resolved = ids(callspec.params[name]) - except Exception: - resolved = None - if resolved is None: - break - parts.append(str(resolved)) - else: - return "-".join(parts) - elif index < len(ids): - return str(ids[index]) - return ", ".join(f"{name}={callspec.params[name]!r}" for name in names) + current = tuple(callspec.params[name] for name in names) + resolved = _id_from_spec( + mark.kwargs.get("ids"), _mark_argvalues(mark), current, ",".join(names) + ) + if resolved is not None: + return resolved + return ", ".join(f"{name}={value!r}" for name, value in zip(names, current)) def _param_axes(item: pytest.Item) -> tuple[tuple[tuple[str, ...], str, str], ...]: diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py index 6a63ddc5c..8f4fb190a 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_hierarchy.py @@ -1561,6 +1561,38 @@ def test_lid(v): assert by_name["two"][0]["parent_step_id"] == parent_id +def test_explicit_list_ids_survive_a_stacked_outer_axis( + pytester: pytest.Pytester, out_dir: Path +) -> None: + """``ids=`` must resolve per axis, not by the item's position in the whole matrix. + + An axis stacked under another one is where ``callspec.indices`` stops meaning + "position within this axis" (it tracks the expanded callspec on pytest 8.4), + so an ID list indexed by it runs off the end or picks a neighbour's entry. + Every ``inner`` universe must carry the same two IDs, in every ``outer`` one. + """ + pytester.makepyfile( + test_slid=dedent( + """ + import pytest + + @pytest.mark.parametrize("outer", ["o1", "o2", "o3"]) + @pytest.mark.parametrize("inner", [1, 2], ids=["one", "two"]) + def test_slid(outer, inner): + pass + """ + ) + ) + result = pytester.runpytest_inprocess("-v") + result.assert_outcomes(passed=6) + by_name = _by_name(capture.load_steps(capture.run_jsonl(out_dir))) + # One leaf per (outer, inner) pair, every one labelled by its own axis's ID. + assert len(by_name.get("one", [])) == 3 + assert len(by_name.get("two", [])) == 3 + assert "inner=1" not in by_name + assert "inner=2" not in by_name + + def test_callable_id_factory_on_inner_parametrize(pytester: pytest.Pytester, out_dir: Path) -> None: """A callable ``ids=`` factory is invoked per value, just as pytest does.""" pytester.makepyfile( @@ -2128,12 +2160,53 @@ def _raise(*_args: object, **_kwargs: object) -> None: raise RuntimeError("simulated pytest internals change") +def test_no_ids_declared_resolves_quietly() -> None: + """The ordinary "author supplied no ``ids=``" path must stay silent.""" + steps.reset_introspection_state() + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftPytestPluginWarning) + assert steps._id_from_spec(None, [1, 2], (1,), "v") is None + assert steps._warned_once == set() + + +def test_unmatchable_value_warns_once_per_axis() -> None: + """A declared ``ids=`` we cannot attribute warns once for that axis. + + Once per AXIS, not once per session: when a pytest version breaks the + resolution every axis is affected, and naming each is what shows the scope. + Repeats of the same axis stay quiet so the matrix cannot flood the output. + """ + steps.reset_introspection_state() + with pytest.warns(SiftPytestPluginWarning, match="could not match the ids="): + assert steps._id_from_spec(["one", "two"], [1, 2], (99,), "v") is None + assert steps._warned_once == {"id_unresolved:v"} + + # Same axis again: latched, no second warning. + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftPytestPluginWarning) + assert steps._id_from_spec(["one", "two"], [1, 2], (98,), "v") is None + + # A different axis is its own signal and must still be reported. + with pytest.warns(SiftPytestPluginWarning, match="argnames 'w'"): + assert steps._id_from_spec(["only"], [1], (99,), "w") is None + assert steps._warned_once == {"id_unresolved:v", "id_unresolved:w"} + + +def test_id_factory_returning_none_does_not_warn() -> None: + """An ID factory returning None is the author's choice, not a fault.""" + steps.reset_introspection_state() + with warnings.catch_warnings(): + warnings.simplefilter("error", SiftPytestPluginWarning) + assert steps._id_from_spec(lambda v: None, [1], (1,), "v") is None + assert steps._warned_once == set() + + def test_fixturedefs_degrades_when_fixture_manager_missing() -> None: steps.reset_introspection_state() item = _fake_item_without_fixture_manager() with pytest.warns(SiftPytestPluginWarning, match="scope-aware parametrize"): assert steps._fixturedefs(item, "x") is None - assert steps._introspection_degraded is True + assert "introspection_degraded" in steps._warned_once def test_fixturedefs_degrades_when_getfixturedefs_raises() -> None: @@ -2313,4 +2386,4 @@ def getfixturedefs(self, name: str, node_or_nodeid: object) -> object: warnings.simplefilter("error") # a degradation warning would raise here result = steps._fixturedefs(item, "x") assert result == (sentinel,) - assert steps._introspection_degraded is False + assert "introspection_degraded" not in steps._warned_once