diff --git a/.github/bump_version.py b/.github/bump_version.py index 78dc45bb0..6826f1e8d 100644 --- a/.github/bump_version.py +++ b/.github/bump_version.py @@ -1,12 +1,47 @@ -"""Infer semver bump from towncrier fragment types and update version.""" +"""Raise the version in pyproject.toml from the pending changelog fragments' types. + +The versioning job in .github/workflows/push.yml and ``make changelog`` run +this before Towncrier builds CHANGELOG.md. It reads the fragments through +``FragmentDefinition`` in changelog_fragments.py, beside this file, the +definition the PR check uses too (rule api.process.changelog-fragment). It +exits with status 1, changing nothing, when changelog.d/ holds anything that +isn't a fragment, holds no fragment, or when ``BUMP_BY_TYPE`` and the types in +[tool.towncrier] differ. +""" import re import sys from pathlib import Path -from typing import TextIO - -# A fragment named "..md" has at least two dot-separated stem parts. -STEM_PARTS_WITH_CATEGORY = 2 +from typing import NoReturn, TextIO + +from changelog_fragments import ( + CONFIG_TABLE, + ConfigurationError, + FragmentDefinition, +) + +# The version part each fragment type raises: breaking the major version, added +# and removed the minor version, and changed and fixed the patch version. Its +# keys must be the types in [tool.towncrier]; infer_bump refuses to run when +# they differ. +BUMP_BY_TYPE = { + "breaking": "major", + "added": "minor", + "changed": "patch", + "fixed": "patch", + "removed": "minor", +} +# Version parts from the smallest bump to the largest. +BUMP_SIZES = ("patch", "minor", "major") + + +def _listed(names: list[str]) -> str: + """Return ``names`` as "a, b and c", or "none".""" + if not names: + return "none" + if len(names) == 1: + return names[0] + return f"{', '.join(names[:-1])} and {names[-1]}" def _report(message: str, *, file: TextIO | None = None) -> None: @@ -14,48 +49,78 @@ def _report(message: str, *, file: TextIO | None = None) -> None: print(message, file=file) +def _fail(message: str) -> NoReturn: + """Print ``message`` to stderr and exit with status 1.""" + _report(message, file=sys.stderr) + sys.exit(1) + + def get_current_version(pyproject_path: Path) -> str: """Return the version in pyproject.toml, exiting with status 1 if it is absent.""" text = pyproject_path.read_text() match = re.search(r'^version\s*=\s*"(\d+\.\d+\.\d+)"', text, re.MULTILINE) if not match: - _report( - "Could not find version in pyproject.toml", - file=sys.stderr, - ) - sys.exit(1) + _fail("Could not find version in pyproject.toml") return match.group(1) -def infer_bump(changelog_dir: Path) -> str: - """Return "major", "minor" or "patch" from the pending changelog fragment types. +def _require_a_bump_for_each_type(types: tuple[str, ...]) -> None: + """Exit with status 1 unless ``BUMP_BY_TYPE`` has a bump for exactly ``types``.""" + if set(types) == set(BUMP_BY_TYPE): + return + _fail( + f"The fragment types in {CONFIG_TABLE} are {_listed(list(types))}, but " + f"BUMP_BY_TYPE in .github/bump_version.py has a bump for " + f"{_listed(list(BUMP_BY_TYPE))}. Types with no bump: " + f"{_listed([name for name in types if name not in BUMP_BY_TYPE])}. " + f"Bumps with no type: " + f"{_listed([name for name in BUMP_BY_TYPE if name not in types])}. " + "Change both together." + ) + + +def infer_bump(root: Path) -> str: + """Return "major", "minor" or "patch" from the fragments in ``root``'s changelog.d/. Any breaking fragment makes a major bump and any added or removed fragment a - minor one; exit with status 1 when there are no fragments. + minor one. Exit with status 1 when the directory holds a file that isn't a + fragment, or no fragment. """ - fragments = [ - f for f in changelog_dir.iterdir() if f.is_file() and f.name != ".gitkeep" + try: + definition = FragmentDefinition.load(root) + except ConfigurationError as error: + _fail(str(error)) + _require_a_bump_for_each_type(definition.types) + scan = definition.scan(root) + problems = [ + *scan.problems.values(), + *(definition.empty_folder_problem(path) for path in scan.empty_folders), ] - if not fragments: - _report("No changelog fragments found", file=sys.stderr) - sys.exit(1) - - categories = {f.suffix.lstrip(".") for f in fragments} - for f in fragments: - parts = f.stem.split(".") - if len(parts) >= STEM_PARTS_WITH_CATEGORY: - categories.add(parts[-1]) - - if "breaking" in categories: - return "major" - if "added" in categories or "removed" in categories: - return "minor" - return "patch" + if problems: + for problem in problems: + _report(problem, file=sys.stderr) + if len(problems) == 1: + unreadable = "1 of them is not a fragment. Fix it" + else: + unreadable = f"{len(problems)} of them are not fragments. Fix them" + _fail( + f"The release reads every file in {definition.directory}/, and " + f"{unreadable} in a pull request; the release stops until then." + ) + if not scan.fragments: + _fail( + f"No changelog fragments found in {definition.directory}/: there is " + "nothing to release." + ) + return max( + (BUMP_BY_TYPE[fragment.fragment_type] for fragment in scan.fragments), + key=BUMP_SIZES.index, + ) def bump_version(version: str, bump: str) -> str: """Return ``version`` raised by one ``bump`` level, resetting the lower parts.""" - major, minor, patch = (int(x) for x in version.split(".")) + major, minor, patch = (int(part) for part in version.split(".")) if bump == "major": return f"{major + 1}.0.0" if bump == "minor": @@ -79,10 +144,9 @@ def main() -> None: """Bump the version in pyproject.toml according to the changelog.d fragments.""" root = Path(__file__).resolve().parent.parent pyproject = root / "pyproject.toml" - changelog_dir = root / "changelog.d" current = get_current_version(pyproject) - bump = infer_bump(changelog_dir) + bump = infer_bump(root) new = bump_version(current, bump) _report(f"Version: {current} -> {new} ({bump})") diff --git a/.github/changelog_fragments.py b/.github/changelog_fragments.py new file mode 100644 index 000000000..eee415c04 --- /dev/null +++ b/.github/changelog_fragments.py @@ -0,0 +1,687 @@ +"""What a changelog fragment is, for the PR check and the release's version bump. + +Rule api.process.changelog-fragment. Every pull request adds a changelog +fragment. After a merge to master, the versioning job in +.github/workflows/push.yml runs .github/bump_version.py, which raises the +version in pyproject.toml from the pending fragments' types, and Towncrier then +builds the fragments into CHANGELOG.md. The PR check +(.github/scripts/check_changelog_fragment.sh) and the version bump both read +fragments through ``FragmentDefinition``, so the check accepts only what the +release reads. + +A fragment is a regular file directly in the fragment directory (``directory`` +in ``[tool.towncrier]`` in pyproject.toml: changelog.d) named +``..md``, where: + +- ```` is letters and digits, in groups joined by single dots, hyphens or + underscores, such as ``4083`` or ``update-policyengine-bundle-6.11.1``; +- ```` is the ``directory`` of one of the ``[[tool.towncrier.type]]`` + entries: breaking, added, changed, fixed or removed; +- its text, read as UTF-8, has at least one visible character: it is not + empty, only whitespace or only invisible characters such as a zero-width + space. + +``.gitkeep``, which keeps the directory in git once a release has removed every +fragment, is the only other file allowed there. Pull request #4084 added +changelog.d/added/4083.md: the old check counted every file under changelog.d/, +so it passed, and the release, which reads only files directly in changelog.d/, +found no fragment. + +Commands, run from the repository root: + + directory print the fragment directory + check-pull-request read ``git diff --find-renames --name-status -z`` output + for the fragment directory on stdin, from the pull + request's base to the checked-out commit, and exit 1 + unless the pull request adds a fragment of its own and + every file in the directory is a fragment. + ``--released-fragments FILE`` holds the fragments the + base branch removed after the pull request's branch left + it, as ``git log --diff-filter=D --raw -z`` prints them +""" + +from __future__ import annotations + +import argparse +import hashlib +import re +import sys +import tomllib +import unicodedata +from dataclasses import dataclass, field +from pathlib import Path, PurePosixPath + +RULE_ID = "api.process.changelog-fragment" +CONFIG_FILE = "pyproject.toml" +CONFIG_TABLE = "[tool.towncrier] in pyproject.toml" +DOCUMENTATION = "docs/engineering/skills/github-prs.md#changelog-fragments" +# Keeps the fragment directory in git once a release has removed every fragment. +KEEP_FILE = ".gitkeep" +FRAGMENT_EXTENSION = "md" +NAME_PATTERN = r"[A-Za-z0-9]+(?:[._-][A-Za-z0-9]+)*" +FRAGMENT_NAME = re.compile(NAME_PATTERN) +FRAGMENT_FILE_NAME = re.compile( + rf"(?P{NAME_PATTERN})\.(?P[^.]+)\.{FRAGMENT_EXTENSION}" +) +TYPE_NAME = re.compile(r"[a-z]+") +BYTE_ORDER_MARK = "\ufeff" +# Unicode general category groups a reader can see: letters, marks, numbers, +# punctuation and symbols. Separators (Z) and other characters (C: controls and +# format characters such as a zero-width space or a byte-order mark) aren't. +VISIBLE_CATEGORY_GROUPS = frozenset("LMNPS") +NAME_TERMS = ( + " is letters and digits, in groups joined by single dots, hyphens or " + "underscores" +) +# git diff --name-status statuses that name two paths: the source, then the +# destination. Every other status names one. +TWO_PATH_STATUSES = frozenset({"R", "C"}) +# Hexadecimal digits in a SHA-256 git object id; a SHA-1 id has 40. +SHA256_OBJECT_ID_LENGTH = 64 +ON_THE_BASE_BRANCH = ( + "It is already on the base branch, so the next release fails until a pull " + "request fixes it." +) +ALREADY_RELEASED = ( + "is a fragment the base branch has already released: a release removed it " + "after this branch left the base branch, so merging would publish it again. " + "This happens when a pull request lower in a stack merged; rebase this " + "branch onto the base branch without that pull request's commits. If it is " + "a new note that reuses the name, rename it." +) + + +class ConfigurationError(Exception): + """[tool.towncrier] doesn't say where fragments live or which types they have.""" + + +@dataclass(frozen=True) +class Fragment: + """A changelog fragment the release reads: its path, name and type.""" + + path: str + name: str + fragment_type: str + + +@dataclass(frozen=True) +class Change: + """One file a pull request changes, from ``git diff --name-status``. + + ``status`` is git's status letter: A added, C copied, D deleted, M + modified, R renamed, T changed type. ``source`` is the path a renamed or + copied file came from. + """ + + status: str + path: str + source: str | None = None + + +@dataclass(frozen=True) +class Scan: + """What the fragment directory holds: its fragments, and everything else. + + ``problems`` maps each other file and symbolic link, by its path from the + repository root, to why the release can't read it. ``empty_folders`` lists + the empty folders, which git can't commit: a checkout has one only when a + local command left it behind or a submodule is there. + """ + + fragments: tuple[Fragment, ...] + problems: dict[str, str] = field(default_factory=dict) + empty_folders: tuple[str, ...] = () + + +def _listed(names: tuple[str, ...]) -> str: + """Return ``names`` as "a, b or c".""" + if len(names) == 1: + return names[0] + return f"{', '.join(names[:-1])} or {names[-1]}" + + +def _nested_entries(folder: Path) -> list[Path]: + """Return every file and symbolic link under ``folder``, and each empty folder. + + Symbolic links to folders are returned, not followed. + """ + children = sorted(folder.iterdir()) + if not children: + return [folder] + entries: list[Path] = [] + for child in children: + if child.is_dir() and not child.is_symlink(): + entries.extend(_nested_entries(child)) + else: + entries.append(child) + return entries + + +def _file_kind_problem(path: str, file: Path) -> str | None: + """Return why ``file``, at ``path``, is not a regular file, or None.""" + if file.is_symlink(): + return ( + f"{path} is a symbolic link. A fragment is a regular file that holds " + "its text; replace the link with one." + ) + if not file.is_file(): + return ( + f"{path} is not a regular file. A fragment is a regular file that " + "holds its text; replace it with one." + ) + return None + + +def _content_problem(path: str, file: Path) -> str | None: + """Return why ``file``, at ``path``, holds no fragment text, or None.""" + kind_problem = _file_kind_problem(path, file) + if kind_problem: + return kind_problem + try: + text = file.read_text(encoding="utf-8") + except UnicodeDecodeError: + return f"{path} is not UTF-8 text. Save it as UTF-8." + if any( + unicodedata.category(character)[0] in VISIBLE_CATEGORY_GROUPS + for character in text + ): + return None + if not text: + emptiness = "is empty." + elif not text.removeprefix(BYTE_ORDER_MARK).strip(): + emptiness = "holds only whitespace." + else: + emptiness = ( + "holds no visible text, only invisible characters such as zero-width " + "spaces." + ) + return ( + f"{path} {emptiness} Write a sentence or two in it saying what changed: " + "Towncrier would publish an empty entry." + ) + + +@dataclass(frozen=True) +class FragmentDefinition: + """Where fragments live and which types they may have, from [tool.towncrier].""" + + directory: str + types: tuple[str, ...] + + @classmethod + def load(cls, root: Path) -> FragmentDefinition: + """Read the fragment directory and types from ``root``'s pyproject.toml.""" + config_path = root / CONFIG_FILE + try: + config = tomllib.loads(config_path.read_text(encoding="utf-8")) + towncrier = config["tool"]["towncrier"] + directory = towncrier["directory"] + types = tuple(entry["directory"] for entry in towncrier["type"]) + except (OSError, tomllib.TOMLDecodeError, KeyError, TypeError) as error: + msg = ( + f"Could not read the fragment directory and types from " + f"{CONFIG_TABLE} ({config_path}): {error!r}" + ) + raise ConfigurationError(msg) from error + if not isinstance(directory, str) or not FRAGMENT_NAME.fullmatch(directory): + msg = ( + f"{CONFIG_TABLE} names a fragment directory that is not one " + f"folder: {directory!r}" + ) + raise ConfigurationError(msg) + if not types or len(set(types)) != len(types): + msg = f"{CONFIG_TABLE} must list each fragment type once: {types!r}" + raise ConfigurationError(msg) + for fragment_type in types: + if ( + not isinstance(fragment_type, str) + or not TYPE_NAME.fullmatch(fragment_type) + or fragment_type == FRAGMENT_EXTENSION + ): + msg = ( + f'{CONFIG_TABLE} lists the fragment type "{fragment_type}"; a ' + f"type is lowercase letters, and not " + f'"{FRAGMENT_EXTENSION}", the fragment extension.' + ) + raise ConfigurationError(msg) + return cls(directory=directory, types=types) + + def listed_types(self) -> str: + """Return the fragment types as "a, b or c".""" + return _listed(self.types) + + def form_terms(self) -> str: + """Return what ```` and ```` stand for in a fragment name.""" + return f" is {self.listed_types()}, and {NAME_TERMS}" + + def accepted_form(self) -> str: + """Return the sentences that say how to add a fragment.""" + return ( + f"Add a file {self.directory}/..md, directly in " + f"{self.directory}/, that says in a sentence or two what changed for " + f"people who use or run the Public API. {self.form_terms()}, such as " + f"an issue number. See {DOCUMENTATION} (rule {RULE_ID})." + ) + + def _stops_the_release(self) -> str: + return ( + f"The release reads only files directly in {self.directory}/ and " + "stops at anything else there." + ) + + def _suggested_name(self, file_name: str) -> str | None: + """Return the fragment name ``file_name`` was probably meant to have. + + It takes the last dot-separated part that is a type, ignoring case, as + Towncrier reads a type, and the parts before it as the name. + """ + parts = file_name.strip().split(".") + for index in range(len(parts) - 1, 0, -1): + candidate_type = parts[index].strip().lower() + if candidate_type in self.types: + name = ".".join(parts[:index]) + if FRAGMENT_NAME.fullmatch(name): + return f"{name}.{candidate_type}.{FRAGMENT_EXTENSION}" + return None + return None + + def _moved_path(self, path: PurePosixPath) -> str | None: + """Return where a fragment filed in a folder belongs, if that is clear. + + ``changelog.d/added/4083.md`` belongs at ``changelog.d/4083.added.md``. + """ + match = FRAGMENT_FILE_NAME.fullmatch(path.name) + if match and match.group("type") in self.types: + return f"{self.directory}/{path.name}" + folders = path.relative_to(self.directory).parts[:-1] + name = path.name.removesuffix(f".{FRAGMENT_EXTENSION}") + if ( + len(folders) == 1 + and folders[0] in self.types + and FRAGMENT_NAME.fullmatch(name) + ): + return f"{self.directory}/{name}.{folders[0]}.{FRAGMENT_EXTENSION}" + return None + + def _nested_problem(self, path: PurePosixPath) -> str: + """Return why a file in a folder inside the fragment directory isn't read.""" + problem = ( + f"{path} is in a folder inside {self.directory}/. " + f"{self._stops_the_release()}" + ) + moved_path = self._moved_path(path) + if moved_path: + return f"{problem} Move it to {moved_path}." + return ( + f"{problem} Move it to {self.directory}/..md, where " + f"{self.form_terms()}, or delete it." + ) + + def _name_problem(self, path: str) -> str | None: + """Return why ``path`` is not where and how a fragment is named, or None.""" + location = PurePosixPath(path) + if not location.is_relative_to(self.directory) or location == PurePosixPath( + self.directory + ): + return f"{path} is not in {self.directory}/." + if location.parent != PurePosixPath(self.directory): + return self._nested_problem(location) + match = FRAGMENT_FILE_NAME.fullmatch(location.name) + if match is None: + suggested_name = self._suggested_name(location.name) + fix = ( + f"Rename it to {self.directory}/{suggested_name}." + if suggested_name + else ( + f"Rename it to {self.directory}/..md, where " + f"{self.form_terms()}." + ) + ) + return ( + f"{path} is not named ..md: its file name is " + f"{location.name!r}. The release can't read it. {fix}" + ) + fragment_type = match.group("type") + if fragment_type in self.types: + return None + if fragment_type.lower() in self.types: + return ( + f'{path} has the type "{fragment_type}", but types are lowercase. ' + f"Rename it to {self.directory}/{match.group('name')}." + f"{fragment_type.lower()}.{FRAGMENT_EXTENSION}." + ) + return ( + f'{path} has the type "{fragment_type}", which is not one of the types ' + f"in {CONFIG_TABLE}: {self.listed_types()}. Rename it to " + f"{self.directory}/{match.group('name')}..md with one of those " + "types." + ) + + def problem_with(self, path: str, file: Path) -> str | None: + """Return why ``file``, at ``path`` from the repository root, isn't a fragment. + + Return None when it is one. + """ + return ( + _file_kind_problem(path, file) + or self._name_problem(path) + or _content_problem(path, file) + ) + + def names_a_fragment(self, path: str) -> bool: + """Return whether ``path`` is where and how a fragment is named. + + This looks only at the path, not at a file's text. + """ + return self._name_problem(path) is None + + def added_paths(self, changes: list[Change]) -> set[str]: + """Return the paths a pull request adds a file at, by its ``changes``. + + A renamed file counts only when its old path was not a fragment name: + renaming a fragment already on the base branch adds no release note, + but moving a file the release couldn't read into place does. + """ + added = set() + for change in changes: + if change.status in {"A", "C"} or ( + change.status == "R" + and change.source is not None + and not self.names_a_fragment(change.source) + ): + added.add(change.path) + return added + + def _fragment(self, path: str) -> Fragment: + """Return the fragment at ``path``, a name ``problem_with`` accepted.""" + match = FRAGMENT_FILE_NAME.fullmatch(PurePosixPath(path).name) + if match is None: + msg = f"{path} is not named ..md" + raise ValueError(msg) + return Fragment( + path=path, name=match.group("name"), fragment_type=match.group("type") + ) + + def empty_folder_problem(self, path: str) -> str: + """Return why the release can't build with an empty folder at ``path``.""" + return ( + f"{path} is an empty folder or a submodule inside {self.directory}/. " + f"{self._stops_the_release()} Delete it." + ) + + def scan(self, root: Path) -> Scan: + """Return the fragments in ``root``'s fragment directory, and every other file. + + Every file and symbolic link in a folder inside the directory is a + problem: Towncrier stops at a folder there. + """ + directory = root / self.directory + if not directory.exists() and not directory.is_symlink(): + return Scan(fragments=()) + if directory.is_symlink() or not directory.is_dir(): + return Scan( + fragments=(), + problems={ + self.directory: ( + f"{self.directory} is not a folder. The release reads " + f"fragments from the folder {self.directory}/; make it one." + ) + }, + ) + fragments: list[Fragment] = [] + problems: dict[str, str] = {} + empty_folders: list[str] = [] + for entry in sorted(directory.iterdir()): + path = f"{self.directory}/{entry.name}" + if entry.is_dir() and not entry.is_symlink(): + for nested_entry in _nested_entries(entry): + nested_path = nested_entry.relative_to(root).as_posix() + if nested_entry.is_dir() and not nested_entry.is_symlink(): + empty_folders.append(nested_path) + else: + problems[nested_path] = self._nested_problem( + PurePosixPath(nested_path) + ) + continue + if entry.name == KEEP_FILE: + kind_problem = _file_kind_problem(path, entry) + if kind_problem: + problems[path] = kind_problem + continue + problem = self.problem_with(path, entry) + if problem is None: + fragments.append(self._fragment(path)) + else: + problems[path] = problem + return Scan( + fragments=tuple(fragments), + problems=problems, + empty_folders=tuple(empty_folders), + ) + + +def parse_name_status(output: bytes) -> list[Change]: + """Return the changes in ``git diff --name-status -z`` output. + + Paths that aren't UTF-8 keep their bytes as surrogate escapes, as + ``os.fsdecode`` decodes them on Linux. + """ + fields = [ + output_field.decode("utf-8", errors="surrogateescape") + for output_field in output.split(b"\0") + ] + if fields and not fields[-1]: + fields.pop() + changes = [] + index = 0 + while index < len(fields): + status = fields[index][:1] + path_count = 2 if status in TWO_PATH_STATUSES else 1 + paths = fields[index + 1 : index + 1 + path_count] + if not status or len(paths) != path_count: + msg = ( + f"git diff --name-status output ends in the middle of a change: " + f"{fields[index:]!r}" + ) + raise ValueError(msg) + if path_count == 1: + changes.append(Change(status=status, path=paths[0])) + else: + changes.append(Change(status=status, path=paths[1], source=paths[0])) + index += 1 + path_count + return changes + + +def parse_removed_blobs(output: bytes) -> dict[str, set[str]]: + """Return the blob ids each path held when deleted, from ``git log --raw -z``. + + Each deletion is a ``: D`` record and + then its path, NUL-separated; git separates one commit's records from the + next with a newline as well. + """ + fields = [ + output_field.strip(b"\n") + for output_field in output.split(b"\0") + if output_field.strip(b"\n") + ] + if len(fields) % 2 or not all(record.startswith(b":") for record in fields[::2]): + msg = f"git log --raw output is not records and paths: {fields!r}" + raise ValueError(msg) + removed_blobs: dict[str, set[str]] = {} + for record, path in zip(fields[::2], fields[1::2], strict=True): + removed_blob = record.split()[2].decode("ascii") + decoded_path = path.decode("utf-8", errors="surrogateescape") + removed_blobs.setdefault(decoded_path, set()).add(removed_blob) + return removed_blobs + + +def git_blob_id(file: Path, id_length: int) -> str: + """Return the id git gives ``file``'s bytes as a blob, as ``git hash-object``. + + ``id_length`` is the length of the repository's object ids: 40 for SHA-1, + 64 for SHA-256. + """ + data = file.read_bytes() + blob = b"blob %d\0" % len(data) + data + if id_length == SHA256_OBJECT_ID_LENGTH: + return hashlib.sha256(blob).hexdigest() + return hashlib.sha1(blob, usedforsecurity=False).hexdigest() + + +def _released_paths( + root: Path, added_paths: set[str], removed_blobs: dict[str, set[str]] +) -> set[str]: + """Return the added paths whose text is a blob the base branch removed there.""" + released = set() + for path in added_paths & set(removed_blobs): + file = root / path + if file.is_symlink() or not file.is_file(): + continue + id_length = len(next(iter(removed_blobs[path]))) + if git_blob_id(file, id_length) in removed_blobs[path]: + released.add(path) + return released + + +def annotation_message(message: str) -> str: + """Escape ``message`` so the Actions runner reads it as one annotation message.""" + return message.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + +def annotation_property(value: str) -> str: + """Escape ``value`` for an annotation property such as ``file=``.""" + return annotation_message(value).replace(":", "%3A").replace(",", "%2C") + + +def _print_error(message: str, path: str | None = None) -> None: + """Print ``message`` as an error annotation, on ``path`` when one is given.""" + location = f" file={annotation_property(path)}" if path else "" + print(f"::error{location}::{annotation_message(message)}") + + +def _no_own_fragment_message( + definition: FragmentDefinition, pending: list[str], changes: list[Change] +) -> str: + """Return the error for a pull request that adds no fragment of its own. + + ``pending`` lists the fragments already on the base branch. + """ + sentences = ["This pull request adds no changelog fragment of its own."] + if pending: + sentences.append( + "Fragments already on the base branch belong to the pull requests " + f"that added them: {', '.join(pending)}." + ) + sentences.extend( + "Renaming a fragment that is already on the base branch " + f"({change.source} to {change.path}) doesn't add one." + for change in changes + if change.status == "R" + and change.source is not None + and definition.names_a_fragment(change.source) + ) + sentences.append(definition.accepted_form()) + return " ".join(sentences) + + +def check_pull_request( + root: Path, + changes: list[Change], + removed_blobs: dict[str, set[str]] | None = None, +) -> int: + """Check the checkout in ``root`` of a pull request that makes ``changes``. + + ``removed_blobs`` maps each fragment the base branch removed after the + pull request's branch left it to the blobs it held. A fragment the pull + request adds back with that text is a problem, not a fragment of its own. + Print an error annotation for each file in the fragment directory that is + not a fragment, and one if the pull request adds no fragment of its own; + return the exit status. + """ + definition = FragmentDefinition.load(root) + scan = definition.scan(root) + changed_paths = {change.path for change in changes if change.status != "D"} + problems = { + path: problem if path in changed_paths else f"{problem} {ON_THE_BASE_BRANCH}" + for path, problem in scan.problems.items() + } + # A path the pull request changed that is neither a fragment, the keep + # file nor a problem holds no file in the checkout: it is a submodule. An + # empty folder the pull request didn't change was left by a local command, + # since git can't commit one. + read_paths = {fragment.path for fragment in scan.fragments} | set(problems) + keep_path = f"{definition.directory}/{KEEP_FILE}" + if (root / keep_path).is_file() and not (root / keep_path).is_symlink(): + read_paths.add(keep_path) + problems.update( + (path, definition.empty_folder_problem(path)) + for path in sorted(changed_paths - read_paths) + ) + added_paths = definition.added_paths(changes) + resurrected_paths = _released_paths(root, added_paths, removed_blobs or {}) + problems.update( + (path, f"{path} {ALREADY_RELEASED}") for path in sorted(resurrected_paths) + ) + own_fragments = [ + fragment + for fragment in scan.fragments + if fragment.path in added_paths - resurrected_paths + ] + for path, problem in problems.items(): + _print_error(problem, path) + if not own_fragments: + pending = [ + fragment.path + for fragment in scan.fragments + if fragment.path not in resurrected_paths + ] + _print_error(_no_own_fragment_message(definition, pending, changes)) + if problems or not own_fragments: + return 1 + + count = ( + "1 changelog fragment" + if len(own_fragments) == 1 + else f"{len(own_fragments)} changelog fragments" + ) + listed = ", ".join( + f"{fragment.path} ({fragment.fragment_type})" for fragment in own_fragments + ) + print(f"This pull request adds {count}: {listed}.") + return 0 + + +def main(arguments: list[str] | None = None) -> int: + """Run the command named in ``arguments``; return the exit status.""" + parser = argparse.ArgumentParser(description=__doc__.split("\n", 1)[0]) + commands = parser.add_subparsers(dest="command", required=True) + commands.add_parser("directory", help="print the fragment directory") + check_parser = commands.add_parser( + "check-pull-request", + help="check a pull request; git diff --name-status -z output on stdin", + ) + check_parser.add_argument( + "--released-fragments", + type=Path, + help="git log --raw -z deletions on the base branch since the branch left it", + ) + options = parser.parse_args(arguments) + root = Path.cwd() + try: + if options.command == "directory": + print(FragmentDefinition.load(root).directory) + return 0 + changes = parse_name_status(sys.stdin.buffer.read()) + removed_blobs = ( + parse_removed_blobs(options.released_fragments.read_bytes()) + if options.released_fragments + else {} + ) + return check_pull_request(root, changes, removed_blobs) + except (ConfigurationError, ValueError) as error: + _print_error(str(error)) + return 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index daaae976d..86564102f 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -7,3 +7,9 @@ Fixes # ## Testing - + + diff --git a/.github/scripts/check_changelog_fragment.sh b/.github/scripts/check_changelog_fragment.sh index 280e9eb0e..ba621218c 100644 --- a/.github/scripts/check_changelog_fragment.sh +++ b/.github/scripts/check_changelog_fragment.sh @@ -1,11 +1,75 @@ #!/usr/bin/env bash +# Require the pull request to add a changelog fragment the release can read +# (rule api.process.changelog-fragment). The "Check changelog fragment" job in +# .github/workflows/pr.yml runs this on the commit GitHub checks out. +# +# It lists what the pull request changes in the fragment directory +# (changelog.d/) relative to its base, and passes that to +# .github/changelog_fragments.py. That fails unless the pull request adds a +# fragment of its own and every file in the directory is a fragment. Fragments +# already on the base branch belong to the pull requests that added them, so +# editing, deleting or renaming one doesn't count; moving a file the release +# couldn't read into place does. +# +# The comparison starts at the merge base of BASE_SHA and HEAD. For a pull +# request event, GitHub checks out its test merge commit, whose first parent +# is BASE_SHA, so this is exactly what merging the pull request adds to its +# base. Checked out at the pull request's own head, it is what the branch has +# added since it left the base branch. +# +# It also lists the fragments the base branch removed after the pull +# request's branch left it, with the blob each one held; a release removes +# each fragment it builds. A branch that still holds the commits of a lower +# pull request in a stack, squash-merged and released since, would add that +# fragment back with the same text, so it must not count. +# +# Run from the repository root, in a clone with the base branch's history +# (actions/checkout with fetch-depth: 0). +# +# Environment: +# BASE_SHA the commit the pull request merges into +# (github.event.pull_request.base.sha); locally, for example, +# BASE_SHA=origin/master +# PYTHON the Python 3.11+ interpreter to run the check with +# (default: python3) + set -euo pipefail -fragments="$(find changelog.d -type f ! -name '.gitkeep' | wc -l)" -if [[ "${fragments}" -eq 0 ]]; then - echo "::error::No changelog fragment found in changelog.d/" - echo "Add one with: echo 'Description.' > changelog.d/\$(git branch --show-current)..md" - echo "Types: added, changed, fixed, removed, breaking" +base_revision="${BASE_SHA:?BASE_SHA is required: the commit the pull request merges into, such as origin/master}" +python_bin="${PYTHON:-python3}" +fragments_module="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)/changelog_fragments.py" + +if [[ "$(git rev-parse --is-shallow-repository)" == "true" ]]; then + echo "::error::This clone is shallow, so it may lack the base commit or the history that leads to it. Check out with fetch-depth: 0." + exit 1 +fi + +if ! base_commit="$(git merge-base "${base_revision}" HEAD)"; then + echo "::error::Could not find a common ancestor of ${base_revision} and HEAD. The clone needs the base branch's history: check out with fetch-depth: 0." + exit 1 +fi + +# The module prints its own error annotation when it can't read the directory. +if ! fragment_directory="$("${python_bin}" "${fragments_module}" directory)"; then + echo "${fragment_directory}" exit 1 fi + +# The pull request's own head: the second parent of GitHub's test merge +# commit, or HEAD when HEAD is the head itself. +pull_request_head="HEAD" +if git rev-parse --verify --quiet "HEAD^2" >/dev/null \ + && [[ "$(git rev-parse "HEAD^1")" == "$(git rev-parse "${base_revision}^{commit}")" ]]; then + pull_request_head="HEAD^2" +fi +branch_point="$(git merge-base "${base_revision}" "${pull_request_head}")" + +released_fragments_file="$(mktemp)" +trap 'rm -f "${released_fragments_file}"' EXIT +git log --diff-filter=D --no-renames --raw --no-abbrev -z --format= \ + "${branch_point}..${base_revision}" -- "${fragment_directory}" >"${released_fragments_file}" + +git diff --find-renames --name-status -z "${base_commit}" HEAD -- "${fragment_directory}" \ + | "${python_bin}" "${fragments_module}" check-pull-request \ + --released-fragments "${released_fragments_file}" diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index a673dc34c..6e63b92ba 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -157,13 +157,30 @@ jobs: secrets: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} check-changelog: + # Rule api.process.changelog-fragment: every pull request adds a changelog + # fragment the release can read. See + # docs/engineering/skills/github-prs.md#changelog-fragments. name: Check changelog fragment # Full CI runs only on the lowest unmerged pull request in a stack. if: ${{ !github.event.pull_request.stack || github.event.pull_request.stack.base.ref == github.event.pull_request.base.ref }} runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read steps: - - uses: actions/checkout@v4 + - name: Checkout repo + uses: actions/checkout@v4 + with: + # The check compares the checked-out merge commit with the pull + # request's base commit, so it needs the base branch's history. + fetch-depth: 0 + - name: Setup Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" - name: Check for changelog fragment + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} run: bash .github/scripts/check_changelog_fragment.sh test_container_builds: name: Docker diff --git a/AGENTS.md b/AGENTS.md index 3cf239dfd..1f7b8c19e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,8 +55,11 @@ code), or splitting a module into smaller ones, read Read `docs/engineering/skills/github-prs.md` before opening, replacing, or sharing any pull request. Every PR needs a tracking issue and -`Fixes #ISSUE_NUMBER` as the first line of its description. Agents must never -bypass branch rules (for example with `gh pr merge --admin`). +`Fixes #ISSUE_NUMBER` as the first line of its description. Every PR also adds +a changelog fragment of its own: a non-empty file directly in `changelog.d/`, +named `changelog.d/..md`, where `` is `breaking`, `added`, +`changed`, `fixed` or `removed` (see "Changelog fragments" in that skill). +Agents must never bypass branch rules (for example with `gh pr merge --admin`). Before creating or sharing a migration PR, run the focused migration guards: diff --git a/README.md b/README.md index 221e2a932..b95c00ee8 100644 --- a/README.md +++ b/README.md @@ -220,18 +220,16 @@ This will automatically format the code for you; no need to do anything else. ### Step 3: Changelogging -Finally, we ask contributors to make it clear for our users what changes have been made by contributing to a changelog. This changelog is formatted in YAML and describes the changes you've made to the code. This should follow the below format: +Finally, we ask contributors to make it clear for our users what changes have been made by adding a changelog fragment. Every PR adds at least one: a file directly in `changelog.d/`, named `changelog.d/..md`, that says in a sentence or two what changed. + +- `` is `breaking`, `added`, `changed`, `fixed` or `removed`. It sets the version bump: `breaking` raises the major version, `added` and `removed` the minor version, and `changed` and `fixed` the patch version. +- `` is letters and digits, in groups joined by single dots, hyphens or underscores, such as the issue number. ``` -- bump: {major, minor, patch} - changes: - {added, removed, changed, fixed}: - - +echo 'Report when the deployed version went live.' > changelog.d/4083.added.md ``` -For more info on the syntax, check out the [semantic versioning docs](https://www.semver.org) and [keep a changelog](https://www.keepachangelog.com). - -Write your changelog info into the empty file called `changelog_entry.yaml`. When you open your PR, this will automatically be added to the overall changelog. +The `Check changelog fragment` PR job fails unless your PR adds such a file, and fails on any other file in `changelog.d/`, such as one in a folder (`changelog.d/added/4083.md`) or an empty one. After the PR merges, the release builds the fragments into `CHANGELOG.md` with [Towncrier](https://towncrier.readthedocs.io) and removes them. See [the PR skill](docs/engineering/skills/github-prs.md#changelog-fragments) for the full rule. ## Opening a Pull Request diff --git a/changelog.d/changelog-fragment-check.fixed.md b/changelog.d/changelog-fragment-check.fixed.md new file mode 100644 index 000000000..140a33989 --- /dev/null +++ b/changelog.d/changelog-fragment-check.fixed.md @@ -0,0 +1 @@ +The changelog fragment check on pull requests accepts only fragments the release reads: the pull request must add a non-empty file directly in `changelog.d/`, named `..md` with one of the types in `[tool.towncrier]`. Fragments other pull requests left on master no longer count, and the version bump and Towncrier stop at any file in `changelog.d/` they can't read instead of leaving it out. diff --git a/docs/engineering/skills/github-prs.md b/docs/engineering/skills/github-prs.md index 2973ae04d..ef00824a0 100644 --- a/docs/engineering/skills/github-prs.md +++ b/docs/engineering/skills/github-prs.md @@ -11,6 +11,60 @@ Before opening a PR, all developers and agents must: 2. Put `Fixes #ISSUE_NUMBER` as the first line of the PR description, using the issue number from the previous step. The PR template starts with this line. +## Changelog fragments + +Every PR adds at least one changelog fragment of its own +(`api.process.changelog-fragment`). A fragment is a file directly in +`changelog.d/`, named `changelog.d/..md`: + +- `` is one of the types in `[tool.towncrier]` in `pyproject.toml`: + `breaking`, `added`, `changed`, `fixed` or `removed`, in lowercase. It sets + the release's version bump: `breaking` raises the major version, `added` and + `removed` the minor version, and `changed` and `fixed` the patch version. +- `` is letters and digits, in groups joined by single dots, hyphens or + underscores: an issue number such as `4083`, or a short label such as + `report-live-since`. A branch name with a `/` in it makes a folder, which + doesn't count. +- The file holds a sentence or two, in UTF-8, saying what changed for people + who use or run the Public API. An empty file, or one with only whitespace + or invisible characters such as a zero-width space, fails. + +For example: + +```bash +echo 'Report when the deployed version went live.' > changelog.d/4083.added.md +``` + +The `Check changelog fragment` job in `pr.yml` compares the PR with its base +commit (`github.event.pull_request.base.sha`): + +- It fails unless the PR adds such a file. Fragments already on master don't + count: they belong to the PRs that added them until the next release removes + them. Editing, deleting or renaming one of them doesn't count either, but + moving a file the release couldn't read to a fragment name does. git decides + what is a rename: a file deleted and one added with mostly the same text. +- A fragment the base branch has already released doesn't count, and fails. + When a lower PR in a stack is squash-merged, the release removes its + fragment; an upper branch that still holds the lower PR's commits would add + it back with the same text. Rebase the upper branch onto master without + those commits. A new fragment that only reuses a released one's name counts. +- It fails on every file in `changelog.d/` the release can't read: a file in a + folder (#4084's `changelog.d/added/4083.md` belonged at + `changelog.d/4083.added.md`), another name, an unknown or uppercase type, an + empty file, a symbolic link or a submodule. Each error says how to fix it. + +After a merge to master, the `versioning` job in `push.yml` runs +`.github/bump_version.py` and then Towncrier. The bump reads fragments through +the same definition as the check, `.github/changelog_fragments.py`, and stops +at any file it can't read; `ignore = [".gitkeep"]` makes Towncrier stop too. +A release therefore never leaves out a fragment. + +To run the check locally before pushing: + +```bash +BASE_SHA=origin/master bash .github/scripts/check_changelog_fragment.sh +``` + ## Branch Rules Agents must never bypass branch rules, even when their credentials have admin @@ -45,7 +99,8 @@ declares an API contract change. If a PR changes migration route contracts, route-group metadata, generated migration artifacts, or the cutover guardrails, it should: -1. Include a changelog fragment. +1. Include a changelog fragment (see + [Changelog fragments](#changelog-fragments)). 2. Refresh generated migration artifacts with `python scripts/export_migration_contracts.py`. 3. Run `python scripts/run_quality_guards.py`. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 5acfc789d..079f41b3f 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -470,7 +470,7 @@ flowchart LR | `api.agents.adapters-point-to-agents-md` | CLAUDE.md and .github/copilot-instructions.md only point to AGENTS.md; skills and rules are listed in AGENTS.md alone. | mechanical | enforced | yes | quality guard `rules-registry` | `pr.yml:quality-guards` | yes | | `api.process.agents-never-bypass` | Agents never bypass branch rules, even with admin credentials: no gh pr merge --admin and no bypass option; a blocked merge is reported to a human. | mechanical | doc-only | no | nothing | — | — | | `api.process.agents-suppress-as-last-resort` | Agents add an inline ruff suppression (noqa) only when neither a code change nor a ruff setting removes the finding, and a PR lists every inline suppression it adds, with the reason. | judgment | doc-only | no | nothing | — | — | -| `api.process.changelog-fragment` | Every PR adds a changelog.d fragment. | mechanical | enforced | yes | CI job `check-changelog` | `pr.yml:check-changelog` | — | +| `api.process.changelog-fragment` | Every PR adds, relative to its base commit, at least one changelog fragment of its own, a non-empty file directly in changelog.d/ named `..md` with a type from [tool.towncrier] in pyproject.toml, and leaves nothing else there but .gitkeep; the PR check and the release's version bump read fragments through one definition, .github/changelog_fragments.py. | mechanical | enforced | yes | CI job `check-changelog`, `versioning` | `pr.yml:check-changelog`, `push.yml:versioning` | yes | | `api.process.cloud-run-image-builds` | The Cloud Run container image builds. | mechanical | enforced | yes | CI job `test_cloud_run_container_builds` | `pr.yml:test_cloud_run_container_builds` | — | | `api.process.docker-image-builds` | The API Docker image builds. | mechanical | enforced | yes | CI job `test_container_builds` | `pr.yml:test_container_builds` | — | | `api.process.makefile-phony` | Command-only Makefile targets are declared in .PHONY, and .PHONY stays in sync when targets are renamed or removed. | mechanical | doc-only | no | nothing | — | — | @@ -488,8 +488,9 @@ flowchart LR gap_unenforced>"not enforced"] r_api_process_agents_suppress_as_last_resort("api.process.agents-suppress-as-last-resort (doc-only)") r_api_process_changelog_fragment("api.process.changelog-fragment (enforced)") - e_CI_job_check_changelog["CI job check-changelog"] + e_CI_job_check_changelog__versioning["CI job check-changelog, versioning"] j_CI_pr_yml_check_changelog["CI pr.yml:check-changelog"] + j_CI_push_yml_versioning["CI push.yml:versioning"] r_api_process_cloud_run_image_builds("api.process.cloud-run-image-builds (enforced)") e_CI_job_test_cloud_run_container_builds["CI job test_cloud_run_container_builds"] j_CI_pr_yml_test_cloud_run_container_builds["CI pr.yml:test_cloud_run_container_builds"] @@ -508,8 +509,9 @@ flowchart LR e_quality_guard_rules_registry --> j_CI_pr_yml_quality_guards r_api_process_agents_never_bypass -.-> gap_unenforced r_api_process_agents_suppress_as_last_resort -.-> gap_unenforced - r_api_process_changelog_fragment --> e_CI_job_check_changelog - e_CI_job_check_changelog --> j_CI_pr_yml_check_changelog + r_api_process_changelog_fragment --> e_CI_job_check_changelog__versioning + e_CI_job_check_changelog__versioning --> j_CI_pr_yml_check_changelog + e_CI_job_check_changelog__versioning --> j_CI_push_yml_versioning r_api_process_cloud_run_image_builds --> e_CI_job_test_cloud_run_container_builds e_CI_job_test_cloud_run_container_builds --> j_CI_pr_yml_test_cloud_run_container_builds r_api_process_docker_image_builds --> e_CI_job_test_container_builds diff --git a/pyproject.toml b/pyproject.toml index 77aebfb1d..f419cfa3d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -176,6 +176,11 @@ warn_return_any = true warn_unused_ignores = true show_error_codes = true +# .github/changelog_fragments.py reads `directory` and the types' `directory` +# names below, so the PR check, the version bump in .github/bump_version.py and +# Towncrier share one list (rule api.process.changelog-fragment). Adding or +# removing a type also needs its entry in BUMP_BY_TYPE in bump_version.py, +# which refuses to run otherwise. [tool.towncrier] package = "policyengine_api" directory = "changelog.d" @@ -183,6 +188,9 @@ filename = "CHANGELOG.md" title_format = "## [{version}] - {project_date}" issue_format = "" underlines = ["", "", ""] +# With an ignore list, Towncrier fails on any other file in changelog.d/, such as +# a folder or a file with an unknown type, instead of skipping it. +ignore = [".gitkeep"] [[tool.towncrier.type]] directory = "breaking" diff --git a/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md b/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md new file mode 100644 index 000000000..18289a059 --- /dev/null +++ b/rules/fixtures/api.process.changelog-fragment/bad/changelog.d/added/4083.md @@ -0,0 +1 @@ +`GET /deployment` reports the policyengine.py version the API serves and when it went live; the candidate deploy keeps that date across redeploys of the same version. diff --git a/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md b/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md new file mode 100644 index 000000000..18289a059 --- /dev/null +++ b/rules/fixtures/api.process.changelog-fragment/good/changelog.d/report-live-since.added.md @@ -0,0 +1 @@ +`GET /deployment` reports the policyengine.py version the API serves and when it went live; the candidate deploy keeps that date across redeploys of the same version. diff --git a/rules/registry.toml b/rules/registry.toml index a10ccd0ba..37a41e923 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -1401,9 +1401,14 @@ kind = "none" [[rule]] id = "api.process.changelog-fragment" category = "process" -statement = "Every PR adds a changelog.d fragment." -rationale = "docs/engineering/skills/github-prs.md" -scope = ["changelog.d"] +statement = "Every PR adds, relative to its base commit, at least one changelog fragment of its own, a non-empty file directly in changelog.d/ named `..md` with a type from [tool.towncrier] in pyproject.toml, and leaves nothing else there but .gitkeep; the PR check and the release's version bump read fragments through one definition, .github/changelog_fragments.py." +rationale = "docs/engineering/skills/github-prs.md#changelog-fragments" +scope = [ + "changelog.d", + ".github/changelog_fragments.py", + ".github/bump_version.py", + ".github/scripts/check_changelog_fragment.sh", +] tier = "mechanical" status = "enforced" autofix = false @@ -1413,6 +1418,17 @@ kind = "workflow-job" workflow = ".github/workflows/pr.yml" job = "check-changelog" invocation = "check_changelog_fragment.sh" +[[rule.ci]] +workflow = ".github/workflows/push.yml" +job = "versioning" +invocation = "bump_version.py" +[rule.proof] +bad_fixture = "rules/fixtures/api.process.changelog-fragment/bad" +good_fixture = "rules/fixtures/api.process.changelog-fragment/good" +test = "tests/unit/test_changelog_fragments.py::test_rule_proof_changelog_fragment" +workflow = ".github/workflows/pr.yml" +job = "test" +invocation = "make test" [[rule]] id = "api.process.docker-image-builds" diff --git a/tests/unit/changelog_fragment_support.py b/tests/unit/changelog_fragment_support.py new file mode 100644 index 000000000..4db012db0 --- /dev/null +++ b/tests/unit/changelog_fragment_support.py @@ -0,0 +1,52 @@ +"""Load .github/changelog_fragments.py and .github/bump_version.py for tests. + +``python .github/bump_version.py`` puts .github/ first on ``sys.path``, so the +bump script imports ``changelog_fragments`` from the file beside it. These +helpers load both files by path and register each under its module name, so +the bump script's import finds the fragment module without a change to +``sys.path``. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from types import ModuleType + +REPO = Path(__file__).resolve().parents[2] +GITHUB_DIRECTORY = REPO / ".github" + + +def load_github_module(module_name: str) -> ModuleType: + """Return the module in .github/.py, loading it on first use.""" + loaded_module = sys.modules.get(module_name) + if loaded_module is not None: + return loaded_module + spec = importlib.util.spec_from_file_location( + module_name, GITHUB_DIRECTORY / f"{module_name}.py" + ) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules[module_name] = module + try: + spec.loader.exec_module(module) + except BaseException: + del sys.modules[module_name] + raise + return module + + +def changelog_fragments() -> ModuleType: + """Return .github/changelog_fragments.py.""" + return load_github_module("changelog_fragments") + + +def bump_version() -> ModuleType: + """Return .github/bump_version.py, after the fragment module it imports.""" + changelog_fragments() + return load_github_module("bump_version") diff --git a/tests/unit/test_bump_version.py b/tests/unit/test_bump_version.py index 4929f3c6a..e7b174394 100644 --- a/tests/unit/test_bump_version.py +++ b/tests/unit/test_bump_version.py @@ -1,66 +1,313 @@ -"""Characterization tests for the changelog-driven version bump inference.""" +"""Tests for .github/bump_version.py, which raises the version at each release. -import importlib.util -from pathlib import Path -from types import ModuleType +The versioning job in .github/workflows/push.yml runs it after a merge to +master, before Towncrier builds CHANGELOG.md. It reads the pending fragments +through .github/changelog_fragments.py, the definition the PR check uses (rule +api.process.changelog-fragment), and stops at any file it can't read. +""" + +from __future__ import annotations + +import re +import shutil +import subprocess +import sys +import tomllib +from typing import TYPE_CHECKING import pytest -BUMP_VERSION_PATH = Path(__file__).parents[2] / ".github" / "bump_version.py" +from tests.unit.changelog_fragment_support import ( + GITHUB_DIRECTORY, + REPO, + bump_version, + changelog_fragments, +) + +if TYPE_CHECKING: + from pathlib import Path + +bump_module = bump_version() + +FRAGMENT_TEXT = "A change.\n" +UNREADABLE_SUMMARY = ( + "The release reads every file in changelog.d/, and 1 of them is not a " + "fragment. Fix it in a pull request; the release stops until then.\n" +) -def _load_bump_version() -> ModuleType: - spec = importlib.util.spec_from_file_location("bump_version", BUMP_VERSION_PATH) - assert spec is not None - assert spec.loader is not None - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - return module +def _write(root: Path, relative_path: str, text: str = FRAGMENT_TEXT) -> None: + file_path = root / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") -def _changelog_dir(tmp_path: Path, *fragment_names: str) -> Path: - changelog_dir = tmp_path / "changelog.d" - changelog_dir.mkdir() - for name in fragment_names: - (changelog_dir / name).write_text("A change.\n") - return changelog_dir +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a root holding the repository's pyproject.toml and changelog.d/.""" + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path @pytest.mark.parametrize( ("fragment_names", "expected_bump"), [ (("1.fixed.md",), "patch"), - (("1.fixed",), "patch"), - (("notes",), "patch"), + (("1.changed.md",), "patch"), + (("1.changed.md", "2.fixed.md"), "patch"), (("1.added.md",), "minor"), - (("1.added",), "minor"), + (("1.removed.md",), "minor"), (("1.removed.md", "2.fixed.md"), "minor"), + (("1.breaking.md",), "major"), (("a.b.breaking.md", "1.added.md"), "major"), - (("1.breaking",), "major"), - (("breaking.md",), "patch"), - ((".added.md",), "minor"), - (("1.changed.md", ".gitkeep"), "patch"), + (("update-policyengine-bundle-6.11.1.changed.md",), "patch"), ], ) -def test_infer_bump_reads_the_category_from_the_suffix_or_the_stem( - tmp_path: Path, fragment_names: tuple[str, ...], expected_bump: str -) -> None: - bump_version = _load_bump_version() - - assert ( - bump_version.infer_bump(_changelog_dir(tmp_path, *fragment_names)) - == expected_bump - ) +def test_infer_bump_takes_the_largest_bump_a_pending_fragment_calls_for( + repository: Path, fragment_names: tuple[str, ...], expected_bump: str +): + for fragment_name in fragment_names: + _write(repository, f"changelog.d/{fragment_name}") + assert bump_module.infer_bump(repository) == expected_bump -def test_infer_bump_exits_when_only_gitkeep_is_present( + +def test_the_bump_table_has_a_bump_for_exactly_the_configured_types(): + definition = changelog_fragments().FragmentDefinition.load(REPO) + + assert bump_module.BUMP_BY_TYPE == { + "breaking": "major", + "added": "minor", + "changed": "patch", + "fixed": "patch", + "removed": "minor", + } + assert set(bump_module.BUMP_BY_TYPE) == set(definition.types) + + +def test_infer_bump_refuses_types_the_bump_table_does_not_list( tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - bump_version = _load_bump_version() - changelog_dir = _changelog_dir(tmp_path, ".gitkeep") +): + type_tables = "".join( + f'\n[[tool.towncrier.type]]\ndirectory = "{fragment_type}"\n' + for fragment_type in ("added", "fixed", "security") + ) + (tmp_path / "pyproject.toml").write_text( + f'[tool.towncrier]\ndirectory = "changelog.d"\n{type_tables}', + encoding="utf-8", + ) + _write(tmp_path, "changelog.d/1.security.md") + + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(tmp_path) + + assert exit_info.value.code == 1 + assert capsys.readouterr().err == ( + "The fragment types in [tool.towncrier] in pyproject.toml are added, " + "fixed and security, but BUMP_BY_TYPE in .github/bump_version.py has a " + "bump for breaking, added, changed, fixed and removed. Types with no " + "bump: security. Bumps with no type: breaking, changed and removed. " + "Change both together.\n" + ) + + +@pytest.mark.parametrize( + ("relative_path", "text", "expected_problem"), + [ + ( + "changelog.d/added/4083.md", + FRAGMENT_TEXT, + "changelog.d/added/4083.md is in a folder inside changelog.d/. The " + "release reads only files directly in changelog.d/ and stops at " + "anything else there. Move it to changelog.d/4083.added.md.", + ), + ("changelog.d/notes", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/1.added", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/.added.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/breaking.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/1.feature.md", FRAGMENT_TEXT, 'has the type "feature"'), + ("changelog.d/1.added.md", "", "changelog.d/1.added.md is empty."), + ( + "changelog.d/1.added.md", + " \n", + "changelog.d/1.added.md holds only whitespace.", + ), + ], + ids=[ + "pull-request-4084", + "no-extension", + "no-md-extension", + "no-name", + "no-type", + "unknown-type", + "empty", + "whitespace-only", + ], +) +def test_infer_bump_stops_at_a_file_it_cannot_read( + repository: Path, + capsys: pytest.CaptureFixture[str], + relative_path: str, + text: str, + expected_problem: str, +): + _write(repository, "changelog.d/2.fixed.md") + _write(repository, relative_path, text) + + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(repository) + + assert exit_info.value.code == 1 + error_output = capsys.readouterr().err + assert expected_problem in error_output + assert error_output.endswith(UNREADABLE_SUMMARY) + + +def test_infer_bump_stops_at_an_empty_folder( + repository: Path, capsys: pytest.CaptureFixture[str] +): + _write(repository, "changelog.d/2.fixed.md") + (repository / "changelog.d/vendored").mkdir() + + with pytest.raises(SystemExit) as exit_info: + bump_module.infer_bump(repository) + + assert exit_info.value.code == 1 + assert capsys.readouterr().err == ( + "changelog.d/vendored is an empty folder or a submodule inside " + "changelog.d/. The release reads only files directly in changelog.d/ " + "and stops at anything else there. Delete it.\n" + UNREADABLE_SUMMARY + ) + +def test_infer_bump_exits_when_only_gitkeep_is_present( + repository: Path, capsys: pytest.CaptureFixture[str] +): with pytest.raises(SystemExit) as exit_info: - bump_version.infer_bump(changelog_dir) + bump_module.infer_bump(repository) assert exit_info.value.code == 1 - assert capsys.readouterr().err == "No changelog fragments found\n" + assert capsys.readouterr().err == ( + "No changelog fragments found in changelog.d/: there is nothing to release.\n" + ) + + +def _release_checkout(tmp_path: Path) -> Path: + """Return a copy of the files the versioning job's bump step reads.""" + (tmp_path / ".github").mkdir() + for script_name in ("bump_version.py", "changelog_fragments.py"): + shutil.copy(GITHUB_DIRECTORY / script_name, tmp_path / ".github" / script_name) + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path + + +def _project_version(root: Path) -> tuple[int, int, int]: + match = re.search( + r'^version = "(\d+)\.(\d+)\.(\d+)"', + (root / "pyproject.toml").read_text(encoding="utf-8"), + re.MULTILINE, + ) + assert match is not None + major, minor, patch = (int(part) for part in match.groups()) + return major, minor, patch + + +def _run_bump(root: Path) -> subprocess.CompletedProcess[str]: + """Run the copied bump script as push.yml does: python .github/bump_version.py.""" + return subprocess.run( + [sys.executable, ".github/bump_version.py"], + cwd=root, + text=True, + capture_output=True, + check=False, + ) + + +def test_the_script_raises_the_version_as_the_release_runs_it(tmp_path: Path): + root = _release_checkout(tmp_path) + _write(root, "changelog.d/4083.added.md") + major, minor, _patch = _project_version(root) + + result = _run_bump(root) + + assert result.returncode == 0, result.stderr + assert _project_version(root) == (major, minor + 1, 0) + assert result.stdout.startswith("Version: ") + assert result.stdout.splitlines()[0].endswith(f"-> {major}.{minor + 1}.0 (minor)") + + +def test_the_script_stops_at_pull_request_4084s_fragment(tmp_path: Path): + root = _release_checkout(tmp_path) + _write(root, "changelog.d/2.fixed.md") + _write(root, "changelog.d/added/4083.md") + version_before = _project_version(root) + + result = _run_bump(root) + + assert result.returncode == 1, result.stdout + assert "Move it to changelog.d/4083.added.md." in result.stderr + assert _project_version(root) == version_before + + +def _towncrier_draft(root: Path) -> subprocess.CompletedProcess[str]: + """Run the locked Towncrier's draft build in ``root``, as the release builds.""" + (root / "CHANGELOG.md").write_text("# Changelog\n", encoding="utf-8") + return subprocess.run( + [sys.executable, "-m", "towncrier", "build", "--draft", "--version", "9.9.9"], + cwd=root, + text=True, + capture_output=True, + check=False, + ) + + +def test_towncrier_is_set_to_stop_at_files_it_cannot_read(): + towncrier = tomllib.loads((REPO / "pyproject.toml").read_text(encoding="utf-8"))[ + "tool" + ]["towncrier"] + + assert towncrier["ignore"] == [".gitkeep"] + + +@pytest.mark.parametrize( + ("relative_path", "invalid_name"), + [ + ("changelog.d/added/4083.md", "added"), + ("changelog.d/4083.feature.md", "4083.feature.md"), + ("changelog.d/4083.Added.md", "4083.Added.md"), + ], + ids=["pull-request-4084", "unknown-type", "uppercase-type"], +) +def test_the_locked_towncrier_stops_at_a_file_it_cannot_read( + repository: Path, relative_path: str, invalid_name: str +): + _write(repository, "changelog.d/2.fixed.md") + _write(repository, relative_path) + + result = _towncrier_draft(repository) + + assert result.returncode != 0, result.stdout + assert f"Invalid news fragment name: {invalid_name}" in result.stderr + + +def test_the_locked_towncrier_builds_the_fragments_the_definition_accepts( + repository: Path, +): + _write(repository, "changelog.d/2.fixed.md", "Fix the second thing.\n") + _write( + repository, + "changelog.d/update-policyengine-bundle-6.11.1.changed.md", + "Update the bundle.\n", + ) + + result = _towncrier_draft(repository) + + assert result.returncode == 0, result.stderr + assert "### Changed\n\n- Update the bundle.\n" in result.stdout + assert "### Fixed\n\n- Fix the second thing.\n" in result.stdout diff --git a/tests/unit/test_bundle_update_pins.py b/tests/unit/test_bundle_update_pins.py index 2ccfb19e6..0f1cfd990 100644 --- a/tests/unit/test_bundle_update_pins.py +++ b/tests/unit/test_bundle_update_pins.py @@ -113,6 +113,24 @@ def test_automatic_update_changes_and_stages_both_image_pins(update_checkout): ] in calls +@pytest.mark.parametrize("target_version", [TARGET_VERSION, "4.1.0rc1", "4.1.0.post1"]) +def test_the_pull_request_adds_a_fragment_the_release_reads( + update_checkout, target_version +): + from tests.unit.changelog_fragment_support import changelog_fragments + + result, _ = run_update( + update_checkout, + LATEST_OVERRIDE=target_version, + BUNDLE_TEST_REPORTED_VERSION=target_version, + ) + + assert result.returncode == 0, result.stderr + fragment = f"changelog.d/update-policyengine-bundle-{target_version}.changed.md" + definition = changelog_fragments().FragmentDefinition.load(REPO) + assert definition.problem_with(fragment, update_checkout / fragment) is None + + @pytest.mark.parametrize("pin", ["3.9.0", "4.0.01", "4.0.0rc1"]) def test_mismatched_image_pin_fails_before_file_changes(update_checkout, pin): image = update_checkout / "docker/Dockerfile" diff --git a/tests/unit/test_changelog_fragment_check.py b/tests/unit/test_changelog_fragment_check.py new file mode 100644 index 000000000..184f8a702 --- /dev/null +++ b/tests/unit/test_changelog_fragment_check.py @@ -0,0 +1,645 @@ +"""Tests for the changelog fragment PR check (rule api.process.changelog-fragment). + +``.github/scripts/check_changelog_fragment.sh`` compares the checked-out +commit with the pull request's base commit (``BASE_SHA``) and passes the +changes in changelog.d/ to ``.github/changelog_fragments.py``. Each test builds +a temporary git repository with a master branch, holding one pending fragment +as master does between releases, and a pull request branch. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from typing import TYPE_CHECKING + +import pytest +import yaml + +from tests.unit.changelog_fragment_support import REPO, changelog_fragments + +if TYPE_CHECKING: + from pathlib import Path + +fragments_module = changelog_fragments() + +CHECK_SCRIPT = REPO / ".github/scripts/check_changelog_fragment.sh" +PR_WORKFLOW = REPO / ".github/workflows/pr.yml" +PENDING_FRAGMENT = "changelog.d/telemetry-rank-cause-copies.fixed.md" +OWN_FRAGMENT = "changelog.d/4114.fixed.md" +PULL_REQUEST_4084_FRAGMENT = "changelog.d/added/4083.md" +LOWER_FRAGMENT = "changelog.d/4200.added.md" +ALREADY_RELEASED = ( + f"::error file={LOWER_FRAGMENT}::{LOWER_FRAGMENT} is a fragment the base " + "branch has already released: a release removed it after this branch left " + "the base branch, so merging would publish it again. This happens when a " + "pull request lower in a stack merged; rebase this branch onto the base " + "branch without that pull request's commits. If it is a new note that " + "reuses the name, rename it.\n" +) +FRAGMENT_TEXT = "Report when the deployed version went live.\n" +NO_OWN_FRAGMENT = "::error::This pull request adds no changelog fragment of its own." +ON_THE_BASE_BRANCH = ( + "It is already on the base branch, so the next release fails until a pull " + "request fixes it." +) + + +def _git_environment() -> dict[str, str]: + """Return an environment that ignores the user's and system git settings.""" + return { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@example.com", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@example.com", + } + + +def _git(directory: Path, *arguments: str) -> str: + return subprocess.run( + ["git", *arguments], + cwd=directory, + env=_git_environment(), + text=True, + capture_output=True, + check=True, + ).stdout.strip() + + +def _write(repository: Path, relative_path: str, text: str = FRAGMENT_TEXT) -> None: + file_path = repository / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") + + +def _commit(repository: Path, message: str) -> str: + _git(repository, "add", "--all") + _git(repository, "commit", "--quiet", "--allow-empty", "-m", message) + return _git(repository, "rev-parse", "HEAD") + + +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a repository on a pull request branch created from master. + + Master holds the repository's pyproject.toml, changelog.d/.gitkeep and one + pending fragment from an earlier pull request. + """ + repository = tmp_path / "repository" + repository.mkdir() + _git(repository, "init", "--quiet", "--initial-branch=master") + (repository / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(repository, "changelog.d/.gitkeep", "") + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause.\n") + _write(repository, "README.md", "The Public API.\n") + _commit(repository, "Master with a pending fragment") + _git(repository, "checkout", "--quiet", "-b", "pull-request") + return repository + + +def _master(repository: Path) -> str: + return _git(repository, "rev-parse", "master") + + +def _run_check( + repository: Path, base_revision: str | None +) -> subprocess.CompletedProcess[str]: + environment = {**_git_environment(), "PYTHON": sys.executable} + environment.pop("BASE_SHA", None) + if base_revision is not None: + environment["BASE_SHA"] = base_revision + return subprocess.run( + ["bash", str(CHECK_SCRIPT)], + cwd=repository, + env=environment, + text=True, + capture_output=True, + check=False, + ) + + +def _merge_into_master(repository: Path) -> str: + """Check out a merge of the pull request into master, as GitHub's checkout does. + + GitHub checks out refs/pull//merge, a merge commit whose first parent + is the base commit and whose second is the pull request's head. + """ + _git(repository, "checkout", "--quiet", "--detach", "master") + _git( + repository, + "merge", + "--quiet", + "--no-ff", + "-m", + "Merge pull-request into master", + "pull-request", + ) + return _git(repository, "rev-parse", "HEAD") + + +def test_a_pull_request_with_no_fragment_fails_while_master_has_a_pending_one( + repository: Path, +): + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "Change the README") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert ( + "Fragments already on the base branch belong to the pull requests that " + f"added them: {PENDING_FRAGMENT}." in result.stdout + ) + assert "Add a file changelog.d/..md" in result.stdout + + +def test_a_pull_request_that_only_edits_a_pending_fragment_fails(repository: Path): + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause, edited.\n") + _commit(repository, "Edit the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_a_pull_request_that_only_deletes_a_pending_fragment_fails(repository: Path): + (repository / PENDING_FRAGMENT).unlink() + _commit(repository, "Delete the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_renaming_a_pending_fragment_does_not_add_one(repository: Path): + _git(repository, "mv", PENDING_FRAGMENT, "changelog.d/renamed.fixed.md") + _commit(repository, "Rename the pending fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert ( + f"Renaming a fragment that is already on the base branch " + f"({PENDING_FRAGMENT} to changelog.d/renamed.fixed.md) doesn't add one." + in result.stdout + ) + + +def test_a_pull_request_that_adds_a_fragment_passes(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert result.stdout == ( + f"This pull request adds 1 changelog fragment: {OWN_FRAGMENT} (fixed).\n" + ) + + +def test_a_pull_request_that_adds_a_fragment_and_edits_a_pending_one_passes( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + _write(repository, PENDING_FRAGMENT, "Copy each telemetry rank cause, edited.\n") + _commit(repository, "Add a fragment and edit the pending one") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + + +def test_moving_a_file_the_release_could_not_read_into_place_adds_a_fragment( + repository: Path, +): + _git(repository, "checkout", "--quiet", "master") + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Master with #4084's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _git( + repository, + "mv", + PULL_REQUEST_4084_FRAGMENT, + "changelog.d/report-live-since.added.md", + ) + _commit(repository, "Move #4084's fragment to the name the release reads") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert "changelog.d/report-live-since.added.md (added)" in result.stdout + + +def test_pull_request_4084s_fragment_fails_and_says_where_it_belongs( + repository: Path, +): + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Add a fragment in a folder") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ( + f"::error file={PULL_REQUEST_4084_FRAGMENT}::{PULL_REQUEST_4084_FRAGMENT} " + "is in a folder inside changelog.d/. The release reads only files " + "directly in changelog.d/ and stops at anything else there. Move it to " + "changelog.d/4083.added.md.\n" in result.stdout + ) + assert NO_OWN_FRAGMENT in result.stdout + + +@pytest.mark.parametrize( + ("relative_path", "text", "expected_problem"), + [ + ("changelog.d/4114.feature.md", FRAGMENT_TEXT, 'has the type "feature"'), + ("changelog.d/4114.md", FRAGMENT_TEXT, "is not named ..md"), + ("changelog.d/4114.fixed.md", "", "is empty."), + ("changelog.d/4114.fixed.md", " \n\n", "holds only whitespace."), + ], + ids=["unknown-type", "no-type", "empty", "whitespace-only"], +) +def test_a_pull_request_whose_only_fragment_is_unreadable_fails( + repository: Path, relative_path: str, text: str, expected_problem: str +): + _write(repository, relative_path, text) + _commit(repository, "Add an unreadable fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"::error file={relative_path}::{relative_path} " in result.stdout + assert expected_problem in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_an_unreadable_file_beside_a_valid_fragment_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _write(repository, "changelog.d/notes.md", "Notes.\n") + _commit(repository, "Add a fragment and a notes file") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert "::error file=changelog.d/notes.md::" in result.stdout + assert NO_OWN_FRAGMENT not in result.stdout + + +def test_emptying_a_pending_fragment_fails_even_with_a_fragment_of_its_own( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + _write(repository, PENDING_FRAGMENT, "") + _commit(repository, "Add a fragment and empty the pending one") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"::error file={PENDING_FRAGMENT}::{PENDING_FRAGMENT} is empty." in ( + result.stdout + ) + assert ON_THE_BASE_BRANCH not in result.stdout + + +def test_a_submodule_in_changelog_d_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _git( + repository, + "update-index", + "--add", + "--cacheinfo", + f"160000,{_master(repository)},changelog.d/vendored.added.md", + ) + _git(repository, "add", OWN_FRAGMENT) + # Commit the index as it stands: git add --all would stage the submodule's + # removal, since the working tree has no folder for it. + _git(repository, "commit", "--quiet", "-m", "Add a fragment and a submodule") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ( + "::error file=changelog.d/vendored.added.md::changelog.d/vendored.added.md " + "is an empty folder or a submodule inside changelog.d/." in result.stdout + ) + + +def test_an_unreadable_file_already_on_master_fails_and_says_so(repository: Path): + _git(repository, "checkout", "--quiet", "master") + _write(repository, PULL_REQUEST_4084_FRAGMENT) + _commit(repository, "Master with #4084's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert f"Move it to changelog.d/4083.added.md. {ON_THE_BASE_BRANCH}" in ( + result.stdout + ) + + +def test_on_the_merge_commit_only_the_pull_requests_own_changes_count( + repository: Path, +): + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "Change the README") + _git(repository, "checkout", "--quiet", "master") + _write(repository, "changelog.d/4110.added.md") + _commit(repository, "Merge another pull request, with its fragment") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + assert "changelog.d/4110.added.md" in result.stdout + + +def test_on_the_merge_commit_the_pull_requests_fragment_counts(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + _git(repository, "checkout", "--quiet", "master") + _write(repository, "changelog.d/4110.added.md") + _commit(repository, "Merge another pull request, with its fragment") + merge_commit = _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert _git(repository, "rev-parse", "HEAD^1") == _master(repository) + assert _git(repository, "rev-parse", "HEAD") == merge_commit + assert result.returncode == 0, result.stdout + assert result.stdout == ( + f"This pull request adds 1 changelog fragment: {OWN_FRAGMENT} (fixed).\n" + ) + + +def test_a_pull_request_stacked_on_another_needs_its_own_fragment( + repository: Path, +): + _write(repository, OWN_FRAGMENT) + lower_head = _commit(repository, "The lower pull request, with its fragment") + _git(repository, "checkout", "--quiet", "-b", "upper-pull-request") + _write(repository, "README.md", "The Public API, changed.\n") + _commit(repository, "The upper pull request, without a fragment") + + result = _run_check(repository, lower_head) + + assert result.returncode == 1, result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def _squash_merge_and_release_the_lower_pull_request(repository: Path) -> None: + """Leave the pull request branch on top of a lower one that merged and released. + + The lower pull request added LOWER_FRAGMENT in two commits. Master + squash-merged it, then the release commit removed every fragment. The pull + request branch, created from the lower one and not rebased, still holds the + lower pull request's commits. + """ + _git(repository, "checkout", "--quiet", "-B", "lower", "master") + _write(repository, "code.txt", "The lower change.\n") + _write(repository, LOWER_FRAGMENT, "Lower change.\n") + _commit(repository, "The lower pull request") + _write(repository, LOWER_FRAGMENT, "Lower change, reworded.\n") + _commit(repository, "Reword the lower pull request's fragment") + _git(repository, "checkout", "--quiet", "-B", "pull-request") + _write(repository, "upper.txt", "The upper change.\n") + _commit(repository, "The upper pull request, without a fragment") + _git(repository, "checkout", "--quiet", "master") + _git(repository, "merge", "--quiet", "--squash", "lower") + _commit(repository, "Squash-merge the lower pull request") + _git(repository, "rm", "--quiet", PENDING_FRAGMENT, LOWER_FRAGMENT) + _commit(repository, "Update PolicyEngine API") + _git(repository, "checkout", "--quiet", "pull-request") + + +def test_on_the_merge_commit_a_fragment_already_released_does_not_count( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert (repository / LOWER_FRAGMENT).is_file() + assert result.returncode == 1, result.stdout + assert ALREADY_RELEASED in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_at_the_pull_requests_head_a_fragment_already_released_does_not_count( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert ALREADY_RELEASED in result.stdout + assert NO_OWN_FRAGMENT in result.stdout + + +def test_a_released_fragment_beside_the_pull_requests_own_still_fails( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add the upper pull request's fragment") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 1, result.stdout + assert result.stdout == ALREADY_RELEASED + + +def test_a_new_fragment_that_reuses_a_released_fragments_name_counts( + repository: Path, +): + _write(repository, "changelog.d/fix-typo.fixed.md", "Fix the household form.\n") + _commit(repository, "This pull request, with its fragment") + _git(repository, "checkout", "--quiet", "-b", "other", "master") + _write(repository, "changelog.d/fix-typo.fixed.md", "Fix the report title.\n") + _commit(repository, "Another pull request, with a fragment of the same name") + _git(repository, "checkout", "--quiet", "master") + _git(repository, "merge", "--quiet", "--no-ff", "-m", "Merge the other", "other") + _git(repository, "rm", "--quiet", PENDING_FRAGMENT, "changelog.d/fix-typo.fixed.md") + _commit(repository, "Update PolicyEngine API") + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + assert result.returncode == 0, result.stdout + assert result.stdout == ( + "This pull request adds 1 changelog fragment: " + "changelog.d/fix-typo.fixed.md (fixed).\n" + ) + + +def test_a_released_fragment_is_not_listed_as_on_the_base_branch( + repository: Path, +): + _squash_merge_and_release_the_lower_pull_request(repository) + _merge_into_master(repository) + + result = _run_check(repository, _master(repository)) + + no_own_fragment = next( + line for line in result.stdout.splitlines() if NO_OWN_FRAGMENT in line + ) + assert "Fragments already on the base branch" not in no_own_fragment + + +def test_a_shallow_clone_fails_and_says_to_fetch_the_history( + repository: Path, tmp_path: Path +): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + _merge_into_master(repository) + shallow_clone = tmp_path / "shallow" + _git( + tmp_path, + "clone", + "--quiet", + "--depth=1", + f"file://{repository}", + str(shallow_clone), + ) + + result = _run_check(shallow_clone, _master(repository)) + + assert result.returncode == 1, result.stdout + assert result.stdout.startswith("::error::This clone is shallow") + assert "fetch-depth: 0" in result.stdout + + +def test_a_base_commit_the_clone_lacks_fails(repository: Path): + _write(repository, OWN_FRAGMENT) + _commit(repository, "Add a fragment") + + result = _run_check(repository, "0" * 40) + + assert result.returncode == 1, result.stdout + assert f"::error::Could not find a common ancestor of {'0' * 40} and HEAD" in ( + result.stdout + ) + + +def test_the_base_commit_is_required(repository: Path): + result = _run_check(repository, None) + + assert result.returncode != 0 + assert "BASE_SHA is required" in result.stderr + + +@pytest.mark.parametrize( + ("name_status", "expected_added_paths"), + [ + (b"A\0changelog.d/a.added.md\0", {"changelog.d/a.added.md"}), + (b"M\0changelog.d/a.added.md\0D\0changelog.d/b.fixed.md\0", set()), + (b"R100\0changelog.d/a.added.md\0changelog.d/b.added.md\0", set()), + ( + b"R087\0changelog.d/added/4083.md\0changelog.d/4083.added.md\0", + {"changelog.d/4083.added.md"}, + ), + ( + b"C100\0changelog.d/a.added.md\0changelog.d/b.added.md\0", + {"changelog.d/b.added.md"}, + ), + (b"T\0changelog.d/a.added.md\0", set()), + ], + ids=[ + "added", + "modified-and-deleted", + "renamed-fragment", + "renamed-unreadable-file", + "copied", + "type-changed", + ], +) +def test_the_paths_a_pull_request_adds( + name_status: bytes, expected_added_paths: set[str] +): + definition = fragments_module.FragmentDefinition.load(REPO) + changes = fragments_module.parse_name_status(name_status) + + assert definition.added_paths(changes) == expected_added_paths + + +def test_the_released_fragments_are_read_from_git_logs_raw_deletions(): + removed_blob = "212c395e8d8e25de20d96d1d6f24f6a42cdecd1c" + other_blob = "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391" + raw_log = ( + f":100644 000000 {removed_blob} {'0' * 40} D\0changelog.d/a.added.md\0" + f"\n:100644 000000 {other_blob} {'0' * 40} D\0changelog.d/b.fixed.md\0" + ).encode() + + assert fragments_module.parse_removed_blobs(raw_log) == { + "changelog.d/a.added.md": {removed_blob}, + "changelog.d/b.fixed.md": {other_blob}, + } + + +def test_a_files_git_blob_id_is_computed_as_git_hash_object_does(tmp_path: Path): + fragment = tmp_path / "4114.fixed.md" + fragment.write_bytes(b"A change.\n") + expected_blob = _git(tmp_path, "hash-object", str(fragment)) + + assert fragments_module.git_blob_id(fragment, len(expected_blob)) == expected_blob + + +def test_name_status_output_that_ends_mid_record_is_refused(): + with pytest.raises(ValueError, match="ends in the middle of a change"): + fragments_module.parse_name_status(b"R100\0changelog.d/a.added.md\0") + + +def test_annotation_properties_escape_the_characters_the_runner_reads(): + assert fragments_module.annotation_property("changelog.d/a,b:c%.md") == ( + "changelog.d/a%2Cb%3Ac%25.md" + ) + assert fragments_module.annotation_message("line one\nline two 100%") == ( + "line one%0Aline two 100%25" + ) + + +def _check_changelog_job() -> dict: + workflow = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8")) + return workflow["jobs"]["check-changelog"] + + +def test_the_job_checks_out_the_history_and_passes_the_base_commit(): + job = _check_changelog_job() + checkout_step, python_step, check_step = job["steps"] + + assert checkout_step["uses"] == "actions/checkout@v4" + assert checkout_step["with"] == {"fetch-depth": 0} + assert python_step["uses"] == "actions/setup-python@v5" + assert python_step["with"] == {"python-version": "3.12"} + assert check_step["env"] == { + "BASE_SHA": "${{ github.event.pull_request.base.sha }}" + } + assert check_step["run"] == "bash .github/scripts/check_changelog_fragment.sh" + assert job["permissions"] == {"contents": "read"} + + +def test_the_job_keeps_the_stack_condition_and_is_needed_by_ci_complete(): + workflow = yaml.safe_load(PR_WORKFLOW.read_text(encoding="utf-8")) + job = workflow["jobs"]["check-changelog"] + + assert job["if"] == ( + "${{ !github.event.pull_request.stack || " + "github.event.pull_request.stack.base.ref == " + "github.event.pull_request.base.ref }}" + ) + assert "check-changelog" in workflow["jobs"]["ci-complete"]["needs"] diff --git a/tests/unit/test_changelog_fragments.py b/tests/unit/test_changelog_fragments.py new file mode 100644 index 000000000..34bd0cd24 --- /dev/null +++ b/tests/unit/test_changelog_fragments.py @@ -0,0 +1,462 @@ +"""Tests for .github/changelog_fragments.py: what a changelog fragment is. + +The PR check and the release's version bump both read fragments through +``FragmentDefinition`` (rule api.process.changelog-fragment), so these cases +pin down what both of them accept. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from tests.unit.changelog_fragment_support import REPO, changelog_fragments + +if TYPE_CHECKING: + from pathlib import Path + +fragments_module = changelog_fragments() + +CONFIGURED_TYPES = ("breaking", "added", "changed", "fixed", "removed") +LISTED_TYPES = "breaking, added, changed, fixed or removed" +RULE_FIXTURES = REPO / "rules/fixtures/api.process.changelog-fragment" +# The fragment #4084 added, and the text #4106 moved to changelog.d/. +PULL_REQUEST_4084_FRAGMENT = "changelog.d/added/4083.md" +FRAGMENT_TEXT = "Report when the deployed version went live.\n" +EMPTY_ENTRY = ( + "Write a sentence or two in it saying what changed: Towncrier would " + "publish an empty entry." +) +FORM_TERMS = ( + f" is {LISTED_TYPES}, and is letters and digits, in groups " + "joined by single dots, hyphens or underscores" +) +INVISIBLE_ONLY = ( + "holds no visible text, only invisible characters such as zero-width spaces." +) +GENERIC_RENAME = f"Rename it to changelog.d/..md, where {FORM_TERMS}." + + +def _definition(): + return fragments_module.FragmentDefinition.load(REPO) + + +def _write(root: Path, relative_path: str, text: str) -> Path: + file_path = root / relative_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text(text, encoding="utf-8") + return file_path + + +@pytest.fixture +def repository(tmp_path: Path) -> Path: + """Return a root holding the repository's pyproject.toml and changelog.d/.""" + (tmp_path / "pyproject.toml").write_text( + (REPO / "pyproject.toml").read_text(encoding="utf-8"), encoding="utf-8" + ) + _write(tmp_path, "changelog.d/.gitkeep", "") + return tmp_path + + +def _towncrier_config(directory: str, *type_directories: str) -> str: + type_tables = "".join( + f'\n[[tool.towncrier.type]]\ndirectory = "{type_directory}"\n' + f'name = "{type_directory.title()}"\n' + for type_directory in type_directories + ) + return f'[tool.towncrier]\ndirectory = "{directory}"\n{type_tables}' + + +def test_the_directory_and_types_come_from_tool_towncrier_in_pyproject(): + definition = _definition() + + assert definition.directory == "changelog.d" + assert definition.types == CONFIGURED_TYPES + + +def test_the_types_are_read_from_the_given_pyproject(tmp_path: Path): + (tmp_path / "pyproject.toml").write_text( + _towncrier_config("notes", "added", "security"), encoding="utf-8" + ) + + definition = fragments_module.FragmentDefinition.load(tmp_path) + + assert definition.directory == "notes" + assert definition.types == ("added", "security") + + +@pytest.mark.parametrize( + ("config", "expected_error"), + [ + ('[project]\nname = "x"\n', "Could not read the fragment directory"), + ('[tool.towncrier]\ndirectory = "changelog.d"\n', "Could not read"), + ( + '[tool.towncrier]\ndirectory = "changelog.d"\ntype = []\n', + "must list each fragment type once", + ), + ( + _towncrier_config("changelog.d", "added", "added"), + "must list each fragment type once", + ), + (_towncrier_config("notes/changelog.d", "added"), "is not one folder"), + (_towncrier_config("../changelog.d", "added"), "is not one folder"), + (_towncrier_config("changelog.d", "Added"), 'type "Added"'), + (_towncrier_config("changelog.d", "md"), 'type "md"'), + ], + ids=[ + "no-towncrier-table", + "no-types", + "empty-type-list", + "duplicate-type", + "nested-directory", + "parent-directory", + "uppercase-type", + "extension-as-type", + ], +) +def test_load_refuses_a_configuration_the_release_cannot_follow( + tmp_path: Path, config: str, expected_error: str +): + (tmp_path / "pyproject.toml").write_text(config, encoding="utf-8") + + with pytest.raises(fragments_module.ConfigurationError, match=expected_error): + fragments_module.FragmentDefinition.load(tmp_path) + + +@pytest.mark.parametrize( + ("file_name", "expected_name", "expected_type"), + [ + ("4083.added.md", "4083", "added"), + ("report-live-since.added.md", "report-live-since", "added"), + ("relock-dependencies.changed.md", "relock-dependencies", "changed"), + ( + "update-policyengine-bundle-6.11.1.changed.md", + "update-policyengine-bundle-6.11.1", + "changed", + ), + ("v2_reports.breaking.md", "v2_reports", "breaking"), + ("fixed.removed.md", "fixed", "removed"), + ], +) +def test_a_file_named_name_type_md_directly_in_changelog_d_is_a_fragment( + repository: Path, file_name: str, expected_name: str, expected_type: str +): + _write(repository, f"changelog.d/{file_name}", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.problems == {} + assert scan.fragments == ( + fragments_module.Fragment( + path=f"changelog.d/{file_name}", + name=expected_name, + fragment_type=expected_type, + ), + ) + + +def test_pull_request_4084s_fragment_in_a_type_folder_is_not_a_fragment( + repository: Path, +): + _write(repository, PULL_REQUEST_4084_FRAGMENT, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == { + PULL_REQUEST_4084_FRAGMENT: ( + "changelog.d/added/4083.md is in a folder inside changelog.d/. The " + "release reads only files directly in changelog.d/ and stops at " + "anything else there. Move it to changelog.d/4083.added.md." + ) + } + + +@pytest.mark.parametrize( + ("relative_path", "expected_problem"), + [ + ( + "changelog.d/fixed/4083.fixed.md", + "is in a folder inside changelog.d/. The release reads only files " + "directly in changelog.d/ and stops at anything else there. Move it " + "to changelog.d/4083.fixed.md.", + ), + ( + "changelog.d/notes/today.txt", + f"Move it to changelog.d/..md, where {FORM_TERMS}, or " + "delete it.", + ), + ( + "changelog.d/added/.gitkeep", + "changelog.d/added/.gitkeep is in a folder inside changelog.d/.", + ), + ], +) +def test_every_file_in_a_folder_inside_changelog_d_is_a_problem( + repository: Path, relative_path: str, expected_problem: str +): + _write(repository, relative_path, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert list(scan.problems) == [relative_path] + assert expected_problem in scan.problems[relative_path] + + +def test_empty_folders_are_listed_apart_from_the_problems(repository: Path): + """Git can't commit an empty folder, so one is in a checkout only by accident. + + A local ``git mv`` out of changelog.d/added/ leaves one behind; in a fresh + checkout, only a submodule makes one. The version bump treats each as a + problem, as Towncrier does; the PR check reports one only if the pull + request changed that path. + """ + (repository / "changelog.d/added").mkdir() + (repository / "changelog.d/notes/drafts").mkdir(parents=True) + + definition = _definition() + scan = definition.scan(repository) + + assert scan.problems == {} + assert scan.empty_folders == ("changelog.d/added", "changelog.d/notes/drafts") + assert definition.empty_folder_problem("changelog.d/added") == ( + "changelog.d/added is an empty folder or a submodule inside changelog.d/. " + "The release reads only files directly in changelog.d/ and stops at " + "anything else there. Delete it." + ) + + +@pytest.mark.parametrize( + ("file_name", "expected_problem"), + [ + ( + "4083.feature.md", + 'changelog.d/4083.feature.md has the type "feature", which is not ' + "one of the types in [tool.towncrier] in pyproject.toml: " + f"{LISTED_TYPES}. Rename it to changelog.d/4083..md with one " + "of those types.", + ), + ( + "4083.Added.md", + 'changelog.d/4083.Added.md has the type "Added", but types are ' + "lowercase. Rename it to changelog.d/4083.added.md.", + ), + ( + "4083.added.extra.md", + 'changelog.d/4083.added.extra.md has the type "extra", which is not ' + "one of the types", + ), + ], +) +def test_a_fragment_with_an_unknown_type_is_a_problem( + repository: Path, file_name: str, expected_problem: str +): + _write(repository, f"changelog.d/{file_name}", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert expected_problem in scan.problems[f"changelog.d/{file_name}"] + + +@pytest.mark.parametrize( + ("file_name", "expected_fix"), + [ + ("notes.md", GENERIC_RENAME), + ("4083.added", "Rename it to changelog.d/4083.added.md."), + ("4083.added.rst", "Rename it to changelog.d/4083.added.md."), + ("4083.added.MD", "Rename it to changelog.d/4083.added.md."), + ("4083.added.md ", "Rename it to changelog.d/4083.added.md."), + (".added.md", GENERIC_RENAME), + ("added.md", GENERIC_RENAME), + ("fix login.added.md", GENERIC_RENAME), + ("4083..added.md", GENERIC_RENAME), + ("-4083.added.md", GENERIC_RENAME), + ], +) +def test_a_file_not_named_name_type_md_is_a_problem( + repository: Path, file_name: str, expected_fix: str +): + relative_path = f"changelog.d/{file_name}" + _write(repository, relative_path, FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert scan.fragments == () + problem = scan.problems[relative_path] + assert problem.startswith( + f"{relative_path} is not named ..md: its file name is " + f"{file_name!r}. The release can't read it." + ) + assert problem.endswith(expected_fix) + + +@pytest.mark.parametrize( + ("text", "expected_problem"), + [ + ("", f"changelog.d/4083.added.md is empty. {EMPTY_ENTRY}"), + ( + " \n\t\n", + f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", + ), + ( + "\ufeff\n", + f"changelog.d/4083.added.md holds only whitespace. {EMPTY_ENTRY}", + ), + ("\u200b\n", f"changelog.d/4083.added.md {INVISIBLE_ONLY} {EMPTY_ENTRY}"), + ( + "\ufeff\u2060\u200d \u00ad\n", + f"changelog.d/4083.added.md {INVISIBLE_ONLY} {EMPTY_ENTRY}", + ), + ], + ids=[ + "empty", + "whitespace-only", + "byte-order-mark-only", + "zero-width-space-only", + "format-characters-only", + ], +) +def test_a_fragment_without_text_is_a_problem( + repository: Path, text: str, expected_problem: str +): + _write(repository, "changelog.d/4083.added.md", text) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == {"changelog.d/4083.added.md": expected_problem} + + +@pytest.mark.parametrize( + "text", ["\u200bFixed.\n", "\u00e9\n", "\u2014\n", "7\n", "\u4e2d\u6587\n"] +) +def test_a_fragment_with_any_visible_character_is_read(repository: Path, text: str): + _write(repository, "changelog.d/4083.added.md", text) + + scan = _definition().scan(repository) + + assert scan.problems == {} + assert [fragment.path for fragment in scan.fragments] == [ + "changelog.d/4083.added.md" + ] + + +def test_a_fragment_that_is_not_utf8_is_a_problem(repository: Path): + (repository / "changelog.d/4083.added.md").write_bytes(b"caf\xe9\n") + + scan = _definition().scan(repository) + + assert scan.problems == { + "changelog.d/4083.added.md": ( + "changelog.d/4083.added.md is not UTF-8 text. Save it as UTF-8." + ) + } + + +@pytest.mark.parametrize( + "link_target", ["../pyproject.toml", "missing.md"], ids=["file", "dangling"] +) +def test_a_symbolic_link_is_not_a_fragment(repository: Path, link_target: str): + (repository / "changelog.d/4083.added.md").symlink_to(link_target) + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert scan.problems == { + "changelog.d/4083.added.md": ( + "changelog.d/4083.added.md is a symbolic link. A fragment is a " + "regular file that holds its text; replace the link with one." + ) + } + + +def test_a_symbolic_link_to_a_folder_is_not_a_fragment(repository: Path): + (repository / "elsewhere").mkdir() + _write(repository, "elsewhere/4083.added.md", FRAGMENT_TEXT) + (repository / "changelog.d/added").symlink_to("../elsewhere") + + scan = _definition().scan(repository) + + assert scan.fragments == () + assert list(scan.problems) == ["changelog.d/added"] + assert "is a symbolic link" in scan.problems["changelog.d/added"] + + +def test_gitkeep_is_the_only_other_file_allowed_and_must_be_a_regular_file( + repository: Path, +): + _write(repository, "changelog.d/4083.added.md", FRAGMENT_TEXT) + + assert _definition().scan(repository).problems == {} + + (repository / "changelog.d/.gitkeep").unlink() + (repository / "changelog.d/.gitkeep").symlink_to("4083.added.md") + + assert list(_definition().scan(repository).problems) == ["changelog.d/.gitkeep"] + + +def test_a_missing_fragment_directory_holds_no_fragments(tmp_path: Path): + scan = _definition().scan(tmp_path) + + assert scan.fragments == () + assert scan.problems == {} + + +def test_a_fragment_directory_that_is_a_file_is_a_problem(tmp_path: Path): + (tmp_path / "changelog.d").write_text(FRAGMENT_TEXT, encoding="utf-8") + + scan = _definition().scan(tmp_path) + + assert scan.fragments == () + assert scan.problems == { + "changelog.d": ( + "changelog.d is not a folder. The release reads fragments from the " + "folder changelog.d/; make it one." + ) + } + + +def test_scan_lists_fragments_and_problems_in_path_order(repository: Path): + _write(repository, "changelog.d/b.fixed.md", FRAGMENT_TEXT) + _write(repository, "changelog.d/a.added.md", FRAGMENT_TEXT) + _write(repository, PULL_REQUEST_4084_FRAGMENT, FRAGMENT_TEXT) + _write(repository, "changelog.d/z.feature.md", FRAGMENT_TEXT) + + scan = _definition().scan(repository) + + assert [fragment.path for fragment in scan.fragments] == [ + "changelog.d/a.added.md", + "changelog.d/b.fixed.md", + ] + assert list(scan.problems) == [ + PULL_REQUEST_4084_FRAGMENT, + "changelog.d/z.feature.md", + ] + + +def test_the_accepted_form_names_the_directory_types_and_documentation(): + assert _definition().accepted_form() == ( + "Add a file changelog.d/..md, directly in changelog.d/, " + "that says in a sentence or two what changed for people who use or " + f"run the Public API. {FORM_TERMS}, such as an issue number. See " + "docs/engineering/skills/github-prs.md#changelog-fragments (rule " + "api.process.changelog-fragment)." + ) + + +def test_rule_proof_changelog_fragment(): + """The definition rejects #4084's fragment and accepts #4106's rename of it.""" + definition = _definition() + + bad_scan = definition.scan(RULE_FIXTURES / "bad") + good_scan = definition.scan(RULE_FIXTURES / "good") + + assert bad_scan.fragments == () + assert list(bad_scan.problems) == [PULL_REQUEST_4084_FRAGMENT] + assert good_scan.problems == {} + assert [fragment.path for fragment in good_scan.fragments] == [ + "changelog.d/report-live-since.added.md" + ] diff --git a/tests/unit/test_relock_pull_request_script.py b/tests/unit/test_relock_pull_request_script.py index dc6241d20..3d7b49c10 100644 --- a/tests/unit/test_relock_pull_request_script.py +++ b/tests/unit/test_relock_pull_request_script.py @@ -154,6 +154,23 @@ def test_opens_a_pull_request_naming_a_new_tracking_issue(checkout: Path) -> Non assert not _calls_to(calls, "pr", "edit") +def test_the_pull_request_adds_a_fragment_the_release_reads(checkout: Path) -> None: + from tests.unit.changelog_fragment_support import changelog_fragments + + result, _ = _open_pull_request(checkout) + + assert result.returncode == 0, result.stderr + pushed = _remote_branch(checkout).split()[0] + added_paths = _git( + checkout, "diff", "--name-only", "--diff-filter=A", "origin/master", pushed + ).splitlines() + assert added_paths == [FRAGMENT] + pushed_fragment = checkout.parent / "pushed-fragment.md" + pushed_fragment.write_text(_git(checkout, "show", f"{pushed}:{FRAGMENT}") + "\n") + definition = changelog_fragments().FragmentDefinition.load(REPO) + assert definition.problem_with(FRAGMENT, pushed_fragment) is None + + def test_reuses_the_open_tracking_issue(checkout: Path) -> None: result, calls = _open_pull_request(checkout, RELOCK_TEST_OPEN_ISSUE="41")