From d83db1002591efcefdea693335a9f8cc3da6c83b Mon Sep 17 00:00:00 2001 From: FENG <223868679+MakerFlyFly@users.noreply.github.com> Date: Sun, 25 Oct 2026 06:31:12 +0000 Subject: [PATCH 1/2] fix(ci): fail tests-ok when workspace discovery fails Require successful member discovery before accepting the pytest result. Only accept a skipped matrix when discovery explicitly produced []. Exercise the actual gate shell and its diagnostics, probe the required Bash environment, and align the branch-protection explanation. Generated-by: Codex (GPT-6) --- .asf.yaml | 6 +- .github/workflows/tests.yml | 50 +++-- tools/dev/tests/test_ci_tests_gate.py | 254 ++++++++++++++++++++++++++ 3 files changed, 287 insertions(+), 23 deletions(-) create mode 100644 tools/dev/tests/test_ci_tests_gate.py diff --git a/.asf.yaml b/.asf.yaml index a1e7489b4..3b432d413 100644 --- a/.asf.yaml +++ b/.asf.yaml @@ -169,9 +169,9 @@ github: # protection and rulesets both require exact-match status # check names, so listing matrix entries here would force # an `.asf.yaml` change on every matrix add / rename / - # removal. `tests-ok` `needs:` every matrix entry and - # fails unless all succeed, so the gate semantics are the - # same. + # removal. `tests-ok` requires successful member discovery + # and a passing matrix (or a confirmed empty selection + # when the matrix is skipped). - "tests-ok" # TEMPORARILY DISABLED — see the note above. Restore by removing # the three leading `# ` markers; the values are the ones that were diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index bc50a534f..f07174dda 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -60,10 +60,12 @@ jobs: # - Applicability mirrors `tools/dev/run-workspace-check.sh`: a # member with `[tool.pytest.ini_options]` and no # `[tool.magpie.checks] skip = [..., "pytest"]`. - # - Path filter (PRs only): a member is picked when its own - # Python source/test files changed. On push to `main`, and when - # a shared build input changes (root `pyproject.toml`, - # `uv.lock`, the helpers, this workflow), every member is. + # - Path filter (PRs only): the helper selects members whose + # eligible files or linked sources changed, plus their + # dependents; `member_changed` documents the exclusions. + # On push to `main`, and when a shared build input changes + # (root `pyproject.toml`, `uv.lock`, the helpers, this workflow), + # every member is selected. # - Grouping: most of a job is setup, so up to six members get a # job each (named after the member, as before); more are split # into at most eight jobs balanced by test size. @@ -95,16 +97,15 @@ jobs: # pull request touching a few projects gets one job per project, so # per-project pass/fail shows in the checks list; a full run batches # them, and each batch job prints a group and a summary row per - # member. The `prek` workflow still exercises pytest as a hook, so - # this workflow is the visible signal — not the gate. + # member. The required `tests-ok` job below aggregates discovery + # and matrix results; the `prek` workflow skips workspace pytest. pytest: name: "pytest (${{ matrix.batch.name }})" runs-on: ubuntu-slim needs: members # Guard against an empty selection. When the path filter selects no - # members (a PR that changed no project's Python), the batches output - # is `[]`; feeding an empty array to `matrix` makes GitHub mark this - # job `failure` ("matrix vector does not contain any values"), not + # members, the batches output is `[]`. An empty `matrix` makes GitHub + # mark this job `failure` ("matrix vector does not contain any values"), not # `skipped` — which would fail `tests-ok`. Skipping the job on an # empty list yields the `skipped` result `tests-ok` treats as a pass. if: ${{ needs.members.outputs.batches != '[]' }} @@ -204,28 +205,37 @@ jobs: # with N entries would otherwise force N `.asf.yaml` lines that # must be kept in lock-step with the matrix. This single # `tests-ok` job is the only pytest-related context required by - # branch protection — it passes iff every entry in the `pytest` - # matrix above passed. Adding, renaming, or removing matrix - # entries no longer touches `.asf.yaml`. + # branch protection — it requires successful member discovery and + # a passing matrix (or a confirmed empty selection). Matrix changes + # no longer require `.asf.yaml` updates. tests-ok: name: tests-ok runs-on: ubuntu-slim - needs: pytest + needs: [members, pytest] # `always()` — without it, this job would be skipped (not # failed) when an upstream matrix entry fails, and a skipped - # required-check is treated as missing by branch protection, - # which leaves the PR mergeable. Run unconditionally and assert - # on the result instead. + # required check does not block merging. Run unconditionally and + # assert on discovery as well: its failure skips the matrix too. if: always() steps: - - name: Assert all pytest matrix entries succeeded + - name: Assert member discovery and pytest matrix results + env: + BATCHES: ${{ needs.members.outputs.batches }} # `success` — every selected member passed. - # `skipped` — the path filter selected no members (the PR - # changed no project's Python source/test files), so there - # was nothing to run; that is a pass, not a gap. + # `skipped` — accepted only after successful discovery with an + # explicitly empty batches output, confirming nothing was selected. # Anything else (`failure` / `cancelled`) fails the gate. run: | + members_result="${{ needs.members.result }}" + if [ "${members_result}" != "success" ]; then + echo "workspace member discovery result: ${members_result}" + exit 1 + fi result="${{ needs.pytest.result }}" + if [ "${result}" = "skipped" ] && [ "${BATCHES:-}" != "[]" ]; then + echo "pytest matrix skipped without an empty selection" + exit 1 + fi if [ "${result}" != "success" ] && [ "${result}" != "skipped" ]; then echo "pytest matrix result: ${result}" exit 1 diff --git a/tools/dev/tests/test_ci_tests_gate.py b/tools/dev/tests/test_ci_tests_gate.py new file mode 100644 index 000000000..2860b437f --- /dev/null +++ b/tools/dev/tests/test_ci_tests_gate.py @@ -0,0 +1,254 @@ +# SPDX-License-Identifier: Apache-2.0 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Exercise the actual tests-ok shell gate, including discovery failures. + +The field reader supports literal needs lists (inline or block), scalar +conditions, quotes, and comments. It is not a general YAML parser: aliases, +tags, and folded scalars are outside this workflow test's supported syntax. +""" + +from __future__ import annotations + +import os +import re +import shlex +import shutil +import subprocess +from pathlib import Path +from textwrap import dedent + +import pytest + +_WORKFLOW = Path(__file__).resolve().parents[3] / ".github/workflows/tests.yml" +_RESULTS = ("success", "failure", "cancelled", "skipped") +_BASH_FLAGS = ("--noprofile", "--norc", "-e", "-o", "pipefail") + + +def _field(job: str, name: str, indent: int = 4) -> str: + lines = job.splitlines() + for index, line in enumerate(lines): + match = re.fullmatch(rf" {{{indent}}}{re.escape(name)}:[ \t]*(.*)", line) + if match is None: + continue + value = [match[1]] + for following in lines[index + 1 :]: + stripped = following.lstrip() + if not stripped or stripped.startswith("#"): + continue + depth = len(following) - len(stripped) + if depth < indent or (depth == indent and not stripped.startswith("- ")): + break + value.append(stripped) + return "\n".join(value) + raise AssertionError(f"tests-ok is missing {name}") + + +def _tokens(value: str, sequence: bool = False) -> list[str]: + lexer = shlex.shlex(value, posix=True) + lexer.whitespace_split = True + if sequence: + lexer.whitespace += ",[]" + return list(lexer) + + +def _expression(value: str, require_wrapper: bool = False) -> str: + value = " ".join(_tokens(value)).strip() + wrapped = value.startswith("${{") and value.endswith("}}") + if require_wrapper: + assert wrapped, "the environment value must evaluate an Actions expression" + if wrapped: + value = value[3:-2].strip() + return value + + +def _assert_dependencies(job: str) -> None: + needs = [token for token in _tokens(_field(job, "needs"), sequence=True) if token != "-"] + assert set(needs) == {"members", "pytest"} + assert _expression(_field(job, "if")) == "always()" + + +def _working_bash() -> str: + executable = shutil.which("bash") + if executable is None: + pytest.skip("the workflow gate needs a working Bash (not found)") + try: + probe = subprocess.run( + [executable, *_BASH_FLAGS, "-c", '[ "${BATCHES:-}" = "[]" ]'], + env={**os.environ, "BATCHES": "[]"}, + capture_output=True, + text=True, + timeout=5, + check=False, + ) + except (OSError, subprocess.TimeoutExpired) as error: + pytest.skip(f"the workflow gate needs a working Bash ({type(error).__name__})") + else: + if probe.returncode != 0: + pytest.skip(f"the workflow gate needs a working Bash (probe exit {probe.returncode})") + return executable + + +@pytest.fixture +def gate_job() -> str: + # Read only this job's literal block; no YAML dependency is needed. + match = re.search(r"(?ms)^ tests-ok:\n(.*?)(?=^ \S|\Z)", _WORKFLOW.read_text(encoding="utf-8")) + assert match is not None, "tests-ok job is missing" + return match[1] + + +@pytest.fixture(scope="module") +def bash() -> str: + return _working_bash() + + +def test_gate_waits_for_both_jobs_and_runs_after_failure(gate_job: str) -> None: + _assert_dependencies(gate_job) + + +def test_gate_reads_batches_from_discovery(gate_job: str) -> None: + value = _field(gate_job, "BATCHES", indent=10) + assert _expression(value, require_wrapper=True) == "needs.members.outputs.batches" + + +def _run_gate( + gate_job: str, bash: str, members_result: str, pytest_result: str, batches: str | None = "[]" +) -> subprocess.CompletedProcess[str]: + run = re.search(r"(?m)^ run: \|\n((?: .*\n|\n)+)", gate_job) + assert run is not None, "tests-ok shell assertion is missing" + script = dedent(run[1]) + for job, result in (("members", members_result), ("pytest", pytest_result)): + script = script.replace("${{ needs." + job + ".result }}", result) + assert "${{" not in script, "unresolved Actions expression in the gate" + env = os.environ.copy() + env.pop("BATCHES", None) + if batches is not None: + env["BATCHES"] = batches + return subprocess.run( + [bash, *_BASH_FLAGS, "-c", script], + env=env, + capture_output=True, + text=True, + timeout=20, + check=False, + ) + + +def _assert_result(completed: subprocess.CompletedProcess[str], code: int, diagnostic: str) -> None: + details = f"exit={completed.returncode}\n{completed.stdout}{completed.stderr}" + assert completed.returncode == code, details + assert diagnostic in completed.stdout, details + + +@pytest.mark.parametrize("members_result", _RESULTS) +@pytest.mark.parametrize("pytest_result", _RESULTS) +def test_gate_results(gate_job: str, bash: str, members_result: str, pytest_result: str) -> None: + completed = _run_gate(gate_job, bash, members_result, pytest_result) + if members_result != "success": + _assert_result(completed, 1, f"workspace member discovery result: {members_result}") + elif pytest_result not in ("success", "skipped"): + _assert_result(completed, 1, f"pytest matrix result: {pytest_result}") + else: + _assert_result(completed, 0, f"pytest gate satisfied (matrix result: {pytest_result})") + + +@pytest.mark.parametrize( + "batches", + [None, "", "[ ]", '[{"name":"dev"}]', "not-json", "$(exit 23)", "`exit 24`"], + ids=["missing", "empty", "noncanonical-empty", "nonempty", "malformed", "substitution", "backticks"], +) +def test_skipped_matrix_needs_a_confirmed_empty_selection( + gate_job: str, bash: str, batches: str | None +) -> None: + completed = _run_gate(gate_job, bash, "success", "skipped", batches) + _assert_result(completed, 1, "pytest matrix skipped without an empty selection") + + +@pytest.mark.parametrize("members_result", ["failure", "cancelled", "skipped"]) +def test_discovery_failure_is_reported_before_invalid_skip( + gate_job: str, bash: str, members_result: str +) -> None: + completed = _run_gate(gate_job, bash, members_result, "skipped", '[{"name":"dev"}]') + _assert_result(completed, 1, f"workspace member discovery result: {members_result}") + + +def test_successful_matrix_accepts_a_nonempty_selection(gate_job: str, bash: str) -> None: + completed = _run_gate(gate_job, bash, "success", "success", '[{"name":"dev"}]') + _assert_result(completed, 0, "pytest gate satisfied (matrix result: success)") + + +@pytest.mark.parametrize( + ("needs", "condition"), + [ + ("[members, pytest]", "always()"), + ("[members, pytest] # comment", "always()"), + ("['members', 'pytest']", "always()"), + ('["members", "pytest"]', "always()"), + ("\n - members\n - pytest", "always()"), + ("\n - 'members'\n - 'pytest' # comment", "${{ always() }}"), + ("[members, pytest]", "${{ always() }}"), + ("[members, pytest]", "'${{ always() }}' # comment"), + ], +) +def test_equivalent_dependency_declarations(needs: str, condition: str) -> None: + _assert_dependencies(f" needs: {needs}\n if: {condition}\n") + + +@pytest.mark.parametrize( + ("needs", "condition"), + [ + ("[pytest]", "always()"), + ("[members]", "always()"), + ("[members, pytest]", "success()"), + ("[members, pytest]", "al ways()"), + ("[members, pytest, other]", "always()"), + ], +) +def test_incorrect_dependency_declarations_are_rejected(needs: str, condition: str) -> None: + with pytest.raises(AssertionError): + _assert_dependencies(f" needs: {needs}\n if: {condition}\n") + + +@pytest.mark.parametrize("mode", ["missing", "launch-error", "timeout", "nonzero"]) +def test_unusable_bash_is_skipped(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.setattr(shutil, "which", lambda name: None if mode == "missing" else "/probe/bash") + + def probe(*args, **kwargs): + if mode == "launch-error": + raise OSError("cannot launch Bash") + if mode == "timeout": + raise subprocess.TimeoutExpired("bash", 5) + return subprocess.CompletedProcess(args[0], 1, "", "WSL launcher unavailable") + + monkeypatch.setattr(subprocess, "run", probe) + with pytest.raises(pytest.skip.Exception, match="needs a working Bash"): + _working_bash() + + +def test_bash_probe_skips_shell_without_gate_environment(bash: str, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(shutil, "which", lambda name: bash) + run = subprocess.run + + def without_batches(*args, **kwargs): + env = kwargs.get("env", os.environ).copy() + env.pop("BATCHES", None) + return run(*args, **{**kwargs, "env": env}) + + monkeypatch.setattr(subprocess, "run", without_batches) + with pytest.raises(pytest.skip.Exception, match="needs a working Bash"): + _working_bash() From bb203c9d5d5d83ebf6bdcd1c99b46b8c9f3764bc Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sun, 25 Oct 2026 16:15:16 +0100 Subject: [PATCH 2/2] fix(ci): fail the tests-ok gate tests on CI when Bash is unusable A broken Bash on the runner used to skip every gate test, leaving the required check green with no coverage; it now fails when CI is set and still skips locally. The assert step sets shell: bash so the runner uses the flags the test runs it with, the script under test is found by the step's name, and the workflow is parsed once per module. Generated-by: Claude Code (Claude Opus 5) --- .github/workflows/tests.yml | 3 ++ tools/dev/tests/test_ci_tests_gate.py | 41 +++++++++++++++++++++++---- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index f07174dda..87a691925 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -219,6 +219,9 @@ jobs: if: always() steps: - name: Assert member discovery and pytest matrix results + # Explicit, so the runner uses the same flags the gate test runs it + # with (`--noprofile --norc -eo pipefail`), not the `bash -e` default. + shell: bash env: BATCHES: ${{ needs.members.outputs.batches }} # `success` — every selected member passed. diff --git a/tools/dev/tests/test_ci_tests_gate.py b/tools/dev/tests/test_ci_tests_gate.py index 2860b437f..ca927f740 100644 --- a/tools/dev/tests/test_ci_tests_gate.py +++ b/tools/dev/tests/test_ci_tests_gate.py @@ -32,6 +32,7 @@ import subprocess from pathlib import Path from textwrap import dedent +from typing import NoReturn import pytest @@ -83,10 +84,19 @@ def _assert_dependencies(job: str) -> None: assert _expression(_field(job, "if")) == "always()" +def _unusable_bash(reason: str) -> NoReturn: + # Skip only off CI: on the Linux runner a broken Bash must fail, or every + # gate test would quietly skip and the required check would stay green. + message = f"the workflow gate needs a working Bash ({reason})" + if os.environ.get("CI"): + pytest.fail(message) + pytest.skip(message) + + def _working_bash() -> str: executable = shutil.which("bash") if executable is None: - pytest.skip("the workflow gate needs a working Bash (not found)") + _unusable_bash("not found") try: probe = subprocess.run( [executable, *_BASH_FLAGS, "-c", '[ "${BATCHES:-}" = "[]" ]'], @@ -97,14 +107,14 @@ def _working_bash() -> str: check=False, ) except (OSError, subprocess.TimeoutExpired) as error: - pytest.skip(f"the workflow gate needs a working Bash ({type(error).__name__})") + _unusable_bash(type(error).__name__) else: if probe.returncode != 0: - pytest.skip(f"the workflow gate needs a working Bash (probe exit {probe.returncode})") + _unusable_bash(f"probe exit {probe.returncode}") return executable -@pytest.fixture +@pytest.fixture(scope="module") def gate_job() -> str: # Read only this job's literal block; no YAML dependency is needed. match = re.search(r"(?ms)^ tests-ok:\n(.*?)(?=^ \S|\Z)", _WORKFLOW.read_text(encoding="utf-8")) @@ -129,7 +139,12 @@ def test_gate_reads_batches_from_discovery(gate_job: str) -> None: def _run_gate( gate_job: str, bash: str, members_result: str, pytest_result: str, batches: str | None = "[]" ) -> subprocess.CompletedProcess[str]: - run = re.search(r"(?m)^ run: \|\n((?: .*\n|\n)+)", gate_job) + # The assert step's own `run: |` block, found by its name, so a step added + # before it can never be the script under test. + run = re.search( + r"(?ms)^ - name: Assert member discovery and pytest matrix results\n.*?^ run: \|\n((?: .*\n|\n)+)", + gate_job, + ) assert run is not None, "tests-ok shell assertion is missing" script = dedent(run[1]) for job, result in (("members", members_result), ("pytest", pytest_result)): @@ -226,6 +241,7 @@ def test_incorrect_dependency_declarations_are_rejected(needs: str, condition: s @pytest.mark.parametrize("mode", ["missing", "launch-error", "timeout", "nonzero"]) def test_unusable_bash_is_skipped(monkeypatch: pytest.MonkeyPatch, mode: str) -> None: + monkeypatch.delenv("CI", raising=False) monkeypatch.setattr(shutil, "which", lambda name: None if mode == "missing" else "/probe/bash") def probe(*args, **kwargs): @@ -241,6 +257,7 @@ def probe(*args, **kwargs): def test_bash_probe_skips_shell_without_gate_environment(bash: str, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("CI", raising=False) monkeypatch.setattr(shutil, "which", lambda name: bash) run = subprocess.run @@ -252,3 +269,17 @@ def without_batches(*args, **kwargs): monkeypatch.setattr(subprocess, "run", without_batches) with pytest.raises(pytest.skip.Exception, match="needs a working Bash"): _working_bash() + + +def test_unusable_bash_fails_on_ci(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("CI", "true") + monkeypatch.setattr(shutil, "which", lambda name: None) + with pytest.raises(pytest.fail.Exception, match="needs a working Bash"): + _working_bash() + + +def test_gate_step_runs_with_the_tested_shell_flags(gate_job: str) -> None: + step = gate_job.split("- name: Assert member discovery and pytest matrix results", 1)[1] + assert re.search(r"(?m)^ shell: bash\s*$", step), ( + "the gate step must set `shell: bash` to match _BASH_FLAGS" + )