From fb5364b7046f7991b35eef274d8cc43fd0ec9937 Mon Sep 17 00:00:00 2001 From: Ian Later Date: Tue, 29 Sep 2026 21:11:10 -0700 Subject: [PATCH 1/3] python(feat): archive pytest reports on create Dev runs can opt in so they drop out of the default Test Results views. An explicit false still overrides a shared true. --- .../guides/pytest_plugin/configuration.md | 51 +++-- .../low_level_wrappers/test_results.py | 5 + .../_internal/pytest_plugin/options.py | 92 +++++++- .../_internal/pytest_plugin/report.py | 3 + .../_internal/pytest_plugin/terminal.py | 20 +- .../low_level_wrappers/test_archive_replay.py | 132 ++++++++++++ .../_tests/pytest_plugin/conftest.py | 1 + .../pytest_plugin/test_archive_on_create.py | 201 ++++++++++++++++++ .../util/test_results/context_manager.py | 22 ++ 9 files changed, 510 insertions(+), 17 deletions(-) create mode 100644 python/lib/sift_client/_tests/_internal/low_level_wrappers/test_archive_replay.py create mode 100644 python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py diff --git a/python/docs/guides/pytest_plugin/configuration.md b/python/docs/guides/pytest_plugin/configuration.md index fc4d5e88f8..e51b1631e9 100644 --- a/python/docs/guides/pytest_plugin/configuration.md +++ b/python/docs/guides/pytest_plugin/configuration.md @@ -131,11 +131,13 @@ Each kind has a home chosen for a specific workflow: - **Pytest behavior** lives in `[tool.pytest.ini_options]` (log/offline/disabled/git/`*_step`/autouse/parametrize). A CLI flag exists for the ones with a real ad-hoc override workflow. - **Connection** comes from the environment first, falling back to the ini keys; the API key is env-only so secrets stay out of committed files. -- **Report content** takes static defaults from `[tool.sift.pytest.report]` and per-run dynamic values from `SIFT_REPORT_*` env vars (CI builds, hardware cycling, anything `.env`-driven; pytest-dotenv loads `.env` for local dev). +- **Report content** takes static defaults from `[tool.sift.pytest.report]` and per-run dynamic values from `SIFT_REPORT_*` env vars (CI builds, hardware cycling, anything `.env`-driven; pytest-dotenv loads `.env` for local dev). `archive_on_create` also accepts a CLI flag and an ini key. Precedence within a setting runs env > CLI flag > ini key > TOML > built-in -default. No setting exposes both env and CLI, so the chain isn't ambiguous in -practice. +default. For a boolean, an explicit `false` is a value: it overrides a `true` +from a lower-precedence source. `archive_on_create` uses every surface, so a +shared `pyproject.toml` can archive dev runs while production sets +`SIFT_REPORT_ARCHIVE_ON_CREATE=false`. The plugin scans `SIFT_*` env vars and `[tool.sift.pytest.*]` keys at session start; anything outside these tables fires a warning with a closest-match @@ -171,15 +173,16 @@ suggestion, so typos like `SIFT_REPORT_SERIALNUM` surface immediately. ### Report content -| Setting | TOML (`[tool.sift...]`) | Env var | -|---|---|---| -| Template for the report display name. Placeholders: {target}, {command}, {args}, {rootdir}, {timestamp}, {count}, {git_repo}, {git_branch}, {git_commit}. | `[tool.sift.pytest.report] name` | — | -| Template for the report's test_case field (same placeholders as report_name). | `[tool.sift.pytest.report] test_case` | — | -| Name of the test system / rig. Defaults to the host's name. | `[tool.sift.pytest.report] test_system_name` | `SIFT_REPORT_TEST_SYSTEM_NAME` | -| Operator running the test. Defaults to the OS user. | `[tool.sift.pytest.report] system_operator` | `SIFT_REPORT_SYSTEM_OPERATOR` | -| Serial number of the unit under test. | `[tool.sift.pytest.report] serial_number` | `SIFT_REPORT_SERIAL_NUMBER` | -| Part number of the unit under test. | `[tool.sift.pytest.report] part_number` | `SIFT_REPORT_PART_NUMBER` | -| Free-form report metadata, as a TOML table of scalar values. For dynamic per-run keys, override the sift_report_metadata fixture in conftest. | `[tool.sift.pytest.report.metadata]` (table) | — | +| Setting | CLI flag | Ini (`[tool.pytest.ini_options]`) | TOML (`[tool.sift...]`) | Env var | +|---|---|---|---|---| +| Template for the report display name. Placeholders: {target}, {command}, {args}, {rootdir}, {timestamp}, {count}, {git_repo}, {git_branch}, {git_commit}. | — | — | `[tool.sift.pytest.report] name` | — | +| Template for the report's test_case field (same placeholders as report_name). | — | — | `[tool.sift.pytest.report] test_case` | — | +| Name of the test system / rig. Defaults to the host's name. | — | — | `[tool.sift.pytest.report] test_system_name` | `SIFT_REPORT_TEST_SYSTEM_NAME` | +| Operator running the test. Defaults to the OS user. | — | — | `[tool.sift.pytest.report] system_operator` | `SIFT_REPORT_SYSTEM_OPERATOR` | +| Serial number of the unit under test. | — | — | `[tool.sift.pytest.report] serial_number` | `SIFT_REPORT_SERIAL_NUMBER` | +| Part number of the unit under test. | — | — | `[tool.sift.pytest.report] part_number` | `SIFT_REPORT_PART_NUMBER` | +| Archive the report right after creating it, so it drops out of the default Test Results views. An explicit false overrides a true from a lower-precedence source. | `--sift-archive-on-create` | `sift_archive_on_create` | `[tool.sift.pytest.report] archive_on_create` | `SIFT_REPORT_ARCHIVE_ON_CREATE` | +| Free-form report metadata, as a TOML table of scalar values. For dynamic per-run keys, override the sift_report_metadata fixture in conftest. | — | — | `[tool.sift.pytest.report.metadata]` (table) | — | @@ -243,6 +246,30 @@ SIFT_REPORT_SYSTEM_OPERATOR=$CI_ACTOR \ pytest tests/ ``` +### Archiving a run at creation + +`archive_on_create` archives the report immediately after the plugin creates +it. Archived reports drop out of the default Test Results views. The plugin +creates the report, then archives it in a second call. If that call fails, the +plugin logs a warning and the test session continues with the report +unarchived. + +```toml title="pyproject.toml" +[tool.sift.pytest.report] +archive_on_create = true +``` + +You can also pass `--sift-archive-on-create`, set `sift_archive_on_create` +under `[tool.pytest.ini_options]`, or set `SIFT_REPORT_ARCHIVE_ON_CREATE`. +Precedence is the environment variable, then the CLI flag, then the ini key, +then TOML. An explicit `false` overrides a `true` from a lower source, so +production can set `SIFT_REPORT_ARCHIVE_ON_CREATE=false` while the shared TOML +stays `true`. + +The terminal summary prints `(archived)` next to the report link. Offline, the +same note follows the `import-test-result-log` command. Replay of that log +archives the uploaded report. + ### `name` vs `test_case` The two fields look similar but serve opposite purposes: diff --git a/python/lib/sift_client/_internal/low_level_wrappers/test_results.py b/python/lib/sift_client/_internal/low_level_wrappers/test_results.py index e0d33dec8c..93616349b0 100644 --- a/python/lib/sift_client/_internal/low_level_wrappers/test_results.py +++ b/python/lib/sift_client/_internal/low_level_wrappers/test_results.py @@ -1338,6 +1338,11 @@ def record_created(simulated_id: str, real_id: str) -> None: real_report = await self._create_report_from_simulated(state.report) real_report_id = real_report._id_or_error record_created(state.report._id_or_error, real_report_id) + # Create has no is_archived field, so the collapsed flag goes out as an update. + if state.report.is_archived: + archive_update = TestReportUpdate(is_archived=True) + archive_update.resource_id = real_report_id + real_report = await self.update_test_report(archive_update, existing=real_report) real_steps: list[TestStep] = [] for sim_step_id in state.steps_order: diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index 6514866f15..646694699d 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -93,6 +93,9 @@ class Option: toml: tuple[str, ...] | None = None env: str | None = None merge: bool = False + # False counts as set. An unset ini default does not, so a lower TOML true + # can still apply. + explicit_bool: bool = False surfaces: tuple[str, ...] = ("env", "cli", "ini", "toml") @property @@ -157,7 +160,11 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: if not self.env: return None env_value = os.getenv(self.env) - return env_value if env_value else None + if not env_value: + return None + if self.explicit_bool: + return _coerce_explicit_bool(env_value, source=self.env) + return env_value if config is None: return None if surface == "cli": @@ -165,6 +172,8 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: if surface == "ini": if not self.ini: return None + if self.explicit_bool and not _ini_explicitly_set(config, self.ini): + return None try: ini_value = config.getini(self.ini) except (KeyError, ValueError): @@ -177,6 +186,9 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: if not self.toml: return None toml_value = _walk_toml(tool_sift(config), self.toml) + if self.explicit_bool: + source = "tool.sift." + ".".join(self.toml) + return _coerce_explicit_bool(toml_value, source=source) return toml_value if toml_value not in (None, "") else None def resolve_merged(self, config: pytest.Config | None) -> dict[str, str | float | bool]: @@ -208,6 +220,65 @@ def resolve_merged(self, config: pytest.Config | None) -> dict[str, str | float return result +_BOOL_TRUE = frozenset({"1", "true", "t", "yes", "y", "on"}) +_BOOL_FALSE = frozenset({"0", "false", "f", "no", "n", "off"}) + + +def _parse_bool_token(raw: str) -> bool | None: + """Parse a boolean token. ``None`` when ``raw`` is not a boolean word.""" + token = raw.strip().lower() + if token in _BOOL_TRUE: + return True + if token in _BOOL_FALSE: + return False + return None + + +def _coerce_explicit_bool(value: Any, *, source: str) -> bool | None: + """Coerce one surface's value to bool. Unset stays ``None``. + + A string ``false`` is ``False``, not a truthy string. Anything that is not + a bool or a boolean word warns and counts as unset. + """ + if isinstance(value, bool): + return value + if value is None or value == "": + return None + if isinstance(value, str): + parsed = _parse_bool_token(value) + if parsed is not None: + return parsed + from sift_client.pytest_plugin import SiftPytestPluginWarning + + log_event( + logger, + logging.WARNING, + "config.bool", + name=source, + value=repr(value), + ) + warnings.warn( + f"Ignoring {source}={value!r}: expected true or false.", + SiftPytestPluginWarning, + stacklevel=2, + ) + return None + + +def _ini_explicitly_set(config: pytest.Config, name: str) -> bool: + """Whether ``name`` was set in the ini file or via ``-o``, not just defaulted.""" + override = getattr(config, "_get_override_ini_value", None) + if override is not None and override(name) is not None: + return True + inicfg = getattr(config, "inicfg", None) + if inicfg is None: + return False + try: + return name in inicfg + except TypeError: + return False + + def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: """Walk a parsed TOML tree along ``path``; return None on any missing key.""" cur: Any = data @@ -445,6 +516,22 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: env="SIFT_REPORT_PART_NUMBER", toml=("pytest", "report", "part_number"), ) +# The ini default is false. explicit_bool keeps that default from hiding TOML. +ARCHIVE_ON_CREATE_OPTION = Option( + name="archive_on_create", + category=CAT_REPORT, + help="Archive the report right after creating it, so it drops out of the " + "default Test Results views. An explicit false overrides a true from a " + "lower-precedence source.", + cli="--sift-archive-on-create", + cli_action="store_true", + ini="sift_archive_on_create", + ini_type="bool", + ini_default=False, + env="SIFT_REPORT_ARCHIVE_ON_CREATE", + toml=("pytest", "report", "archive_on_create"), + explicit_bool=True, +) METADATA_OPTION = Option( name="metadata", category=CAT_REPORT, @@ -478,6 +565,7 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: SYSTEM_OPERATOR_OPTION, SERIAL_NUMBER_OPTION, PART_NUMBER_OPTION, + ARCHIVE_ON_CREATE_OPTION, METADATA_OPTION, ) @@ -565,6 +653,8 @@ def _env_cell(opt: Option) -> str: ("Env var", _env_cell), ], CAT_REPORT: [ + ("CLI flag", _cli_cell), + ("Ini (`[tool.pytest.ini_options]`)", _ini_cell), ("TOML (`[tool.sift...]`)", _toml_cell), ("Env var", _env_cell), ], diff --git a/python/lib/sift_client/_internal/pytest_plugin/report.py b/python/lib/sift_client/_internal/pytest_plugin/report.py index 4b8eb21c55..fb3a3680d2 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/report.py +++ b/python/lib/sift_client/_internal/pytest_plugin/report.py @@ -23,6 +23,7 @@ from sift_client._internal.pytest_plugin.audit_log import log_event from sift_client._internal.pytest_plugin.modes import is_offline from sift_client._internal.pytest_plugin.options import ( + ARCHIVE_ON_CREATE_OPTION, GIT_METADATA_OPTION, LOG_FILE_OPTION, METADATA_OPTION, @@ -474,6 +475,7 @@ def report_context_impl( replay_log_file=not (disabled or offline), metadata=report_metadata, audit_log=audit_log, + archive_on_create=bool(ARCHIVE_ON_CREATE_OPTION.resolve(pytestconfig)), # 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 @@ -494,6 +496,7 @@ def report_context_impl( serial=report.serial_number or "-", part=report.part_number or "-", metadata=meta_kv, + archived=report.is_archived, ) # What actually happens with the JSONL log, not the raw setting: the # effective path (temp or pinned), or "disabled", plus whether the diff --git a/python/lib/sift_client/_internal/pytest_plugin/terminal.py b/python/lib/sift_client/_internal/pytest_plugin/terminal.py index c3bd3f417b..6966c194d9 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/terminal.py +++ b/python/lib/sift_client/_internal/pytest_plugin/terminal.py @@ -136,6 +136,13 @@ def write_disabled_summary(terminalreporter: Any) -> None: terminalreporter.write_line("Sift disabled — no test report created.") +def archived_suffix(report: Any) -> str: + """`` (archived)`` when the report is archived, else empty.""" + if getattr(report, "is_archived", False): + return " (archived)" + return "" + + def write_report_summary( terminalreporter: Any, context: Any, @@ -203,10 +210,15 @@ def write_report_summary( if log_file is not None: sift_kv(terminalreporter, "Log file", str(log_file)) + archived = archived_suffix(report) if offline: if log_file is not None: terminalreporter.write_sep("-", "to upload to Sift") - terminalreporter.write_line(f" >> import-test-result-log {log_file}", cyan=True) + # The command stays the line prefix so a copied line still runs. + terminalreporter.write(f" >> import-test-result-log {log_file}", cyan=True) + if archived: + terminalreporter.write(archived) + terminalreporter.write_line("") else: if not report_id: # Incremental upload never mapped the report (the worker died before @@ -214,16 +226,16 @@ def write_report_summary( sift_kv( terminalreporter, "Report", - f"not uploaded — replay with: import-test-result-log {log_file}", + f"not uploaded — replay with: import-test-result-log {log_file}{archived}", yellow=True, ) elif report_url is not None: - sift_kv(terminalreporter, "Report", report_url, cyan=True) + sift_kv(terminalreporter, "Report", f"{report_url}{archived}", cyan=True) else: sift_kv( terminalreporter, "Report", - f"id {report_id} (set sift_app_url for a clickable link)", + f"id {report_id} (set sift_app_url for a clickable link){archived}", ) if report_id and getattr(context, "replay_incomplete", False) and log_file is not None: diff --git a/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_archive_replay.py b/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_archive_replay.py new file mode 100644 index 0000000000..f30c984240 --- /dev/null +++ b/python/lib/sift_client/_tests/_internal/low_level_wrappers/test_archive_replay.py @@ -0,0 +1,132 @@ +"""Replay keeps ``is_archived`` from a log written by ``archive_on_create``. + +Batch replay collapses the log into one create. ``CreateTestReportRequest`` +has no archive field, so the collapsed flag has to go out as a follow-up +update. Incremental replay sends the logged update as its own line. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from sift_client._internal.low_level_wrappers.test_results import ( + TestResultsLowLevelClient as ResultsLowLevelClient, +) +from sift_client.sift_types.test_report import ( + TestReport, + TestReportCreate, + TestReportUpdate, + TestStatus, +) + +T0 = datetime(2026, 1, 1, tzinfo=timezone.utc) + + +def _make_report(id_: str, *, is_archived: bool = False) -> TestReport: + return TestReport( + id_=id_, + status=TestStatus.PASSED, + name="n", + test_system_name="s", + test_case="c", + start_time=T0, + end_time=T0, + metadata={}, + is_archived=is_archived, + ) + + +def _report_create() -> TestReportCreate: + return TestReportCreate( + status=TestStatus.IN_PROGRESS, + name="n", + test_system_name="s", + test_case="c", + start_time=T0, + end_time=T0, + ) + + +def _install_create_spy(client: ResultsLowLevelClient) -> None: + """Real creates return a stand-in. Simulate and log writes stay on the client.""" + original = client.create_test_report + + async def create_spy(*args, **kwargs): + if kwargs.get("simulate") or kwargs.get("log_file") is not None: + return await original(*args, **kwargs) + return _make_report("real-report") + + client.create_test_report = create_spy # type: ignore[method-assign] + + +async def _write_archived_log(log_file, client: ResultsLowLevelClient) -> None: + report = await client.create_test_report(test_report=_report_create(), log_file=log_file) + update = TestReportUpdate(is_archived=True) + update.resource_id = report.id_ + await client.update_test_report(update=update, log_file=log_file) + + +@pytest.mark.asyncio +async def test_batch_replay_archives_after_create(tmp_path): + """The default upload path archives the report it just created.""" + log_file = tmp_path / "archived.jsonl" + client = ResultsLowLevelClient(grpc_client=MagicMock()) + await _write_archived_log(log_file, client) + + original_update = client.update_test_report + real_updates: list[TestReportUpdate] = [] + _install_create_spy(client) + + async def update_spy(*args, **kwargs): + if kwargs.get("simulate") or kwargs.get("log_file") is not None: + return await original_update(*args, **kwargs) + real_updates.append(args[0]) + return _make_report("real-report", is_archived=True) + + client.update_test_report = update_spy # type: ignore[method-assign] + + result = await client.import_log_file(log_file) + + assert len(real_updates) == 1 + assert real_updates[0].is_archived is True + assert real_updates[0].resource_id == "real-report" + assert result.report is not None + assert result.report.is_archived is True + + +@pytest.mark.asyncio +async def test_batch_replay_skips_archive_when_unset(tmp_path): + """A log with no archive update does not send one.""" + log_file = tmp_path / "plain.jsonl" + client = ResultsLowLevelClient(grpc_client=MagicMock()) + await client.create_test_report(test_report=_report_create(), log_file=log_file) + + client.update_test_report = AsyncMock() # type: ignore[method-assign] + _install_create_spy(client) + + result = await client.import_log_file(log_file) + + client.update_test_report.assert_not_called() + assert result.report is not None + assert result.report.is_archived is False + + +@pytest.mark.asyncio +async def test_incremental_replay_sends_archive_update(tmp_path): + """Line-by-line replay forwards the logged archive update.""" + log_file = tmp_path / "incremental.jsonl" + client = ResultsLowLevelClient(grpc_client=MagicMock()) + await _write_archived_log(log_file, client) + + archived = _make_report("real-report", is_archived=True) + client.create_test_report = AsyncMock(return_value=_make_report("real-report")) # type: ignore[method-assign] + client.update_test_report = AsyncMock(return_value=archived) # type: ignore[method-assign] + + await client.import_log_file(log_file, incremental=True) + + sent = client.update_test_report.await_args.kwargs["request"] + assert "is_archived" in sent.update_mask.paths + assert sent.test_report.is_archived is True diff --git a/python/lib/sift_client/_tests/pytest_plugin/conftest.py b/python/lib/sift_client/_tests/pytest_plugin/conftest.py index 96d3966345..f64035bfc2 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/conftest.py +++ b/python/lib/sift_client/_tests/pytest_plugin/conftest.py @@ -37,6 +37,7 @@ "SIFT_APP_URL", "SIFT_PROFILE", "SIFT_CONFIG_FILE", + "SIFT_REPORT_ARCHIVE_ON_CREATE", ) diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py b/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py new file mode 100644 index 0000000000..05c887f798 --- /dev/null +++ b/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py @@ -0,0 +1,201 @@ +"""``archive_on_create``: four config surfaces, explicit false, and the log. + +The plugin creates the report, then archives it. An explicit ``false`` from a +higher-precedence surface overrides a ``true`` from a lower one, so a shared +TOML default can archive dev runs while production sets the env var to false. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Callable +from unittest.mock import MagicMock + +import pytest + +from sift_client._internal.pytest_plugin.options import ARCHIVE_ON_CREATE_OPTION +from sift_client.errors import SiftWarning +from sift_client.pytest_plugin import SiftPytestPluginWarning + +if TYPE_CHECKING: + from pathlib import Path + + +def _print_archive_probe() -> str: + return """ + from sift_client._internal.pytest_plugin.options import ARCHIVE_ON_CREATE_OPTION + value, source = ARCHIVE_ON_CREATE_OPTION.resolve_with_source(config) + print(f"ARCHIVE: {value} {source}") + """ + + +class TestArchiveOnCreateResolution: + """Precedence is env, then CLI, then ini, then TOML. False is a real value.""" + + def test_env_false_is_false_not_a_string(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SIFT_REPORT_ARCHIVE_ON_CREATE", "false") + assert ARCHIVE_ON_CREATE_OPTION.resolve_with_source(None) == (False, "env") + + def test_env_true(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SIFT_REPORT_ARCHIVE_ON_CREATE", "true") + assert ARCHIVE_ON_CREATE_OPTION.resolve_with_source(None) == (True, "env") + + def test_invalid_env_warns_and_stays_unset(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("SIFT_REPORT_ARCHIVE_ON_CREATE", "maybe") + with pytest.warns(SiftPytestPluginWarning, match="expected true or false"): + assert ARCHIVE_ON_CREATE_OPTION.resolve_with_source(None) == (None, "default") + + def test_unset_is_default( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_probe_conftest: Callable[[str], None], + ) -> None: + monkeypatch.delenv("SIFT_REPORT_ARCHIVE_ON_CREATE", raising=False) + write_probe_conftest(_print_archive_probe()) + pytester.makepyfile("def test_noop(): pass") + result = pytester.runpytest_subprocess("-s", "--co") + result.stdout.fnmatch_lines(["ARCHIVE: None default"]) + + def test_toml_true( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_probe_conftest: Callable[[str], None], + ) -> None: + monkeypatch.delenv("SIFT_REPORT_ARCHIVE_ON_CREATE", raising=False) + write_probe_conftest(_print_archive_probe()) + pytester.makepyprojecttoml( + """ + [tool.sift.pytest.report] + archive_on_create = true + """ + ) + pytester.makepyfile("def test_noop(): pass") + result = pytester.runpytest_subprocess("-s", "--co") + result.stdout.fnmatch_lines(["ARCHIVE: True toml"]) + + def test_ini_false_overrides_toml_true( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_probe_conftest: Callable[[str], None], + ) -> None: + monkeypatch.delenv("SIFT_REPORT_ARCHIVE_ON_CREATE", raising=False) + write_probe_conftest(_print_archive_probe()) + pytester.makepyprojecttoml( + """ + [tool.pytest.ini_options] + sift_archive_on_create = false + + [tool.sift.pytest.report] + archive_on_create = true + """ + ) + pytester.makepyfile("def test_noop(): pass") + result = pytester.runpytest_subprocess("-s", "--co") + result.stdout.fnmatch_lines(["ARCHIVE: False ini"]) + + def test_cli_true_overrides_ini_false( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_probe_conftest: Callable[[str], None], + ) -> None: + monkeypatch.delenv("SIFT_REPORT_ARCHIVE_ON_CREATE", raising=False) + write_probe_conftest(_print_archive_probe()) + pytester.makepyprojecttoml( + """ + [tool.pytest.ini_options] + sift_archive_on_create = false + """ + ) + pytester.makepyfile("def test_noop(): pass") + result = pytester.runpytest_subprocess("-s", "--co", "--sift-archive-on-create") + result.stdout.fnmatch_lines(["ARCHIVE: True cli"]) + + def test_env_false_overrides_cli_true( + self, + pytester: pytest.Pytester, + monkeypatch: pytest.MonkeyPatch, + write_probe_conftest: Callable[[str], None], + ) -> None: + """Production can force false even when the flag and the TOML say true.""" + monkeypatch.setenv("SIFT_REPORT_ARCHIVE_ON_CREATE", "false") + write_probe_conftest(_print_archive_probe()) + pytester.makepyprojecttoml( + """ + [tool.sift.pytest.report] + archive_on_create = true + """ + ) + pytester.makepyfile("def test_noop(): pass") + result = pytester.runpytest_subprocess("-s", "--co", "--sift-archive-on-create") + result.stdout.fnmatch_lines(["ARCHIVE: False env"]) + + +class TestArchiveOnCreateReport: + """The setting archives after create, including through the offline log.""" + + def test_archive_failure_warns_and_continues(self) -> None: + from sift_client.util.test_results import ReportContext + + report = MagicMock() + report.archive.side_effect = RuntimeError("down") + client = MagicMock() + client.test_results.create.return_value = report + with pytest.warns(SiftWarning, match="Could not archive"): + context = ReportContext(client, name="n", log_file=False, archive_on_create=True) + assert context.report is report + report.archive.assert_called_once() + + def test_offline_log_and_footer_record_archive( + self, + pytester: pytest.Pytester, + tmp_path: Path, + clear_sift_env: None, + write_plugin_conftest: Callable[[], None], + ) -> None: + from sift_client._tests.pytest_plugin._step_status_capture import run_jsonl + + out_dir = tmp_path / "sift-out" + write_plugin_conftest() + pytester.makepyprojecttoml( + """ + [tool.sift.pytest.report] + archive_on_create = true + """ + ) + pytester.makepyfile("def test_one(step): pass") + result = pytester.runpytest_subprocess("--sift-offline", f"--sift-output-dir={out_dir}") + result.assert_outcomes(passed=1) + log_text = run_jsonl(out_dir).read_text() + create_at = log_text.index("[CreateTestReport:") + archive_at = log_text.index('"isArchived":true') + assert create_at < archive_at + result.stdout.fnmatch_lines(["*(archived)*"]) + + def test_env_false_skips_archive( + self, + pytester: pytest.Pytester, + tmp_path: Path, + clear_sift_env: None, + monkeypatch: pytest.MonkeyPatch, + write_plugin_conftest: Callable[[], None], + ) -> None: + from sift_client._tests.pytest_plugin._step_status_capture import run_jsonl + + monkeypatch.setenv("SIFT_REPORT_ARCHIVE_ON_CREATE", "false") + out_dir = tmp_path / "sift-out" + write_plugin_conftest() + pytester.makepyprojecttoml( + """ + [tool.sift.pytest.report] + archive_on_create = true + """ + ) + pytester.makepyfile("def test_one(step): pass") + result = pytester.runpytest_subprocess("--sift-offline", f"--sift-output-dir={out_dir}") + result.assert_outcomes(passed=1) + log_text = run_jsonl(out_dir).read_text() + assert '"isArchived":true' not in log_text + result.stdout.no_fnmatch_line("*(archived)*") 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 ad0003fed3..79241d3238 100644 --- a/python/lib/sift_client/util/test_results/context_manager.py +++ b/python/lib/sift_client/util/test_results/context_manager.py @@ -240,6 +240,7 @@ def __init__( metadata: dict[str, str | float | bool] | None = None, audit_log: str | Path | None = None, defer_finalize: bool = False, + archive_on_create: bool = False, ): """Initialize a new report context. @@ -270,6 +271,9 @@ def __init__( 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. + archive_on_create: If true, archive the report immediately after + creating it. If the archive call fails, log a warning and leave + the report unarchived. If false, leave the report unarchived. """ self.client = client self.replay_log_file = replay_log_file @@ -318,6 +322,24 @@ def __init__( metadata=combined_metadata or None, # type: ignore ) self.report = client.test_results.create(create, log_file=self.log_file) + if archive_on_create: + self._archive_report() + + def _archive_report(self) -> None: + """Archive the report. Create has no archive field, so this is a second call. + + A failure warns and leaves the report unarchived. + """ + try: + self.report.archive() + except Exception as exc: + log_event(logger, logging.WARNING, "report.archive_failed", error=repr(exc)) + warnings.warn( + f"Could not archive the Sift test report: {exc}. " + "The session continues, and the report stays unarchived.", + SiftWarning, + stacklevel=2, + ) def _build_replay_command(self) -> list[str]: """Build the argv for the background replay worker subprocess. From 63b91cd83525e1c1c437ad51361257d0fd0bb4e0 Mon Sep 17 00:00:00 2001 From: Ian Later Date: Wed, 30 Sep 2026 14:30:44 -0700 Subject: [PATCH 2/3] python(fix): keep the offline upload command copyable The archived note sat on the import command line, so copying that line would not run. Put the note on the next line. --- python/docs/guides/pytest_plugin/configuration.md | 3 ++- python/lib/sift_client/_internal/pytest_plugin/options.py | 5 +++-- python/lib/sift_client/_internal/pytest_plugin/terminal.py | 7 +++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/python/docs/guides/pytest_plugin/configuration.md b/python/docs/guides/pytest_plugin/configuration.md index e51b1631e9..bf2494c5e0 100644 --- a/python/docs/guides/pytest_plugin/configuration.md +++ b/python/docs/guides/pytest_plugin/configuration.md @@ -267,7 +267,8 @@ production can set `SIFT_REPORT_ARCHIVE_ON_CREATE=false` while the shared TOML stays `true`. The terminal summary prints `(archived)` next to the report link. Offline, the -same note follows the `import-test-result-log` command. Replay of that log +same note is the line after the `import-test-result-log` command, so the +command line itself still runs when copied. Replay of that log archives the uploaded report. ### `name` vs `test_case` diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index 646694699d..5e54cfd208 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -135,8 +135,9 @@ def resolve(self, config: pytest.Config | None) -> Any: The walk order is :attr:`surfaces`, which puts env before cli by default. ``getini`` returns the typed default for unset bool/list keys, so this - only returns ini values for booleans (always meaningful), non-empty - strings, and non-empty lists. + returns ini values for booleans, non-empty strings, and non-empty lists. + ``explicit_bool`` is the exception: an ini bool counts only when the key + is set, so the registered default does not hide a lower value. """ return self.resolve_with_source(config)[0] diff --git a/python/lib/sift_client/_internal/pytest_plugin/terminal.py b/python/lib/sift_client/_internal/pytest_plugin/terminal.py index 6966c194d9..0ae7a991b1 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/terminal.py +++ b/python/lib/sift_client/_internal/pytest_plugin/terminal.py @@ -214,11 +214,10 @@ def write_report_summary( if offline: if log_file is not None: terminalreporter.write_sep("-", "to upload to Sift") - # The command stays the line prefix so a copied line still runs. - terminalreporter.write(f" >> import-test-result-log {log_file}", cyan=True) + # (archived) is its own line so copying the command still runs. + terminalreporter.write_line(f" >> import-test-result-log {log_file}", cyan=True) if archived: - terminalreporter.write(archived) - terminalreporter.write_line("") + terminalreporter.write_line(archived) else: if not report_id: # Incremental upload never mapped the report (the worker died before From a350e9db171c023f3022b93d42b00eac30e1b312 Mon Sep 17 00:00:00 2001 From: Ian Later Date: Wed, 30 Sep 2026 18:40:39 -0700 Subject: [PATCH 3/3] python(fix): address review on archive_on_create Use pytest's None ini default so an unset key does not hide TOML, and print archived on the status row so the upload command stays copyable. --- .../guides/pytest_plugin/configuration.md | 28 ++++++------ .../_internal/pytest_plugin/options.py | 45 ++++++------------- .../_internal/pytest_plugin/terminal.py | 26 ++++------- .../pytest_plugin/test_archive_on_create.py | 4 +- .../util/test_results/context_manager.py | 1 + 5 files changed, 40 insertions(+), 64 deletions(-) diff --git a/python/docs/guides/pytest_plugin/configuration.md b/python/docs/guides/pytest_plugin/configuration.md index bf2494c5e0..17bfa95994 100644 --- a/python/docs/guides/pytest_plugin/configuration.md +++ b/python/docs/guides/pytest_plugin/configuration.md @@ -173,19 +173,23 @@ suggestion, so typos like `SIFT_REPORT_SERIALNUM` surface immediately. ### Report content -| Setting | CLI flag | Ini (`[tool.pytest.ini_options]`) | TOML (`[tool.sift...]`) | Env var | -|---|---|---|---|---| -| Template for the report display name. Placeholders: {target}, {command}, {args}, {rootdir}, {timestamp}, {count}, {git_repo}, {git_branch}, {git_commit}. | — | — | `[tool.sift.pytest.report] name` | — | -| Template for the report's test_case field (same placeholders as report_name). | — | — | `[tool.sift.pytest.report] test_case` | — | -| Name of the test system / rig. Defaults to the host's name. | — | — | `[tool.sift.pytest.report] test_system_name` | `SIFT_REPORT_TEST_SYSTEM_NAME` | -| Operator running the test. Defaults to the OS user. | — | — | `[tool.sift.pytest.report] system_operator` | `SIFT_REPORT_SYSTEM_OPERATOR` | -| Serial number of the unit under test. | — | — | `[tool.sift.pytest.report] serial_number` | `SIFT_REPORT_SERIAL_NUMBER` | -| Part number of the unit under test. | — | — | `[tool.sift.pytest.report] part_number` | `SIFT_REPORT_PART_NUMBER` | -| Archive the report right after creating it, so it drops out of the default Test Results views. An explicit false overrides a true from a lower-precedence source. | `--sift-archive-on-create` | `sift_archive_on_create` | `[tool.sift.pytest.report] archive_on_create` | `SIFT_REPORT_ARCHIVE_ON_CREATE` | -| Free-form report metadata, as a TOML table of scalar values. For dynamic per-run keys, override the sift_report_metadata fixture in conftest. | — | — | `[tool.sift.pytest.report.metadata]` (table) | — | +| Setting | TOML (`[tool.sift...]`) | Env var | +|---|---|---| +| Template for the report display name. Placeholders: {target}, {command}, {args}, {rootdir}, {timestamp}, {count}, {git_repo}, {git_branch}, {git_commit}. | `[tool.sift.pytest.report] name` | — | +| Template for the report's test_case field (same placeholders as report_name). | `[tool.sift.pytest.report] test_case` | — | +| Name of the test system / rig. Defaults to the host's name. | `[tool.sift.pytest.report] test_system_name` | `SIFT_REPORT_TEST_SYSTEM_NAME` | +| Operator running the test. Defaults to the OS user. | `[tool.sift.pytest.report] system_operator` | `SIFT_REPORT_SYSTEM_OPERATOR` | +| Serial number of the unit under test. | `[tool.sift.pytest.report] serial_number` | `SIFT_REPORT_SERIAL_NUMBER` | +| Part number of the unit under test. | `[tool.sift.pytest.report] part_number` | `SIFT_REPORT_PART_NUMBER` | +| Archive the report right after creating it, so it drops out of the default Test Results views. An explicit false overrides a true from a lower-precedence source. | `[tool.sift.pytest.report] archive_on_create` | `SIFT_REPORT_ARCHIVE_ON_CREATE` | +| Free-form report metadata, as a TOML table of scalar values. For dynamic per-run keys, override the sift_report_metadata fixture in conftest. | `[tool.sift.pytest.report.metadata]` (table) | — | +`archive_on_create` can also be set with `--sift-archive-on-create` or with +`sift_archive_on_create` in `[tool.pytest.ini_options]`. The table omits those +columns because the other report settings do not use them. + ### Quick-start examples ```toml title="pyproject.toml" @@ -266,9 +270,7 @@ then TOML. An explicit `false` overrides a `true` from a lower source, so production can set `SIFT_REPORT_ARCHIVE_ON_CREATE=false` while the shared TOML stays `true`. -The terminal summary prints `(archived)` next to the report link. Offline, the -same note is the line after the `import-test-result-log` command, so the -command line itself still runs when copied. Replay of that log +The terminal summary prints `· archived` on the status row. Replay of that log archives the uploaded report. ### `name` vs `test_case` diff --git a/python/lib/sift_client/_internal/pytest_plugin/options.py b/python/lib/sift_client/_internal/pytest_plugin/options.py index 5e54cfd208..69c6396890 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/options.py +++ b/python/lib/sift_client/_internal/pytest_plugin/options.py @@ -75,6 +75,8 @@ class Option: - ``toml``: tuple path under ``[tool.sift...]``, e.g. ``("pytest", "report", "name")`` -> ``tool.sift.pytest.report.name``. - ``env``: full env var name, e.g. ``"SIFT_API_KEY"``. + - ``value_type``: how to read env and TOML values. ``"bool"`` accepts + true/false words. Ini values use ``ini_type`` instead. - ``surfaces``: the precedence order. The default puts env before cli. Override it if a flag that the user types must outrank an environment variable, as ``profile`` does. @@ -93,9 +95,7 @@ class Option: toml: tuple[str, ...] | None = None env: str | None = None merge: bool = False - # False counts as set. An unset ini default does not, so a lower TOML true - # can still apply. - explicit_bool: bool = False + value_type: str | None = None surfaces: tuple[str, ...] = ("env", "cli", "ini", "toml") @property @@ -120,6 +120,8 @@ def __post_init__(self) -> None: raise ValueError(f"Option({self.name!r}): ini_type requires ini") if self.merge and not self.toml: raise ValueError(f"Option({self.name!r}): merge=True needs toml") + if self.value_type not in (None, "bool"): + raise ValueError(f"Option({self.name!r}): value_type must be None or 'bool'") if not any([self.cli, self.ini, self.toml, self.env]): raise ValueError(f"Option({self.name!r}): declares no surfaces") if self.category not in CATEGORIES: @@ -136,8 +138,7 @@ def resolve(self, config: pytest.Config | None) -> Any: The walk order is :attr:`surfaces`, which puts env before cli by default. ``getini`` returns the typed default for unset bool/list keys, so this returns ini values for booleans, non-empty strings, and non-empty lists. - ``explicit_bool`` is the exception: an ini bool counts only when the key - is set, so the registered default does not hide a lower value. + A ``None`` ini default counts as unset, so a lower value can still apply. """ return self.resolve_with_source(config)[0] @@ -163,8 +164,8 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: env_value = os.getenv(self.env) if not env_value: return None - if self.explicit_bool: - return _coerce_explicit_bool(env_value, source=self.env) + if self.value_type == "bool": + return _coerce_bool(env_value, source=self.env) return env_value if config is None: return None @@ -173,8 +174,6 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: if surface == "ini": if not self.ini: return None - if self.explicit_bool and not _ini_explicitly_set(config, self.ini): - return None try: ini_value = config.getini(self.ini) except (KeyError, ValueError): @@ -187,9 +186,9 @@ def _read_surface(self, surface: str, config: pytest.Config | None) -> Any: if not self.toml: return None toml_value = _walk_toml(tool_sift(config), self.toml) - if self.explicit_bool: + if self.value_type == "bool": source = "tool.sift." + ".".join(self.toml) - return _coerce_explicit_bool(toml_value, source=source) + return _coerce_bool(toml_value, source=source) return toml_value if toml_value not in (None, "") else None def resolve_merged(self, config: pytest.Config | None) -> dict[str, str | float | bool]: @@ -235,7 +234,7 @@ def _parse_bool_token(raw: str) -> bool | None: return None -def _coerce_explicit_bool(value: Any, *, source: str) -> bool | None: +def _coerce_bool(value: Any, *, source: str) -> bool | None: """Coerce one surface's value to bool. Unset stays ``None``. A string ``false`` is ``False``, not a truthy string. Anything that is not @@ -266,20 +265,6 @@ def _coerce_explicit_bool(value: Any, *, source: str) -> bool | None: return None -def _ini_explicitly_set(config: pytest.Config, name: str) -> bool: - """Whether ``name`` was set in the ini file or via ``-o``, not just defaulted.""" - override = getattr(config, "_get_override_ini_value", None) - if override is not None and override(name) is not None: - return True - inicfg = getattr(config, "inicfg", None) - if inicfg is None: - return False - try: - return name in inicfg - except TypeError: - return False - - def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: """Walk a parsed TOML tree along ``path``; return None on any missing key.""" cur: Any = data @@ -517,7 +502,7 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: env="SIFT_REPORT_PART_NUMBER", toml=("pytest", "report", "part_number"), ) -# The ini default is false. explicit_bool keeps that default from hiding TOML. +# None, not False: an unset ini key must not hide a TOML true. ARCHIVE_ON_CREATE_OPTION = Option( name="archive_on_create", category=CAT_REPORT, @@ -528,10 +513,10 @@ def _walk_toml(data: dict[str, Any], path: tuple[str, ...]) -> Any: cli_action="store_true", ini="sift_archive_on_create", ini_type="bool", - ini_default=False, + ini_default=None, env="SIFT_REPORT_ARCHIVE_ON_CREATE", toml=("pytest", "report", "archive_on_create"), - explicit_bool=True, + value_type="bool", ) METADATA_OPTION = Option( name="metadata", @@ -654,8 +639,6 @@ def _env_cell(opt: Option) -> str: ("Env var", _env_cell), ], CAT_REPORT: [ - ("CLI flag", _cli_cell), - ("Ini (`[tool.pytest.ini_options]`)", _ini_cell), ("TOML (`[tool.sift...]`)", _toml_cell), ("Env var", _env_cell), ], diff --git a/python/lib/sift_client/_internal/pytest_plugin/terminal.py b/python/lib/sift_client/_internal/pytest_plugin/terminal.py index 0ae7a991b1..1ed2877761 100644 --- a/python/lib/sift_client/_internal/pytest_plugin/terminal.py +++ b/python/lib/sift_client/_internal/pytest_plugin/terminal.py @@ -136,13 +136,6 @@ def write_disabled_summary(terminalreporter: Any) -> None: terminalreporter.write_line("Sift disabled — no test report created.") -def archived_suffix(report: Any) -> str: - """`` (archived)`` when the report is archived, else empty.""" - if getattr(report, "is_archived", False): - return " (archived)" - return "" - - def write_report_summary( terminalreporter: Any, context: Any, @@ -169,14 +162,15 @@ def write_report_summary( status_word, status_markup = "PASSED", {"green": True, "bold": True} # Offline results live only in the local log until replayed, so the status # row calls that out instead of repeating the version (already in the header). + # Archived rides on this row too. The upload lines below are copied as commands. + report = context.report + archived = " · archived" if getattr(report, "is_archived", False) else "" status_context = ( - f"{mode_label(config)} · not uploaded" + f"{mode_label(config)} · not uploaded{archived}" if offline - else f"{mode_label(config)} · sift-stack-py {sdk_version()}" + else f"{mode_label(config)} · sift-stack-py {sdk_version()}{archived}" ) - report = context.report - terminalreporter.write_sep( "=", report_panel_title(report, terminalreporter), cyan=True, bold=True ) @@ -210,14 +204,10 @@ def write_report_summary( if log_file is not None: sift_kv(terminalreporter, "Log file", str(log_file)) - archived = archived_suffix(report) if offline: if log_file is not None: terminalreporter.write_sep("-", "to upload to Sift") - # (archived) is its own line so copying the command still runs. terminalreporter.write_line(f" >> import-test-result-log {log_file}", cyan=True) - if archived: - terminalreporter.write_line(archived) else: if not report_id: # Incremental upload never mapped the report (the worker died before @@ -225,16 +215,16 @@ def write_report_summary( sift_kv( terminalreporter, "Report", - f"not uploaded — replay with: import-test-result-log {log_file}{archived}", + f"not uploaded — replay with: import-test-result-log {log_file}", yellow=True, ) elif report_url is not None: - sift_kv(terminalreporter, "Report", f"{report_url}{archived}", cyan=True) + sift_kv(terminalreporter, "Report", report_url, cyan=True) else: sift_kv( terminalreporter, "Report", - f"id {report_id} (set sift_app_url for a clickable link){archived}", + f"id {report_id} (set sift_app_url for a clickable link)", ) if report_id and getattr(context, "replay_incomplete", False) and log_file is not None: diff --git a/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py b/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py index 05c887f798..c9758f3867 100644 --- a/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py +++ b/python/lib/sift_client/_tests/pytest_plugin/test_archive_on_create.py @@ -172,7 +172,7 @@ def test_offline_log_and_footer_record_archive( create_at = log_text.index("[CreateTestReport:") archive_at = log_text.index('"isArchived":true') assert create_at < archive_at - result.stdout.fnmatch_lines(["*(archived)*"]) + result.stdout.fnmatch_lines(["*· archived*"]) def test_env_false_skips_archive( self, @@ -198,4 +198,4 @@ def test_env_false_skips_archive( result.assert_outcomes(passed=1) log_text = run_jsonl(out_dir).read_text() assert '"isArchived":true' not in log_text - result.stdout.no_fnmatch_line("*(archived)*") + result.stdout.no_fnmatch_line("*· archived*") 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 79241d3238..3c94a87581 100644 --- a/python/lib/sift_client/util/test_results/context_manager.py +++ b/python/lib/sift_client/util/test_results/context_manager.py @@ -240,6 +240,7 @@ def __init__( metadata: dict[str, str | float | bool] | None = None, audit_log: str | Path | None = None, defer_finalize: bool = False, + *, archive_on_create: bool = False, ): """Initialize a new report context.