diff --git a/clients/pytest-mergify/README.md b/clients/pytest-mergify/README.md index 5e27a0a..dbb3de3 100644 --- a/clients/pytest-mergify/README.md +++ b/clients/pytest-mergify/README.md @@ -48,12 +48,21 @@ bisection — only the tests that failed on the previous attempt are informative. The plugin asks Mergify whether the current run is such a rerun and, if so, runs only those tests; the rest are reported as deselected. -Nothing to configure: the plugin uses the token and job identity it already -has, and Mergify decides. Any other situation — a normal run, a rerun Mergify -has no previous results for, an unreachable API — runs the full suite, so the -feature can only remove work, never coverage. The feature is also enabled per -organization on Mergify's side, so it stays inactive until your organization -is opted in. +Two other answers exist. Mergify may say the previous attempt of this job +already ran every one of these tests and they all passed: the run then executes +no test and exits green, and still reports itself so the attempt is visible. +Or Mergify may stop the run outright. That happens when several runs of this +job report under the same name and run the same tests: Mergify cannot tell +which one the current run repeats, and it will not guess which tests to skip. +The run then **fails**, showing Mergify's explanation of what it saw — usually +asking you to give each of those runs its own `MERGIFY_TEST_JOB_NAME`. + +Otherwise there is nothing to configure: the plugin uses the token and job +identity it already has, and Mergify decides. Every remaining situation — a +normal run, a rerun Mergify has no previous results for, an unreachable API, an +answer from a newer Mergify this plugin does not understand — runs the full +suite, so the feature never costs coverage. It is also enabled per organization +on Mergify's side, so it stays inactive until your organization is opted in. Set `MERGIFY_TEST_SELECTION_DISABLE=true` in your CI to opt out: the plugin then always runs the full suite and never queries the endpoint. It is scoped diff --git a/clients/pytest-mergify/python/pytest_mergify/__init__.py b/clients/pytest-mergify/python/pytest_mergify/__init__.py index 9989401..273b195 100644 --- a/clients/pytest-mergify/python/pytest_mergify/__init__.py +++ b/clients/pytest-mergify/python/pytest_mergify/__init__.py @@ -294,7 +294,16 @@ def pytest_collection_modifyitems( self.mergify_ci.on_tests_collected([item.nodeid for item in items]) if self.mergify_ci.test_selection: - self.mergify_ci.test_selection.filter_items(config, items) + try: + self.mergify_ci.test_selection.filter_items(config, items) + except pytest.UsageError: + # A refusal ends the run here, before any test span exists. The + # session is still uploaded (from `pytest_sessionfinish`), and + # without this it would reach Mergify as a finished, error-free + # session holding no test — the exact shape of a run that was + # legitimately told to execute nothing. + self.has_error = True + raise def pytest_collection_finish(self, session: _pytest.main.Session) -> None: detector = self.mergify_ci.flaky_detector @@ -321,6 +330,7 @@ def pytest_collection_finish(self, session: _pytest.main.Session) -> None: def pytest_sessionfinish( self, session: _pytest.main.Session, + exitstatus: int, ) -> typing.Generator[None, None, None]: # xdist worker: export metrics via workeroutput (independent of tracer). if _is_xdist_worker(session.config): @@ -335,17 +345,45 @@ def pytest_sessionfinish( self.mergify_ci.test_retrier.to_serializable_metrics() ) - if not self._traces_enabled or self._session_span is None: - yield - return - yield # Export here rather than in the terminal summary: the summary's token # checks return early on a run with nothing to upload, but the capture # path (tests) and the debug path have spans to hand off regardless. + # Called unconditionally -- it holds the "traces off / already exported" + # guard itself, and stating it twice is how the two drift apart. self._finalize_and_export() + self._green_a_run_told_to_execute_nothing(session, exitstatus) + + def _green_a_run_told_to_execute_nothing( + self, + session: _pytest.main.Session, + exitstatus: int, + ) -> None: + """Turn "no tests ran" into a pass when Mergify asked for exactly that. + + An `empty` selection deselects the whole collection, which pytest + reports as exit code 5 — the code that means "you thought you were + running tests and you were not". Here the run did precisely what it was + told, so it exits 0. + + Outside the export above rather than inside it, because the verdict a + job reports does not depend on whether this run had traces to upload: + `PYTEST_MERGIFY_DEBUG`, or a token the plugin never got, must not turn a + deliberately empty run red. + """ + selection = self.mergify_ci.test_selection + if ( + exitstatus == pytest.ExitCode.NO_TESTS_COLLECTED + and selection is not None + and selection.selection == "empty" + # A run that collected nothing to begin with is not a run Mergify + # emptied, and its exit code 5 is the real answer. + and selection.deselected_count > 0 + ): + session.exitstatus = pytest.ExitCode.OK + def _finalize_and_export(self) -> None: """Close the session span and export every collected span, exactly once. diff --git a/clients/pytest-mergify/python/pytest_mergify/ci_insights.py b/clients/pytest-mergify/python/pytest_mergify/ci_insights.py index d44ecce..1672b59 100644 --- a/clients/pytest-mergify/python/pytest_mergify/ci_insights.py +++ b/clients/pytest-mergify/python/pytest_mergify/ci_insights.py @@ -333,6 +333,12 @@ def _load_test_selection(self, collection_fingerprint: str) -> None: selection=fetched["selection"], reason=fetched["reason"], tests=fetched["tests"], + # `.get`, unlike the keys above, even though the binding sets + # this key on every answer: the plugin already holds a wording + # for a refusal that arrives without one, so a binding that + # stopped setting it should reach the user as that fallback + # rather than as a `KeyError` crashing their pytest session. + message=fetched.get("message"), ) def _load_run_context(self) -> None: diff --git a/clients/pytest-mergify/python/pytest_mergify/test_selection.py b/clients/pytest-mergify/python/pytest_mergify/test_selection.py index 7b83286..a0d8d54 100644 --- a/clients/pytest-mergify/python/pytest_mergify/test_selection.py +++ b/clients/pytest-mergify/python/pytest_mergify/test_selection.py @@ -3,6 +3,43 @@ import _pytest.config import _pytest.nodes +import pytest + + +# What a refusal says when the server sent no wording of its own. The copy +# belongs to the server -- it can be corrected there without publishing a +# client, and it alone knows which job it is talking about -- so this is a +# fallback, not the message. +# +# Written for someone who has jobs and runs, not for someone who knows how +# Mergify stores them: "test session", "previous attempt" and "the run this one +# continues" are our vocabulary, and a reader meeting them in a red build +# learns nothing. So it opens with what happened to THEM, then why, then the +# fix with its documentation, then the way out when the fix does not apply. +# +# It still asserts no cause. A build matrix is the likely producer of several +# runs under one name, not the only one -- a job rerun on the same revision +# leaves the same signature, and renaming per matrix leg would not fix it. +# Hence the condition on the remedy, and the last line. +# +# It names no job, unlike the server's message, which formats one in: the +# plugin would have to carry the job name on every answer to enrich a string +# that renders only if the marshalling loses `message` -- a field on every +# answer to improve one that is not supposed to appear. +FALLBACK_REFUSAL_MESSAGE = ( + "Mergify Test Selection stopped this run.\n" + "\n" + "Several runs of this job report to Mergify under the same name, and they" + " run the same tests — so Mergify cannot tell which one this run repeats," + " and it will not guess which tests to skip.\n" + "\n" + "If this job runs more than once (a build matrix, for example), give each" + " run its own name with MERGIFY_TEST_JOB_NAME:\n" + "https://docs.mergify.com/ci-insights/test-frameworks/pytest/\n" + "\n" + "If this job only runs once, this is unexpected — please contact Mergify" + " support." +) @dataclasses.dataclass @@ -17,21 +54,45 @@ class TestSelection: tests the previous attempt did, so the request cannot be made before the collection is known. The bundled binding (`CiApiClient.fetch_test_selection`) fetches the answer and it is injected - here. Every error, timeout, or unknown situation degrades to running the - full suite — this feature can only remove work, never correctness. + here. + + Four answers are understood: + + * `full` -- run everything. + * `subset` -- run only `tests`. + * `empty` -- run nothing: the predecessor's attempt of this job already ran + these tests and they passed. The run exits green having executed none of + them, and still uploads its session. + * `refused` -- Mergify holds several candidate sessions for this job and + will not guess between them. The run FAILS, showing the server's own + explanation (`message`), or `FALLBACK_REFUSAL_MESSAGE` if it sent none. + + Every error, timeout, and every answer outside that list degrades to + running the full suite — the feature can remove work, never correctness, + and a client is routinely older than the server it talks to. """ - selection: typing.Literal["full", "subset"] = "full" + selection: typing.Literal["full", "subset", "empty", "refused"] = "full" reason: str = "not_requested" tests: typing.List[str] = dataclasses.field(default_factory=list) + # What the server wants shown to the CI user about this answer, when it has + # something to say -- today only a refusal does. Shown verbatim: the wording + # is the server's so it can be improved without publishing a client. + message: typing.Optional[str] = None init_error_msg: typing.Optional[str] = None kept_count: typing.Optional[int] = dataclasses.field(init=False, default=None) deselected_count: int = dataclasses.field(init=False, default=0) def __post_init__(self) -> None: - # A subset is only honoured with a non-empty list; anything else (a - # `full` answer, or a `subset` the server sent empty) runs everything. - if not (self.selection == "subset" and self.tests): + # `empty` and `refused` are answers in themselves and carry no tests. + # A subset is only honoured with a non-empty list; anything else -- a + # `full` answer, a `subset` the server sent empty, or a variant this + # client predates -- runs everything. Acting on a value we cannot + # reason about is the one outcome that loses coverage silently, on a + # run that reports green. + if self.selection in ("empty", "refused"): + self.tests = [] + elif not (self.selection == "subset" and self.tests): self.selection = "full" self.tests = [] @@ -40,14 +101,31 @@ def filter_items( config: _pytest.config.Config, items: typing.List[_pytest.nodes.Item], ) -> None: - """Reduce the collected items to the served subset, in place. + """Apply the served answer to the collected items, in place. Matching is by exact nodeid — the identifiers Mergify serves are the ones this plugin previously uploaded. Served names absent from the collection are ignored; if NOTHING matches (e.g. the tests were renamed since the previous attempt), the full suite runs — an empty reduced run would turn green without testing anything. + + Raises `pytest.UsageError` on a refusal, which is what fails the run, + carrying the server's explanation of it. """ + if self.selection == "refused": + # Deliberately not the degradation path. Everywhere else, a shape + # Mergify cannot resolve costs time and nothing else; here it is + # Mergify saying it holds several candidate predecessors for this + # job, which means one job name is standing for several runs. That + # keeps the reporting wrong for every future attempt, so it has to + # be seen and fixed rather than absorbed into a full run nobody + # notices. + raise pytest.UsageError(self.message or FALLBACK_REFUSAL_MESSAGE) + + if self.selection == "empty": + self._deselect_everything(config, items) + return + if self.selection != "subset": return @@ -66,10 +144,41 @@ def filter_items( self.kept_count = len(kept) self.deselected_count = len(deselected) + def _deselect_everything( + self, + config: _pytest.config.Config, + items: typing.List[_pytest.nodes.Item], + ) -> None: + """Empty the collection through pytest's own deselection path. + + Deselecting rather than stopping the session is what keeps the rest of + the run intact: the session still finishes, so it still uploads. A + `pytest.exit` here would be shorter and would make the one job that + legitimately ran nothing the only one missing from Mergify's reporting. + + A collection that is already empty is left alone, counters included: the + run is then red for a reason of its own (a `-k` matching nothing), and + recording an application would have this answer both green that exit + code and announce a skip over a suite it never emptied. + """ + if not items: + return + + self.deselected_count = len(items) + deselected = list(items) + items[:] = [] + config.hook.pytest_deselected(items=deselected) + def report(self) -> str: report_str = f"""✂️ Test selection - Selection: {self.selection} (reason: {self.reason}) """ if self.selection == "subset" and self.kept_count is not None: report_str += f"- Reduced rerun: executing {self.kept_count} previously-failing test(s), {self.deselected_count} deselected\n" + elif self.selection == "empty" and self.deselected_count: + # "selected", not "collected": this hook runs `trylast`, so the + # count is what survived the user's own filters, and pytest's own + # header two lines above already spends "collected" on the number + # before them. + report_str += f"- Skipped rerun: executing no test, the previous attempt of this job ran all {self.deselected_count} selected test(s) and they passed\n" return report_str diff --git a/clients/pytest-mergify/src/lib.rs b/clients/pytest-mergify/src/lib.rs index 62e90f1..119939d 100644 --- a/clients/pytest-mergify/src/lib.rs +++ b/clients/pytest-mergify/src/lib.rs @@ -114,8 +114,8 @@ impl CiApiClient { } } - /// The test selection as a dict (`selection`, `reason`, `tests`), or `None` - /// when test selection is not enabled for the repository. + /// The test selection as a dict (`selection`, `reason`, `tests`, `message`), + /// or `None` when test selection is not enabled for the repository. /// /// `collection_fingerprint` is what [`compute_test_collection_fingerprint`] /// returned for the tests this run collected — which is why the plugin asks @@ -201,13 +201,19 @@ fn test_selection_dict(py: Python<'_>, selection: &TestSelection) -> PyResult typing.Dict[str, typing.Any]: return {kv.key: _decode_any_value(kv.value) for kv in key_values} +# OTLP's `Status.code` enum, as the names this plugin hands the binding. +_UPLOADED_STATUS = {0: "unset", 1: "ok", 2: "error"} + + @dataclasses.dataclass class UploadedSpan: name: str attributes: typing.Dict[str, typing.Any] + status: str = "unset" @dataclasses.dataclass @@ -341,6 +347,7 @@ class _OTLPServer(socketserver.TCPServer): def __init__(self, *args: typing.Any, **kwargs: typing.Any) -> None: self.bodies: typing.List[bytes] = [] + self.test_selection: typing.Optional[typing.Dict[str, typing.Any]] = None super().__init__(*args, **kwargs) @@ -359,7 +366,21 @@ def do_POST(self) -> None: def do_GET(self) -> None: # Quarantine and test selection share this base URL. Answering 404 keeps - # them out of the way without pretending they were served. + # them out of the way without pretending they were served -- unless the + # test asked for a selection to be served, which is the only way a run + # in a *subprocess* can be given one (the in-process fake client never + # reaches it). + if self.path.split("?")[0].endswith("/test-selection") and ( + self.server.test_selection is not None + ): + payload = json.dumps(self.server.test_selection).encode("utf-8") + self.send_response(200) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(payload))) + self.end_headers() + self.wfile.write(payload) + return + self.send_response(404) self.end_headers() @@ -380,6 +401,10 @@ class OTLPCollector: url: str _server: _OTLPServer + def serve_test_selection(self, payload: typing.Dict[str, typing.Any]) -> None: + """Answer the test-selection endpoint with `payload` from now on.""" + self._server.test_selection = payload + @property def batches(self) -> typing.List[UploadedBatch]: batches = [] @@ -393,6 +418,7 @@ def batches(self) -> typing.List[UploadedBatch]: UploadedSpan( name=span.name, attributes=_decode_attributes(span.attributes), + status=_UPLOADED_STATUS[span.status.code], ) for scope_spans in resource_spans.scope_spans for span in scope_spans.spans @@ -413,6 +439,26 @@ def span_names(self) -> typing.Set[str]: return {span.name for batch in self.batches for span in batch.spans} +def configure_upload( + monkeypatch: pytest.MonkeyPatch, + collector: OTLPCollector, +) -> None: + """A CI whose traces and fetches both go to `collector`, over real HTTP. + + For a run in a *subprocess*, which is the only kind that actually puts a + payload on the wire -- and the only kind `install_fake_api_client` cannot + reach, since it patches this process. + """ + monkeypatch.setenv("CI", "true") + monkeypatch.setenv("GITHUB_ACTIONS", "true") + monkeypatch.setenv("GITHUB_REPOSITORY", "Mergifyio/pytest-mergify") + monkeypatch.setenv("MERGIFY_TOKEN", "token") + monkeypatch.setenv("MERGIFY_API_URL", collector.url) + # Both of these swap the exporter for one that uploads nothing. + monkeypatch.delenv("_PYTEST_MERGIFY_TEST", raising=False) + monkeypatch.delenv("PYTEST_MERGIFY_DEBUG", raising=False) + + @pytest.fixture def otlp_collector() -> typing.Generator[OTLPCollector, None, None]: with _OTLPServer(("127.0.0.1", 0), _OTLPRequestHandler) as httpd: diff --git a/clients/pytest-mergify/tests/test_test_selection.py b/clients/pytest-mergify/tests/test_test_selection.py index f9a6240..5e2be4c 100644 --- a/clients/pytest-mergify/tests/test_test_selection.py +++ b/clients/pytest-mergify/tests/test_test_selection.py @@ -27,6 +27,14 @@ def pytest_deselected(self, items: typing.List[FakeItem]) -> None: self.deselected.extend(items) +# Stands in for the engine's own copy, which is required on a refusal +# (`web/api/ci_insights/test_selection/types.py`, `AMBIGUOUS_TEST_SESSIONS_MESSAGE`). +# Deliberately not a copy of that text: the plugin shows whatever arrives, +# verbatim, and never reads it -- so a fixture quoting the real wording would +# only give this diff a second wording to keep in step with the server's. +_SERVED_REFUSAL_MESSAGE = "" + + @dataclasses.dataclass class FakeConfig: hook: FakeHook = dataclasses.field(default_factory=FakeHook) @@ -99,14 +107,17 @@ def test_subset_without_tests_normalises_to_full() -> None: assert selection.tests == [] -@pytest.mark.parametrize("served", ["empty", "a-variant-this-client-predates"]) +@pytest.mark.parametrize( + "served", ["a-variant-this-client-predates", "partial", "none", ""] +) def test_an_unrecognised_selection_runs_everything(served: str) -> None: - # The server may answer with a `selection` this client predates -- `empty` - # ("run no test, the predecessor already ran them and they passed") is the - # first one. Anything the client cannot reason about must become "run the - # full suite", never "run nothing": skipping tests on a value we do not - # understand is the one outcome that loses coverage, and it would do so - # silently, on a run that reports green. + # The server may answer with a `selection` this client predates. Anything + # the client cannot reason about must become "run the full suite", never + # "run nothing" and never a failure: acting on a value we do not understand + # is what loses coverage, and it would do so silently, on a run that reports + # green. This is the property that lets the engine grow new answers without + # breaking the clients already published -- `empty` and `refused` below were + # both served through it before this client knew them. # # The annotation is a `Literal`, but the value crosses the wire as a plain # string (the binding hands over a `Dict[str, Any]`), so this is the shape @@ -115,6 +126,84 @@ def test_an_unrecognised_selection_runs_everything(served: str) -> None: assert selection.selection == "full" +def test_an_empty_selection_deselects_the_whole_collection() -> None: + selection = test_selection.TestSelection( + selection="empty", reason="predecessor_job_succeeded" + ) + # Not normalised away: "run nothing" is an answer, unlike a `subset` that + # arrived without its tests. + assert selection.selection == "empty" + + items = [FakeItem("tests/a.py::test_one"), FakeItem("tests/b.py::test_two")] + config = FakeConfig() + selection.filter_items(config, items) # type: ignore[arg-type] + + assert items == [] + # Through pytest's own deselection hook, so the run reports two deselected + # tests rather than a collection that mysteriously came up empty. + assert [item.nodeid for item in config.hook.deselected] == [ + "tests/a.py::test_one", + "tests/b.py::test_two", + ] + assert selection.deselected_count == 2 + assert "executing no test" in selection.report() + assert "all 2 selected test(s)" in selection.report() + + +def test_a_refusal_raises_rather_than_degrading() -> None: + # The one answer that is not allowed to fall back to a full run: Mergify is + # saying one job name stands for several runs, which stays wrong for every + # future attempt until someone changes the configuration. + selection = test_selection.TestSelection( + selection="refused", + reason="ambiguous_test_sessions", + message=_SERVED_REFUSAL_MESSAGE, + ) + assert selection.selection == "refused" + + items = [FakeItem("tests/a.py::test_one")] + config = FakeConfig() + with pytest.raises(pytest.UsageError) as raised: + selection.filter_items(config, items) # type: ignore[arg-type] + + # The server's wording, verbatim. Not a paraphrase and not a client-side + # string: the server names the job and can be corrected without publishing + # a plugin, so a client that rewords it goes stale the day it is improved. + assert str(raised.value) == _SERVED_REFUSAL_MESSAGE + # And the collection is untouched, so nothing half-applied the answer. + assert [item.nodeid for item in items] == ["tests/a.py::test_one"] + + +def test_a_refusal_without_a_message_still_explains_itself() -> None: + # Not an engine we can point at: `refused` was born carrying a required + # `message`, so no deployed version serves one without. The branch guards a + # regression on THIS side -- a `set_item` dropped from the marshalling, + # exactly the failure the binding's own docstring warns about, which does + # not break a build and does not fail a test. The run must still fail with + # something a reader can act on rather than a bare exit code. + selection = test_selection.TestSelection( + selection="refused", reason="ambiguous_test_sessions" + ) + + items = [FakeItem("tests/a.py::test_one")] + config = FakeConfig() + with pytest.raises(pytest.UsageError) as raised: + selection.filter_items(config, items) # type: ignore[arg-type] + + message = str(raised.value) + assert message == test_selection.FALLBACK_REFUSAL_MESSAGE + # Says up front that the run was stopped -- the reader's own situation, not + # a justification of ours -- then the remedy with the page documenting it, + # then a way out for the cases a rename does not fix, because several runs + # under one job name is an observation and not a diagnosis of a matrix. + assert message.startswith("Mergify Test Selection stopped this run.") + assert "MERGIFY_TEST_JOB_NAME" in message + # A link that rots or was invented is worse than none: this is the page the + # repository points at everywhere else, and it documents the variable. + assert "https://docs.mergify.com/ci-insights/test-frameworks/pytest/" in message + assert "support" in message + + # The lifecycle above is unit-level. What follows runs the plugin over a real # collection, because the one thing unit tests cannot show is *when* the # selection is asked for: the request carries the fingerprint of the collected @@ -347,3 +436,165 @@ def test_an_xdist_worker_reports_no_fingerprint( resource = plugin.mergify_ci.resource_attributes assert resource is not None assert "test.collection.fingerprint" not in resource + + +def test_an_empty_selection_runs_nothing_and_exits_green( + pytester: _pytest.pytester.Pytester, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The whole point of the answer: the job is red-or-green like any other, and + # a run that legitimately executed nothing has to be green. pytest's own + # verdict on an empty collection is exit code 5, so this is the assertion + # that matters -- `assert_outcomes` alone would pass on a red run. + result, plugin, calls = _run_with_selection( + pytester, + monkeypatch, + _TWO_TESTS, + served={ + "selection": "empty", + "reason": "predecessor_job_succeeded", + "tests": [], + }, + ) + + assert result.ret == pytest.ExitCode.OK + result.assert_outcomes(passed=0, failed=0, deselected=2) + assert len(calls) == 1 + assert plugin.mergify_ci.test_selection is not None + assert plugin.mergify_ci.test_selection.selection == "empty" + result.stdout.fnmatch_lines(["*executing no test*all 2 selected test(s)*"]) + + +def test_an_empty_selection_still_uploads_its_session( + pytester: _pytest.pytester.Pytester, + monkeypatch: pytest.MonkeyPatch, + otlp_collector: conftest.OTLPCollector, +) -> None: + # The half that disappears in silence if it is forgotten. Running no test is + # the most visible thing this feature does, so a job that legitimately ran + # nothing must still show up in Mergify -- otherwise it is the only one + # missing from the reporting, and it is the one a developer comes asking + # about. Asserted on the decoded payload rather than on the plugin's own + # state, because a run can hold a finished session span and have uploaded + # nothing. + conftest.configure_upload(monkeypatch, otlp_collector) + # The coordinates the answer is keyed on, as `_run_with_selection` sets them + # for the in-process runs above. + monkeypatch.setenv("GITHUB_HEAD_REF", "queue/main/42") + monkeypatch.setenv("GITHUB_SHA", "cafecafe") + monkeypatch.setenv("GITHUB_WORKFLOW", "CI") + monkeypatch.setenv("GITHUB_JOB", "unit") + otlp_collector.serve_test_selection( + {"selection": "empty", "reason": "predecessor_job_succeeded"} + ) + pytester.makepyfile(_TWO_TESTS) + + result = pytester.runpytest_subprocess() + + assert result.ret == pytest.ExitCode.OK + result.assert_outcomes(passed=0, deselected=2) + (batch,) = otlp_collector.batches + # The session, and only the session: zero test executed is zero test span. + assert [span.name for span in batch.spans] == ["pytest session start"] + # And it carries the collection it was answered on, which is what lets + # Mergify answer the attempt after this one. + assert batch.resource_attributes[ + "test.collection.fingerprint" + ] == conftest.collection_fingerprint( + [ + "test_an_empty_selection_still_uploads_its_session.py::test_kept", + "test_an_empty_selection_still_uploads_its_session.py::test_filtered_out", + ] + ) + + +def test_a_refusal_fails_the_run( + pytester: _pytest.pytester.Pytester, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # The other answer that must not degrade. A full run here would be the + # comfortable outcome and the wrong one: nobody would ever learn that this + # job's name covers several runs, and the reduced reruns would stay off + # forever with no symptom. + result, _, calls = _run_with_selection( + pytester, + monkeypatch, + _TWO_TESTS, + served={ + "selection": "refused", + "reason": "ambiguous_test_sessions", + "tests": [], + "message": _SERVED_REFUSAL_MESSAGE, + }, + ) + + # `USAGE_ERROR`, specifically: the run stops on something the user has to + # change, which is what pytest's own exit codes call this, and it tells a + # deliberate refusal apart from the plugin having crashed (`INTERNAL_ERROR`). + assert result.ret == pytest.ExitCode.USAGE_ERROR + result.assert_outcomes(passed=0, failed=0) + assert len(calls) == 1 + assert _SERVED_REFUSAL_MESSAGE in result.stderr.str() + result.stdout.str() + + +def test_an_empty_selection_over_an_empty_collection_stays_an_error( + pytester: _pytest.pytester.Pytester, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # `-k` leaves nothing to run, so pytest's exit code 5 is the honest answer + # and not something this plugin emptied. Greening it would hide a mistyped + # filter behind a Mergify answer -- exactly the "green run that tested + # nothing" the whole feature is built to avoid. + result, plugin, _ = _run_with_selection( + pytester, + monkeypatch, + _TWO_TESTS, + "-k", + "matches-no-test", + served={ + "selection": "empty", + "reason": "predecessor_job_succeeded", + "tests": [], + }, + ) + + assert result.ret == pytest.ExitCode.NO_TESTS_COLLECTED + assert plugin.mergify_ci.test_selection is not None + assert plugin.mergify_ci.test_selection.deselected_count == 0 + # And the Mergify section says nothing about a skip: announcing that a + # previous attempt ran and passed "all 0 selected test(s)" would send whoever + # is debugging that red job to look at Mergify instead of at their filter. + assert "Skipped rerun" not in result.stdout.str() + + +def test_a_refused_run_uploads_a_session_marked_failed( + pytester: _pytest.pytester.Pytester, + monkeypatch: pytest.MonkeyPatch, + otlp_collector: conftest.OTLPCollector, +) -> None: + # A refusal produces the same payload shape as an `empty` answer -- one + # session span, no test span -- and the exit code that tells them apart + # never leaves the machine. Without a status on the session, Mergify is + # handed a clean, complete-looking run of a job that in fact refused to run, + # for a job name it has just said is ambiguous. + conftest.configure_upload(monkeypatch, otlp_collector) + monkeypatch.setenv("GITHUB_HEAD_REF", "queue/main/42") + monkeypatch.setenv("GITHUB_SHA", "cafecafe") + monkeypatch.setenv("GITHUB_WORKFLOW", "CI") + monkeypatch.setenv("GITHUB_JOB", "unit") + otlp_collector.serve_test_selection( + { + "selection": "refused", + "reason": "ambiguous_test_sessions", + "message": _SERVED_REFUSAL_MESSAGE, + } + ) + pytester.makepyfile(_TWO_TESTS) + + result = pytester.runpytest_subprocess() + + assert result.ret == pytest.ExitCode.USAGE_ERROR + (batch,) = otlp_collector.batches + (span,) = batch.spans + assert span.name == "pytest session start" + assert span.status == "error" diff --git a/clients/pytest-mergify/tests/test_uploaded_spans.py b/clients/pytest-mergify/tests/test_uploaded_spans.py index c72d904..ca1d33a 100644 --- a/clients/pytest-mergify/tests/test_uploaded_spans.py +++ b/clients/pytest-mergify/tests/test_uploaded_spans.py @@ -6,26 +6,12 @@ from tests import conftest -def _configure_upload( - monkeypatch: pytest.MonkeyPatch, - collector: conftest.OTLPCollector, -) -> None: - monkeypatch.setenv("CI", "true") - monkeypatch.setenv("GITHUB_ACTIONS", "true") - monkeypatch.setenv("GITHUB_REPOSITORY", "Mergifyio/pytest-mergify") - monkeypatch.setenv("MERGIFY_TOKEN", "token") - monkeypatch.setenv("MERGIFY_API_URL", collector.url) - # Both of these swap the exporter for one that uploads nothing. - monkeypatch.delenv("_PYTEST_MERGIFY_TEST", raising=False) - monkeypatch.delenv("PYTEST_MERGIFY_DEBUG", raising=False) - - def test_a_run_uploads_its_spans( pytester: _pytest.pytester.Pytester, monkeypatch: pytest.MonkeyPatch, otlp_collector: conftest.OTLPCollector, ) -> None: - _configure_upload(monkeypatch, otlp_collector) + conftest.configure_upload(monkeypatch, otlp_collector) pytester.makepyfile("def test_pass(): pass") result = pytester.runpytest_subprocess() @@ -45,7 +31,7 @@ def test_an_uploaded_span_carries_its_attributes( ) -> None: # Asserting on the decoded payload rather than on terminal text: a run can # print a run id and still have uploaded nothing. - _configure_upload(monkeypatch, otlp_collector) + conftest.configure_upload(monkeypatch, otlp_collector) pytester.makepyfile("def test_pass(): pass") result = pytester.runpytest_subprocess() @@ -79,7 +65,7 @@ def test_the_uploaded_fingerprint_describes_the_uploaded_tests( # catches them drifting apart -- the failure mode of MRGFY-8695, where a # runner built one name and its reporter uploaded another, and quarantine # silently matched nothing ever after. - _configure_upload(monkeypatch, otlp_collector) + conftest.configure_upload(monkeypatch, otlp_collector) pytester.makepyfile( """ import pytest @@ -125,7 +111,7 @@ def test_no_session_of_a_distributed_run_claims_a_fingerprint( # keeping collection out of the controller, which is a third-party detail # this repo pins nowhere else; this runs the real thing rather than # simulating a worker with an environment variable. - _configure_upload(monkeypatch, otlp_collector) + conftest.configure_upload(monkeypatch, otlp_collector) pytester.makepyfile( """ def test_one(): pass diff --git a/clients/ts/packages/native/src/lib.rs b/clients/ts/packages/native/src/lib.rs index b304a44..48c0e0f 100644 --- a/clients/ts/packages/native/src/lib.rs +++ b/clients/ts/packages/native/src/lib.rs @@ -166,6 +166,17 @@ pub struct TestSelection { pub tests: Option>, } +// A plain comment, not a doc one: `napi` copies `///` into the generated +// `index.d.ts`, and this says nothing a caller of that interface needs. +// +// `ApiTestSelection` also carries `message` -- the server's own explanation of +// a `refused` answer -- and it is deliberately NOT mirrored here. These clients +// collapse anything that is not a `subset` to a full run and never show the +// user a reason, so the field would be dead weight on the interface. Whoever +// wires the refusal path here (test selection is off for vitest and playwright +// during the pilot, MRGFY-8906) should carry it over rather than write the +// wording client-side: the copy is the server's so it can be corrected without +// publishing a client. impl From for TestSelection { fn from(selection: ApiTestSelection) -> Self { Self { diff --git a/crates/mergify-ci-api/src/models.rs b/crates/mergify-ci-api/src/models.rs index b2d43ee..d738c65 100644 --- a/crates/mergify-ci-api/src/models.rs +++ b/crates/mergify-ci-api/src/models.rs @@ -76,7 +76,10 @@ pub struct FlakyDetectionContext { /// to the collected items it compares against. #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] pub struct TestSelection { - /// `"full"` (run everything) or `"subset"` (run only `tests`). + /// Which kind of answer this is — `"full"` (run everything), `"subset"` + /// (run only `tests`), or any value a client may not know: see the + /// forward-compatibility test below for why this is a `String` and not an + /// enum. pub selection: String, /// Why the server chose this selection — surfaced in the plugin report. pub reason: String, @@ -85,6 +88,12 @@ pub struct TestSelection { /// variant this client predates included; a protocol break for `"subset"`. /// An `Option` field is optional to serde, so a missing key is `None`. pub tests: Option>, + /// A human-readable explanation to show the CI user, when the answer has + /// one — today only a refusal does. The copy is the server's on purpose: + /// it can be corrected without publishing a client, so a client that shows + /// it verbatim keeps being right about answers written after it shipped. + /// `None` for every answer carrying none. + pub message: Option, } #[cfg(test)] @@ -141,6 +150,28 @@ mod tests { assert!(ctx.budget_ratio_for_test_retries.abs() < f64::EPSILON); } + #[test] + fn deserializes_a_refusal_carrying_the_servers_message() { + let json = r#"{"selection":"refused","reason":"ambiguous_test_sessions","message":"several test sessions reported the same tests"}"#; + let selection: TestSelection = serde_json::from_str(json).unwrap(); + assert_eq!(selection.selection, "refused"); + assert_eq!( + selection.message.unwrap(), + "several test sessions reported the same tests" + ); + // A refusal carries no subset: there is nothing for the caller to run. + assert_eq!(selection.tests, None); + } + + #[test] + fn deserializes_an_answer_without_a_message() { + // Every `full` and `subset` answer, and any older server: a missing + // `message` is `None`, never a decode failure. + let json = r#"{"selection":"full","reason":"no_predecessor"}"#; + let selection: TestSelection = serde_json::from_str(json).unwrap(); + assert!(selection.message.is_none()); + } + #[test] fn deserializes_subset_test_selection() { let json = r#"{"selection":"subset","reason":"queue_rerun","tests":["t::a","t::b"]}"#; @@ -159,20 +190,24 @@ mod tests { assert!(selection.tests.is_none()); } - // A published client is older than the server it talks to: the server may - // answer with a `selection` variant that did not exist when the client was - // built (`empty` -- run nothing, the predecessor already ran them -- is the - // first one). Decoding must still succeed, so that the client's own - // normalisation can fall back to running the full suite. Keeping - // `selection` a plain `String` rather than a closed enum is what makes that - // true: an enum would reject the payload outright and every already-shipped - // client would stop selecting -- or worse, fail its run -- the day the - // server ships a new variant. + // A published client is older than the server it talks to, and this is the + // property that lets the server grow answers without breaking it: decoding + // must succeed on a `selection` the client has never heard of, so that the + // client's own normalisation can fall back to running the full suite. + // Keeping `selection` a plain `String` rather than a closed enum is what + // makes that true -- an enum would reject the payload outright and every + // already-shipped client would stop selecting, or worse fail its run, the + // day the server ships a new variant. + // + // The payload is a value nothing implements, deliberately: `empty` and + // `refused` both reached published clients through this property and are + // now understood by pytest-mergify, so either of them would leave this test + // green while no longer testing what it is named after. #[test] fn deserializes_a_selection_variant_this_client_predates() { - let json = r#"{"selection":"empty","reason":"predecessor_job_succeeded"}"#; + let json = r#"{"selection":"a-variant-this-client-predates","reason":"whatever"}"#; let selection: TestSelection = serde_json::from_str(json).unwrap(); - assert_eq!(selection.selection, "empty"); + assert_eq!(selection.selection, "a-variant-this-client-predates"); assert_eq!(selection.tests, None); } }