fix(release): notes must not open with the bot commit; guard matches the subject (t44 3b/3c)
t45's review of the release versioning found three lows; two touch behaviour here (the
third, README wording, is the next commit).
R1 — with a write-back the tag points at the bot's own commit, so the notes range
`PREV..TAG` published "- chore(release): vX [skip ci]" as the first line of every
release. The range now ends at `${TAG}^` when BUMP=yes (the last commit a developer
wrote) and at `${TAG}` when it is not (there the tag IS the pushed commit, and it
belongs in the notes). `git describe` for the previous tag is unchanged: it already
started from `${TAG}^`, so it keeps naming the previous release.
R3 — the job guard matched the `[skip ci]` marker ANYWHERE in the commit message, so a
developer commit whose body merely mentioned it silently skipped its own release. The
guard is now anchored to the release commit's subject prefix
(`!startsWith(message, 'chore(release):')`), which is a structural property, not a
substring a body can trip. The marker stays in the commit message: it is the
convention Gitea itself may honour to skip the run outright, and the README now says
so, together with the fact that the job guard is the layer that holds when it does not.
3c — the resolver's tests used tag fixtures, so `refs/tags/X^{commit}` was never
exercised, and that dereference is the entire reason an annotated tag is recognised
(for one, `git rev-parse refs/tags/X` is the TAG OBJECT, not the commit, so a
comparison that forgot to dereference would miss a released commit). The resolver now
also runs inside real temporary repositories built with real annotated and lightweight
tags: released-on-HEAD both ways, a tag on an older commit falling back to the date,
no tags at all, the same-day scan walking real tags, and gap reuse. The workflow's own
run: scripts are executed too — extracted from the YAML by hand rather than with a
parser the fast suite does not have — covering the bump/verify/tag path (commit
subject, tag == HEAD, the tag's manifest, the pushed tag), the inconsistent-manifest
refusal, the rejected-push failure, and the notes range: with BUMP=yes the fixed range
yields "- feat: the real change" while the range it replaced demonstrably contains
"- chore(release):". The job-level `if:` itself cannot be executed here (no Gitea
runner) and stays a contract pin; the script-side layer that makes a bypassed guard
harmless is executed instead.
Verified: 414 passed / 2 skipped, ruff clean, release.yml parses.
This commit is contained in:
James Mackay committed
1 parent
89c05f1dcd
commit
b03cfbae13
2 files changed
+322
-12
No files matched your search
@@ -333,6 +333,26 @@ def test_the_bot_commit_cannot_trigger_another_release(workflow_text):
|
||||
assert "workflow_dispatch" in workflow_text
|
||||
|
||||
|
||||
def test_the_skip_guard_matches_the_subject_not_the_whole_message(workflow_text):
|
||||
"""t44 R3: `contains(message, '[skip ci]')` skipped a release silently.
|
||||
|
||||
A developer commit whose BODY mentions the marker is not a release commit, so
|
||||
the guard is anchored to the release bot's subject prefix instead.
|
||||
"""
|
||||
assert "!startsWith(github.event.head_commit.message || '', 'chore(release):')" in (
|
||||
workflow_text
|
||||
)
|
||||
assert "!contains(github.event.head_commit.message" not in workflow_text
|
||||
|
||||
|
||||
def test_the_release_notes_exclude_the_write_back_commit(workflow_text):
|
||||
"""t44 R1: with a write-back the tag IS the bot commit, so notes must skip it."""
|
||||
assert 'if [ "$BUMP" = "yes" ]; then' in workflow_text
|
||||
assert 'NOTES_END="${TAG}^"' in workflow_text
|
||||
assert 'RANGE="${PREV}..${NOTES_END}"' in workflow_text
|
||||
assert 'BUMP: ${{ steps.version.outputs.BUMP }}' in workflow_text
|
||||
|
||||
|
||||
def test_the_preserved_release_behaviour_is_still_there(workflow_text):
|
||||
# one release at a time, never cancelled mid-flight
|
||||
assert "group: release-master" in workflow_text
|
||||
@@ -355,3 +375,280 @@ def test_the_script_reads_the_manifest_the_workflow_bumps():
|
||||
manifest_version = json.loads(MANIFEST.read_text(encoding="utf-8"))["version"]
|
||||
# the script rejects anything else, so a typo here would fail every release
|
||||
assert re.fullmatch(r"[0-9]{4}\.[0-9]{2}\.[0-9]{2}(\.[0-9]+)?", manifest_version)
|
||||
|
||||
|
||||
# ── t44 3c: real repositories, and the workflow's own steps by execution ─────
|
||||
|
||||
MANIFEST_REL = "custom_components/nvidia_shield/manifest.json"
|
||||
|
||||
|
||||
def _git(repo, *args) -> str:
|
||||
return subprocess.run(
|
||||
["git", *args], cwd=repo, capture_output=True, text=True, check=True
|
||||
).stdout
|
||||
|
||||
|
||||
def _repo(tmp_path, *, version="2026.10.09", commits=("a feature commit",)):
|
||||
"""A real repository: the resolver's tag lookups really are git commands."""
|
||||
repo = tmp_path / "repo"
|
||||
repo.mkdir(parents=True)
|
||||
_git(repo, "init", "-q", "-b", "master")
|
||||
_git(repo, "config", "user.email", "dev@example.com")
|
||||
_git(repo, "config", "user.name", "dev")
|
||||
manifest = repo / MANIFEST_REL
|
||||
manifest.parent.mkdir(parents=True, exist_ok=True)
|
||||
for index, subject in enumerate(commits):
|
||||
manifest.write_text(
|
||||
json.dumps(
|
||||
{"domain": "nvidia_shield", "version": version, "note": index}
|
||||
)
|
||||
+ "\n"
|
||||
)
|
||||
_git(repo, "add", "-A")
|
||||
_git(repo, "commit", "-qm", subject)
|
||||
return repo
|
||||
|
||||
|
||||
def _resolve_in(repo, *, today=TODAY):
|
||||
"""Run the resolver inside a real repo — no tag fixtures, the real git paths."""
|
||||
result = subprocess.run(
|
||||
["bash", str(SCRIPT), "--manifest", MANIFEST_REL, "--today", today],
|
||||
cwd=repo, capture_output=True, text=True, check=False,
|
||||
)
|
||||
values = dict(
|
||||
line.split("=", 1) for line in result.stdout.splitlines() if "=" in line
|
||||
)
|
||||
return result, values
|
||||
|
||||
|
||||
class TestRealRepositories:
|
||||
"""The tagged-version branches against real git (t44 3c).
|
||||
|
||||
The fixture-based tests above never exercise `refs/tags/X^{commit}`, and that
|
||||
dereference is the whole reason an annotated tag is recognised: for one,
|
||||
`git rev-parse refs/tags/X` is the TAG OBJECT, not the commit, so a comparison
|
||||
that forgot to dereference would miss a released commit entirely.
|
||||
"""
|
||||
|
||||
def test_an_annotated_tag_on_head_is_already_released(self, tmp_path):
|
||||
repo = _repo(tmp_path)
|
||||
_git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09")
|
||||
|
||||
result, values = _resolve_in(repo)
|
||||
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert values["SOURCE"] == "released"
|
||||
assert values["RELEASE"] == "no"
|
||||
assert "nothing to release" in result.stderr
|
||||
# premise: the tag object really is not the commit
|
||||
assert _git(repo, "rev-parse", "v2026.10.09").strip() != _git(
|
||||
repo, "rev-parse", "HEAD"
|
||||
).strip()
|
||||
|
||||
def test_a_lightweight_tag_on_head_is_also_already_released(self, tmp_path):
|
||||
repo = _repo(tmp_path)
|
||||
_git(repo, "tag", "v2026.10.09")
|
||||
|
||||
_result, values = _resolve_in(repo)
|
||||
|
||||
assert values["SOURCE"] == "released"
|
||||
assert values["RELEASE"] == "no"
|
||||
|
||||
def test_a_tag_on_an_older_commit_falls_back_to_the_date(self, tmp_path):
|
||||
repo = _repo(tmp_path, commits=("the released work", "later work"))
|
||||
_git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09", "HEAD~1")
|
||||
|
||||
_result, values = _resolve_in(repo)
|
||||
|
||||
assert values["VERSION"] == TODAY
|
||||
assert values["SOURCE"] == "date"
|
||||
assert values["BUMP"] == "yes"
|
||||
assert values["RELEASE"] == "yes"
|
||||
|
||||
def test_no_tags_at_all_means_the_manifest_wins(self, tmp_path):
|
||||
repo = _repo(tmp_path, version="2026.10.10")
|
||||
|
||||
_result, values = _resolve_in(repo)
|
||||
|
||||
assert values["VERSION"] == "2026.10.10"
|
||||
assert values["SOURCE"] == "manifest"
|
||||
assert values["BUMP"] == "no"
|
||||
|
||||
def test_the_same_day_scan_walks_real_tags(self, tmp_path):
|
||||
repo = _repo(tmp_path, commits=("released", "more"))
|
||||
for tag in ("v2026.10.09", f"v{TODAY}", f"v{TODAY}.2"):
|
||||
_git(repo, "tag", "-a", tag, "-m", f"Release {tag}", "HEAD~1")
|
||||
|
||||
_result, values = _resolve_in(repo)
|
||||
|
||||
assert values["VERSION"] == f"{TODAY}.3"
|
||||
assert values["SOURCE"] == "same-day"
|
||||
|
||||
def test_a_gap_in_the_suffixes_is_reused(self, tmp_path):
|
||||
repo = _repo(tmp_path, commits=("released", "more"))
|
||||
for tag in ("v2026.10.09", f"v{TODAY}", f"v{TODAY}.3"):
|
||||
_git(repo, "tag", "-a", tag, "-m", f"Release {tag}", "HEAD~1")
|
||||
|
||||
_result, values = _resolve_in(repo)
|
||||
|
||||
assert values["VERSION"] == f"{TODAY}.2"
|
||||
|
||||
def test_the_guard_has_a_script_side_layer(self, tmp_path):
|
||||
"""Even with the job guard bypassed, a released commit releases nothing.
|
||||
|
||||
The job-level `if:` cannot be executed here (no Gitea runner), but the
|
||||
decision it protects can: this is the layer that makes a bypass harmless.
|
||||
"""
|
||||
repo = _repo(tmp_path)
|
||||
_git(repo, "tag", "-a", "v2026.10.09", "-m", "Release v2026.10.09")
|
||||
# a re-run of the same push, guard or no guard
|
||||
for _attempt in range(2):
|
||||
_result, values = _resolve_in(repo)
|
||||
assert values["RELEASE"] == "no"
|
||||
assert values["BUMP"] == "no"
|
||||
|
||||
|
||||
def _step_script(name: str) -> str:
|
||||
"""The `run:` body of one workflow step, read by hand.
|
||||
|
||||
The fast suite has no YAML parser (the verify command parses the file with
|
||||
pyyaml instead), and executing these steps is worth more than matching their
|
||||
text, so the block is taken verbatim and dedented.
|
||||
"""
|
||||
lines = WORKFLOW.read_text(encoding="utf-8").splitlines()
|
||||
start = next(i for i, line in enumerate(lines) if line.strip() == f"- name: {name}")
|
||||
run_at = next(
|
||||
i for i in range(start, len(lines)) if lines[i].strip() in {"run: |", "run: >-"}
|
||||
)
|
||||
indent = len(lines[run_at + 1]) - len(lines[run_at + 1].lstrip())
|
||||
body = []
|
||||
for line in lines[run_at + 1 :]:
|
||||
if line.strip() and not line.startswith(" " * indent):
|
||||
break
|
||||
body.append(line[indent:] if len(line) >= indent else "")
|
||||
return "\n".join(body)
|
||||
|
||||
|
||||
def _run_step(name: str, cwd, env):
|
||||
return subprocess.run(
|
||||
["bash", "-euo", "pipefail", "-c", _step_script(name)],
|
||||
cwd=cwd, capture_output=True, text=True, env={**os.environ, **env},
|
||||
)
|
||||
|
||||
|
||||
class TestWorkflowStepsByExecution:
|
||||
"""Run the workflow's own steps against a bare origin (t44 3c).
|
||||
|
||||
Textual pins cannot tell whether the bump lands on the commit the tag names;
|
||||
these do, and they are what the dry run in the task output was.
|
||||
"""
|
||||
|
||||
VERSION = "2026.10.11"
|
||||
TAG = "v2026.10.11"
|
||||
|
||||
def _clone(self, tmp_path, *, protect=False):
|
||||
origin = tmp_path / "origin.git"
|
||||
subprocess.run(
|
||||
["git", "init", "--bare", "-q", "-b", "master", str(origin)], check=True
|
||||
)
|
||||
work = tmp_path / "work"
|
||||
subprocess.run(["git", "clone", "-q", str(origin), str(work)], check=True)
|
||||
_git(work, "config", "user.email", "dev@example.com")
|
||||
_git(work, "config", "user.name", "dev")
|
||||
manifest = work / MANIFEST_REL
|
||||
manifest.parent.mkdir(parents=True, exist_ok=True)
|
||||
manifest.write_text(
|
||||
json.dumps({"domain": "nvidia_shield", "version": "2026.10.08"}) + "\n"
|
||||
)
|
||||
_git(work, "add", "-A")
|
||||
_git(work, "commit", "-qm", "a feature commit")
|
||||
_git(work, "push", "-q", "origin", "master")
|
||||
if protect:
|
||||
hook = origin / "hooks" / "pre-receive"
|
||||
hook.write_text("#!/bin/sh\necho 'protected branch' >&2\nexit 1\n")
|
||||
hook.chmod(0o755)
|
||||
return work
|
||||
|
||||
def _env(self):
|
||||
return {
|
||||
"MANIFEST": MANIFEST_REL,
|
||||
"VERSION": self.VERSION,
|
||||
"TAG": self.TAG,
|
||||
}
|
||||
|
||||
def test_the_bump_verify_and_tag_steps_agree_on_one_commit(self, tmp_path):
|
||||
work = self._clone(tmp_path)
|
||||
|
||||
bump = _run_step("Write the version back into the manifest", work, self._env())
|
||||
assert bump.returncode == 0, bump.stderr
|
||||
assert _git(work, "log", "-1", "--pretty=%s").strip() == (
|
||||
f"chore(release): {self.TAG} [skip ci]"
|
||||
)
|
||||
|
||||
verify = _run_step("Verify the released manifest on master", work, self._env())
|
||||
assert verify.returncode == 0, verify.stderr
|
||||
assert f"matching {self.TAG}" in verify.stdout
|
||||
|
||||
tag = _run_step("Create tag", work, self._env())
|
||||
assert tag.returncode == 0, tag.stderr
|
||||
|
||||
head = _git(work, "rev-parse", "HEAD").strip()
|
||||
assert _git(work, "rev-parse", f"{self.TAG}^{{commit}}").strip() == head
|
||||
tagged_manifest = json.loads(_git(work, "show", f"{self.TAG}:{MANIFEST_REL}"))
|
||||
assert tagged_manifest["version"] == self.VERSION
|
||||
assert _git(work, "ls-remote", "--tags", "origin", self.TAG).split()[1] == (
|
||||
f"refs/tags/{self.TAG}"
|
||||
)
|
||||
|
||||
def test_the_verify_step_refuses_an_inconsistent_manifest(self, tmp_path):
|
||||
work = self._clone(tmp_path)
|
||||
_run_step("Write the version back into the manifest", work, self._env())
|
||||
# someone else's release lands on master before this one is verified
|
||||
(work / MANIFEST_REL).write_text(
|
||||
json.dumps({"domain": "nvidia_shield", "version": "2026.10.99"}) + "\n"
|
||||
)
|
||||
_git(work, "add", "-A")
|
||||
_git(work, "commit", "-qm", "a competing release")
|
||||
_git(work, "push", "-q", "origin", "master")
|
||||
|
||||
verify = _run_step("Verify the released manifest on master", work, self._env())
|
||||
|
||||
assert verify.returncode == 1
|
||||
assert "inconsistent" in verify.stderr
|
||||
assert "2026.10.99" in verify.stderr
|
||||
|
||||
def test_the_notes_range_describes_the_developers_commits(self, tmp_path):
|
||||
"""t44 R1, executed: the range the notes use must not open with the bot commit.
|
||||
|
||||
With a write-back the tag IS the bot's commit, so `PREV..TAG` (the range
|
||||
before this fix) published "- chore(release): vX [skip ci]" as the first
|
||||
line of every release. The range is built from ``BUMP``, and this runs both
|
||||
halves through real git.
|
||||
"""
|
||||
work = self._clone(tmp_path)
|
||||
_git(work, "tag", "-a", "v2026.10.08", "-m", "previous release")
|
||||
_git(work, "commit", "--allow-empty", "-qm", "feat: the real change")
|
||||
_run_step("Write the version back into the manifest", work, self._env())
|
||||
_run_step("Create tag", work, self._env())
|
||||
|
||||
def subjects(rev_range):
|
||||
return _git(work, "log", "--pretty=format:- %s", rev_range)
|
||||
|
||||
notes_end = f"{self.TAG}^" # BUMP=yes: the write-back commit is skipped
|
||||
prev = _git(work, "describe", "--tags", "--abbrev=0", f"{notes_end}^").strip()
|
||||
|
||||
assert prev == "v2026.10.08"
|
||||
assert subjects(f"{prev}..{notes_end}") == "- feat: the real change"
|
||||
# ...and the range this replaced really did include the bot's own subject
|
||||
assert "- chore(release):" in subjects(f"{prev}..{self.TAG}")
|
||||
|
||||
def test_a_rejected_bump_push_fails_and_tags_nothing(self, tmp_path):
|
||||
work = self._clone(tmp_path, protect=True)
|
||||
|
||||
bump = _run_step("Write the version back into the manifest", work, self._env())
|
||||
|
||||
assert bump.returncode == 1
|
||||
assert "could not push" in bump.stderr
|
||||
assert "manually" in bump.stderr
|
||||
assert MANIFEST_REL in bump.stderr
|
||||
assert _git(work, "tag", "-l").strip() == ""
|
||||
Reference in new issue
Block a user