diff --git a/changelog.d/core649-release-test-followups.changed.md b/changelog.d/core649-release-test-followups.changed.md new file mode 100644 index 000000000..88e52c082 --- /dev/null +++ b/changelog.d/core649-release-test-followups.changed.md @@ -0,0 +1 @@ +The release tests now also fail if the root `Makefile` mentions `.IGNORE`, `.ONESHELL`, `MAKEFLAGS` (also inside a longer name such as `GNUMAKEFLAGS`) or `PYTEST_ADDOPTS`, or assigns `SHELL` or `.SHELLFLAGS` (with any assignment operator, also target-specific). They fail if it holds a conditional (`ifeq` … `endif`), a `define` block, `.RECIPEPREFIX`, a line that ends in `\` or a line break other than a newline (such as a form feed), which their recipe parser can't follow. They fail if a target on any of its rule lines, or a prerequisite that `make test` runs at any depth, lacks exactly one single-colon rule line that starts in the first column with no `;` recipe on it. They fail if a recipe line of `make test` or of a prerequisite it runs at any depth (order-only included) contains `||` or ends in `; true`, `; exit 0` or `; :`. The parser reads past comment-only lines inside a recipe, as make does. The tests also fail if the workflow, the Test job or the push workflow's test step sets `MAKEFLAGS` in `env`. They fail if any workflow job other than `push.yaml`'s Publish declares `environment: pypi` or gets `id-token: write` (from its own `permissions`, the workflow's, or `write-all`). A missing test step's failure message now says how to update the locator. diff --git a/tests/core/test_release_tagging.py b/tests/core/test_release_tagging.py index c8c3a1e5a..a5bffebce 100644 --- a/tests/core/test_release_tagging.py +++ b/tests/core/test_release_tagging.py @@ -405,22 +405,59 @@ def pypi_publishing_actions(actions: Path) -> list[str]: ] +# Make syntax that make_rules does not follow, so recipe lines after it could +# run unread: a conditional or define block, a changed recipe prefix, or a +# backslash at the end of a line. +MAKE_SYNTAX_MAKE_RULES_SKIPS = re.compile( + r"^[ \t\f\v]*((override|export)\s+)*(ifn?eq|ifn?def|else|endif|define|endef)\b" + r"|\.RECIPEPREFIX|\\$", + re.MULTILINE, +) + +# Python ends a line at \r, \v, \f, \x1c to \x1e, \x85, \u2028 and \u2029 +# as well as at \n: read_text turns a lone \r into \n, and splitlines splits +# at the rest. make ends a line only at \n. So make_rules would read a recipe +# line such as `cmd $(if ,
)|| true` as two lines and miss its +# `|| true` or upload. +MAKE_LINE_BREAK_MAKE_IGNORES = re.compile(r"\r(?!\n)|[\v\f\x1c-\x1e\x85\u2028\u2029]") + + def make_rules(makefile: Path) -> dict[str, tuple[list[str], list[str]]]: """Each target's prerequisites and recipe lines (without the tab). - Enough for this Makefile: it has no line continuations, includes or - pattern rules. + Reads rule lines that start in the first column with a single colon, and + the tab-indented lines after each one as its recipe. Like make, it reads + past blank lines and lines of just a comment among recipe lines; any + other line ends the recipe. It fails on a conditional, a define block, + .RECIPEPREFIX, a line that ends in a backslash or a line break other than + a newline, since recipe lines after them could run unread. It also fails + on a recipe line that opens with a directive word, such as `else`, which + it can't tell from a directive. It misreads a double-colon, indented or + repeated rule line and a `;` recipe on the rule line, which + make_targets_make_rules_misreads reports, and it doesn't follow include + or pattern rules. """ + assert not MAKE_LINE_BREAK_MAKE_IGNORES.search(makefile.read_bytes().decode()), ( + "make_rules cannot read a line break other than \\n or \\r\\n in the Makefile" + ) + text = makefile.read_text() + assert not MAKE_SYNTAX_MAKE_RULES_SKIPS.search(text), ( + "make_rules cannot read a conditional, define block, .RECIPEPREFIX, " + "line continuation or recipe line that opens with a directive word in " + "the Makefile" + ) rules = {} recipe = [] - for line in makefile.read_text().splitlines(): + for line in text.splitlines(): if line.startswith("\t"): recipe.append(line[1:]) elif rule := MAKE_RULE.match(line): recipe = [] for target in rule.group(1).split(): rules[target] = (rule.group(2).split(), recipe) - elif line.strip(): + # make ignores blank lines and lines of just a comment among recipe + # lines, so neither ends a recipe. + elif line.strip() and not line.lstrip().startswith("#"): recipe = [] return rules @@ -437,21 +474,82 @@ def pypi_uploading_recipe_lines(makefile: Path) -> list[str]: def test_only_push_publish_job_uploads_to_pypi(): # The checks above cover push.yaml's Publish job alone, so an upload in # any other job or workflow file would reach PyPI without them. - # Recognises the pypa publish action, twine upload and - # uv/poetry/flit/hatch/pdm/maturin publish in workflow steps, local - # composite action steps and Makefile recipes, and rejects reusable - # workflows from other repositories; not uploads inside scripts or remote - # actions. + # Recognises the pypa publish action (lower-case owner), and twine upload + # and uv/poetry/flit/hatch/pdm/maturin publish written as adjacent words, + # in workflow steps, composite action steps under .github/actions and root + # Makefile recipes (each target must have one rule line that make_rules + # reads), and rejects reusable workflows from other repositories. + # It misses uploads inside scripts and remote, Docker or JavaScript + # actions; test_only_publish_job_gets_pypi_credentials limits those that + # rely on trusted publishing. assert pypi_publishing_jobs(WORKFLOWS) == [("push.yaml", "Publish")] assert remote_reusable_workflow_jobs(WORKFLOWS) == [], ( "a job calls a reusable workflow from another repository" ) assert pypi_publishing_actions(ACTIONS) == [], "a composite action uploads to PyPI" + assert make_targets_make_rules_misreads(MAKEFILE) == [], ( + "a Makefile target needs exactly one single-colon rule line" + ) assert pypi_uploading_recipe_lines(MAKEFILE) == [], ( "a Makefile recipe uploads to PyPI" ) +def pypi_environment(job: dict) -> bool: + # environment: is a name, or a mapping with a name and a url. + environment = job.get("environment") + if isinstance(environment, dict): + environment = environment.get("name") + return str(environment).lower() == "pypi" + + +def grants_id_token(permissions) -> bool: + return permissions == "write-all" or ( + isinstance(permissions, dict) and permissions.get("id-token") == "write" + ) + + +def pypi_credential_jobs(workflows: Path) -> list[tuple[str, str, list[str]]]: + """Each job that declares the pypi environment or an id-token: write grant. + + A job without permissions of its own is checked against the workflow's. + The environment name is compared ignoring case. + """ + return [ + (file_name, name, grants) + for file_name, workflow in load_workflows(workflows) + for name, job in (workflow.get("jobs") or {}).items() + if ( + grants := [ + grant + for grant, declared in [ + ("environment: pypi", pypi_environment(job)), + ( + "id-token: write", + grants_id_token( + job.get("permissions", workflow.get("permissions")) + ), + ), + ] + if declared + ] + ) + ] + + +def test_only_publish_job_gets_pypi_credentials(): + # Per the comment on push.yaml's Publish job, PyPI trusted publishing + # accepts uploads only from push.yaml in the pypi environment, and Publish + # declares that environment and id-token: write for it. The scan above + # cannot read inside a remote, Docker or JavaScript action, so this keeps + # both declarations off every other job. It rests on PyPI's + # trusted-publisher settings, which these tests cannot read, and does not + # cover an upload that uses a stored API token. + assert pypi_credential_jobs(WORKFLOWS) == [ + ("push.yaml", "Publish", ["environment: pypi", "id-token: write"]) + ] + + def copy_workflows_with_pr_test_job_change(tmp_path: Path, change) -> None: shutil.copytree(WORKFLOWS, tmp_path, dirs_exist_ok=True) pr_workflow = tmp_path / "pr.yaml" @@ -498,6 +596,62 @@ def call_reusable_workflows(jobs: dict) -> None: assert remote_reusable_workflow_jobs(tmp_path) == [("pr.yaml", "Remote")] +@pytest.mark.parametrize( + ("change", "grants"), + [ + ({"environment": "pypi"}, ["environment: pypi"]), + ( + {"environment": {"name": "PyPI", "url": "https://pypi.org"}}, + ["environment: pypi"], + ), + ({"permissions": {"id-token": "write"}}, ["id-token: write"]), + ({"permissions": "write-all"}, ["id-token: write"]), + ( + {"environment": "pypi", "permissions": {"id-token": "write"}}, + ["environment: pypi", "id-token: write"], + ), + ], + ids=[ + "environment", + "environment-mapping", + "id-token", + "write-all", + "both", + ], +) +def test_pypi_credential_check_finds_another_job(tmp_path, change, grants): + copy_workflows_with_pr_test_job_change( + tmp_path, lambda jobs: jobs["Test"].update(change) + ) + + assert pypi_credential_jobs(tmp_path) == [ + ("pr.yaml", "Test", grants), + ("push.yaml", "Publish", ["environment: pypi", "id-token: write"]), + ] + + +def test_pypi_credential_check_applies_workflow_permissions(tmp_path): + shutil.copytree(WORKFLOWS, tmp_path, dirs_exist_ok=True) + workflow = { + "on": "push", + "permissions": {"id-token": "write"}, + "jobs": { + "Inherits": {"runs-on": "ubuntu-latest", "steps": [{"run": "true"}]}, + "Overrides": { + "runs-on": "ubuntu-latest", + "permissions": {"contents": "read"}, + "steps": [{"run": "true"}], + }, + }, + } + (tmp_path / "other.yaml").write_text(yaml.safe_dump(workflow)) + + assert pypi_credential_jobs(tmp_path) == [ + ("other.yaml", "Inherits", ["id-token: write"]), + ("push.yaml", "Publish", ["environment: pypi", "id-token: write"]), + ] + + @pytest.mark.parametrize("file_name", ["action.yml", "action.yaml"]) def test_pypi_upload_check_finds_upload_in_composite_action(tmp_path, file_name): action = tmp_path / "publish" / file_name @@ -544,7 +698,8 @@ def assert_test_job_fails_when_tests_fail(workflow: dict) -> None: only_step( steps, lambda step: step.get("run", "").split()[:2] == ["make", "test"], - "test", + "test (a step whose run starts with the words `make test`; if the " + "command moved, update this locator and the exact match below)", ) ] # A skipped step does not fail the job, so a condition on the step that @@ -573,25 +728,93 @@ def assert_test_job_fails_when_tests_fail(workflow: dict) -> None: "PYTEST_ADDOPTS" in (item.get("env") or {}) for item in [workflow, test_job, test_step] ), "Test sets PYTEST_ADDOPTS" + # make takes options from MAKEFLAGS, so MAKEFLAGS=-i lets make test pass + # after a recipe line fails, as `make -i test` would. + assert not any( + "MAKEFLAGS" in (item.get("env") or {}) + for item in [workflow, test_job, test_step] + ), "Test sets MAKEFLAGS" + + +# A setting that lets make test pass after a recipe line fails: any mention of +# .IGNORE or .ONESHELL (with .ONESHELL only the last line's status counts), any +# mention of MAKEFLAGS, including GNUMAKEFLAGS (=, +=, override, export, +# $(eval ...)), or a SHELL or .SHELLFLAGS of its own, also target-specific +# (`test: SHELL = /bin/true`). +# Any mention of PYTEST_ADDOPTS fails it too. make passes a variable named +# in an `export` line to the commands it runs, and pytest adds the options +# in PYTEST_ADDOPTS to its command line, so `export PYTEST_ADDOPTS = +# --collect-only` lets make test pass without running tests. +# Reading MAKEFLAGS, as `$(MAKEFLAGS)` does, also fails this check: a loud +# false positive, and the Makefile has no such line. +MAKE_SETTING_THAT_MASKS = re.compile( + r"\.(IGNORE|ONESHELL)\b|MAKEFLAGS|PYTEST_ADDOPTS|(? list[str]: + """Targets whose recipe make_rules may not read as make does. + + make_rules keeps only a target's last rule line, skips double-colon rules + and rule lines indented with spaces, and reads a recipe written after `;` + on the rule line as prerequisites. So each target needs exactly one rule + line, single-colon, starting in the first column, with no `;`. + """ + lines = {} + for line in makefile.read_text().splitlines(): + if rule := ANY_MAKE_RULE.match(line): + for target in rule.group(1).split(): + lines.setdefault(target, []).append(line) + return sorted( + target + for target, rule_lines in lines.items() + if len(rule_lines) != 1 + or not (rule := MAKE_RULE.match(rule_lines[0])) + or ";" in rule.group(2) + ) -def assert_make_test_fails_when_tests_fail( - rules: dict[str, tuple[list[str], list[str]]], -) -> None: - prerequisites, _ = rules["test"] - # make ignores a failing recipe line whose prefix (any mix of @, + and - - # after the tab) includes -, so make test could pass after tests fail. - for target in ["test", *prerequisites]: - for line in rules.get(target, ([], []))[1]: +def assert_make_test_fails_when_tests_fail(makefile: Path) -> None: + assert not MAKE_SETTING_THAT_MASKS.search(makefile.read_text()), ( + "the Makefile can ignore recipe failures" + ) + rules = make_rules(makefile) + misread = make_targets_make_rules_misreads(makefile) + # make runs test's prerequisites at every depth, and their recipes. + checked, queue = [], ["test"] + while queue: + target = queue.pop() + if target in checked: + continue + checked.append(target) + assert target in rules and target not in misread, ( + f"make {target} needs exactly one single-colon rule line" + ) + prerequisites, recipe = rules[target] + # `|` separates order-only prerequisites, which make also runs. + queue += [name for name in prerequisites if name != "|"] + for line in recipe: + # make ignores a failing recipe line whose prefix (any mix of @, + + # and - after the tab) includes -. prefix = line[: len(line) - len(line.lstrip("@+- \t"))] assert "-" not in prefix, f"make {target} ignores a failing recipe line" + assert not MASKED_RECIPE_LINE.search(line), ( + f"make {target} masks a failing command: {line}" + ) def test_test_job_cannot_pass_with_failing_tests(): workflow = yaml.safe_load(PUSH_WORKFLOW.read_text()) assert_test_job_fails_when_tests_fail(workflow) - assert_make_test_fails_when_tests_fail(make_rules(MAKEFILE)) + assert_make_test_fails_when_tests_fail(MAKEFILE) def find_test_step(workflow: dict) -> dict: @@ -642,6 +865,18 @@ def collect_tests_only_in_workflow(workflow: dict) -> None: workflow["env"] = {"PYTEST_ADDOPTS": "--collect-only"} +def ignore_make_errors_in_test_step(workflow: dict) -> None: + find_test_step(workflow)["env"] = {"MAKEFLAGS": "-i"} + + +def ignore_make_errors_in_test_job(workflow: dict) -> None: + workflow["jobs"]["Test"]["env"] = {"MAKEFLAGS": "-i"} + + +def ignore_make_errors_in_workflow(workflow: dict) -> None: + workflow["env"] = {"MAKEFLAGS": "-i"} + + @pytest.mark.parametrize( ("mutate", "violation"), [ @@ -657,6 +892,9 @@ def collect_tests_only_in_workflow(workflow: dict) -> None: (collect_tests_only_in_test_step, "Test sets PYTEST_ADDOPTS"), (collect_tests_only_in_test_job, "Test sets PYTEST_ADDOPTS"), (collect_tests_only_in_workflow, "Test sets PYTEST_ADDOPTS"), + (ignore_make_errors_in_test_step, "Test sets MAKEFLAGS"), + (ignore_make_errors_in_test_job, "Test sets MAKEFLAGS"), + (ignore_make_errors_in_workflow, "Test sets MAKEFLAGS"), ] ], ) @@ -695,4 +933,239 @@ def test_make_test_check_rejects_ignored_recipe_failure(tmp_path, target, prefix with pytest.raises( AssertionError, match=f"make {target} ignores a failing recipe line" ): - assert_make_test_fails_when_tests_fail(make_rules(makefile)) + assert_make_test_fails_when_tests_fail(makefile) + + +@pytest.mark.parametrize( + "run", + ["make -C . test", "uv run make test"], +) +def test_test_job_check_explains_missing_test_step(run): + workflow = yaml.safe_load(PUSH_WORKFLOW.read_text()) + find_test_step(workflow)["run"] = run + + with pytest.raises(AssertionError) as failure: + assert_test_job_fails_when_tests_fail(workflow) + + message = str(failure.value).splitlines()[0] + assert message.startswith("expected one test (a step whose run starts with") + assert "update this locator and the exact match below" in message + assert message.endswith("found []") + + +def write_makefile(tmp_path: Path, text: str) -> Path: + makefile = tmp_path / "Makefile" + makefile.write_text(text) + return makefile + + +@pytest.mark.parametrize( + "setting", + [ + ".IGNORE: test", + ".IGNORE:", + " .IGNORE:", + ".IGNORE: # c", + ".IGNORE:test", + "foo .IGNORE:", + "$(eval .IGNORE:)", + ".ONESHELL:", + "MAKEFLAGS += -i", + "export MAKEFLAGS = -i", + " MAKEFLAGS := i", + "override MAKEFLAGS += -i", + "$(eval MAKEFLAGS += -i)", + "GNUMAKEFLAGS += -i", + "SHELL := /bin/true", + "override SHELL = /bin/true", + ".SHELLFLAGS := -c true", + "test: SHELL = /bin/true", + "export PYTEST_ADDOPTS = --collect-only", + "test: export PYTEST_ADDOPTS = --collect-only", + ], +) +def test_make_test_check_rejects_setting_that_masks(tmp_path, setting): + makefile = write_makefile(tmp_path, f"{setting}\n" + MAKEFILE.read_text()) + + with pytest.raises(AssertionError, match="the Makefile can ignore recipe failures"): + assert_make_test_fails_when_tests_fail(makefile) + + +def edit_makefile(tmp_path: Path, old: str, new: str) -> Path: + text = MAKEFILE.read_text() + assert text.count(old) == 1 + return write_makefile(tmp_path, text.replace(old, new)) + + +TEMPLATE_RULE = "test-country-template:\n" + + +@pytest.mark.parametrize( + ("old", "new", "violation"), + [ + pytest.param( + TEMPLATE_RULE, + "test-country-template: deeper\n", + "make deeper needs exactly one single-colon rule line", + id="prerequisite-without-rule", + ), + pytest.param( + TEMPLATE_RULE, + "deeper:\n\tfalse || true\n\ntest-country-template: deeper\n", + "make deeper masks a failing command", + id="prerequisite-of-prerequisite", + ), + pytest.param( + "test: test-country-template\n", + "test: test-country-template | deeper\n", + "make deeper needs exactly one single-colon rule line", + id="order-only-prerequisite", + ), + pytest.param( + "build:\n", + "test: format\n\nbuild:\n", + "make test needs exactly one single-colon rule line", + id="later-rule-line", + ), + pytest.param( + "build:\n", + "format test:\n\nbuild:\n", + "make test needs exactly one single-colon rule line", + id="later-rule-line-with-other-target", + ), + pytest.param( + "build:\n", + " test:\n\ttrue\n\nbuild:\n", + "make test needs exactly one single-colon rule line", + id="later-indented-rule-line", + ), + pytest.param( + TEMPLATE_RULE, + "test-country-template::\n", + "make test-country-template needs exactly one single-colon rule line", + id="double-colon-rule", + ), + pytest.param( + TEMPLATE_RULE, + "test-country-template: ; false || true\n", + "make test-country-template needs exactly one single-colon rule line", + id="recipe-on-rule-line", + ), + pytest.param( + "\tcoverage xml -i\n", + "# write the report\n\tcoverage xml -i || true\n", + "make test masks a failing command", + id="comment-line-before-masked-line", + ), + ], +) +def test_make_test_check_rejects_recipe_it_cannot_read(tmp_path, old, new, violation): + makefile = edit_makefile(tmp_path, old, new) + + with pytest.raises(AssertionError, match=violation): + assert_make_test_fails_when_tests_fail(makefile) + + +@pytest.mark.parametrize( + ("old", "new"), + [ + pytest.param( + "\tcoverage xml -i\n", + "ifeq (1,1)\n\tcoverage xml -i || true\nendif\n", + id="conditional", + ), + pytest.param( + "all: install", ".RECIPEPREFIX = >\nall: install", id="recipeprefix" + ), + pytest.param( + "all: install", + "define SHELL\n/bin/true\nendef\n\nall: install", + id="define", + ), + pytest.param( + "\tcoverage xml -i\n", + "\tcoverage xml -i; \\\n\ttrue\n", + id="line-continuation", + ), + pytest.param( + "\tcoverage xml -i\n", + " \tifeq (1,1)\n\tcoverage xml -i || true\n \tendif\n", + id="conditional-after-space-and-tab", + ), + pytest.param( + "all: install", + "\fdefine SHELL\n/bin/true\n\fendef\n\nall: install", + id="define-after-form-feed", + ), + pytest.param( + "all: install", + "X = 1\n\tifeq (1,0)\n\tendif\n\nall: install", + id="tab-conditional-outside-rule", + ), + pytest.param( + "\tcoverage xml -i\n", + "\tcoverage xml -i $(if ,\f)|| true\n", + id="form-feed-inside-recipe-line", + ), + pytest.param( + "\tcoverage xml -i\n", + "\tcoverage xml -i $(if ,\r)|| true\n", + id="carriage-return-inside-recipe-line", + ), + ], +) +def test_make_rules_rejects_syntax_it_cannot_follow(tmp_path, old, new): + makefile = edit_makefile(tmp_path, old, new) + + with pytest.raises(AssertionError, match="make_rules cannot read"): + make_rules(makefile) + + +def test_pypi_upload_check_reads_past_comment_line(tmp_path): + makefile = edit_makefile( + tmp_path, + "build:\n\tpython -m build\n", + "build:\n\tpython -m build\n# upload\n\ttwine upload dist/*\n", + ) + + assert pypi_uploading_recipe_lines(makefile) == ["build: twine upload dist/*"] + + +@pytest.mark.parametrize( + "new", + [ + pytest.param("build::\n\ttwine upload dist/*\n", id="double-colon-rule"), + pytest.param( + "build:\n\ttwine upload dist/*\n\nbuild: install\n", + id="later-rule-line", + ), + pytest.param("build: ; twine upload dist/*\n", id="recipe-on-rule-line"), + ], +) +def test_pypi_upload_check_rejects_recipe_it_cannot_read(tmp_path, new): + makefile = edit_makefile(tmp_path, "build:\n\tpython -m build\n", new) + + assert make_targets_make_rules_misreads(makefile) == ["build"] + + +@pytest.mark.parametrize( + ("target", "suffix"), + [ + ("test", " || true"), + ("test", " || exit 0"), + ("test", "; true"), + ("test", "; exit 0"), + ("test", " ; :"), + ("test-country-template", " || true"), + ], +) +def test_make_test_check_rejects_masked_recipe_line(tmp_path, target, suffix): + lines = MAKEFILE.read_text().splitlines() + recipe_start = next( + index for index, line in enumerate(lines) if line.startswith(f"{target}:") + ) + lines[recipe_start + 1] += suffix + makefile = write_makefile(tmp_path, "\n".join(lines) + "\n") + + with pytest.raises(AssertionError, match=f"make {target} masks a failing command"): + assert_make_test_fails_when_tests_fail(makefile)