From 4ff815045a269618e8fd8e3e896e524a506b6790 Mon Sep 17 00:00:00 2001 From: ivan Date: Wed, 9 Sep 2026 15:19:05 -0700 Subject: [PATCH 1/7] add junit option --- README.md | 89 ++++++++++++++++++++++++++++++++++++++ scripts/post-release.sh | 14 +++--- scripts/prepare-release.sh | 26 ++++++----- tools/cli/main.go | 63 +++++++++++++++++++++++++++ 4 files changed, 175 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 75a8930..d515389 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better - [Quiet Mode](#quiet-mode) - [Hunk Filters](#hunk-filters) - [CLI Tool](#cli-tool) +- [JUnit Owners Action](#junit-owners-action) - [Contributing](#contributing) - [Future Features](#future-features) @@ -462,6 +463,94 @@ Available subcommands are: * `unowned` to check for unowned files * `owner` to check who owns a specific file or list of files * `validate` to check for typos in a `.codeowners` file +* `map` to generate a JSON ownership map of the entire repository +* `junit` to annotate JUnit XML test reports with code owners + +### Annotating test reports + +`junit` writes the owners of each test's source file onto its `` element: + +```bash +codeowners-cli junit --in-place junit.xml +``` + +```xml + +``` + +This lets whatever consumes the report downstream — a test analytics service, a dashboard, a +flaky-test tracker — group results by the team that owns the test. + +Each testcase is traced back to a file in two ways. When the framework records a `file` +attribute (jest-junit's [`addFileAttribute`](https://github.com/jest-community/jest-junit#configuration), +among others) that path is used. Otherwise `classname` is read as a dotted module path, with +trailing segments trimmed until a real file is found, which is what pytest emits for both +module-level tests (`abuse.tests.test_abuse`) and class-based ones +(`abuse.tests.test_abuse.TestAbuse`). + +### Report types + +`--type` names the framework that produced the report, which selects the right strategy and, +just as importantly, skips the wrong one: + +`--type` is required, because there is no reliable way to tell the frameworks apart from the +report alone and guessing wrong misattributes tests. + +| Type | `file` attribute | `classname` as a path | Extensions tried | Writes `file` | +|------|------------------|-----------------------|------------------|---------------| +| `pytest` | yes | yes | `.py` | **yes** | +| `jest` | yes | **no** | — | no | + +`pytest` writes the resolved path back to `file` because pytest omits the attribute entirely +under its default `xunit2` family, so the write is purely additive. `jest` does not, because +overwriting a path the framework already set would change the meaning of a field its consumers +may rely on. + +`jest` also refuses to read `classname` as a path, because jest puts the text of the describe +block there. A block named something like `chatconnection.reconnectlimiter` looks exactly like a +module path and could otherwise resolve to an unrelated file. + +Reports that name files relative to a subdirectory rather than the repository root — as jest +does in a monorepo, where paths are relative to the package — need `--prefix`: + +```bash +codeowners-cli junit --in-place --type jest --prefix frontend/react frontend/junit-react.xml +``` + +Useful options: + +| Option | Purpose | +|--------|---------| +| `--in-place`, `-i` | Rewrite the report in place instead of writing to stdout | +| `--type`, `-t` | **Required.** Framework that produced the report: `pytest` or `jest` | +| `--prefix`, `-p` | Path prefix for reports that name files relative to a subdirectory | +| `--attribute`, `-a` | Attribute to write the owners to (default `codeowners`) | + +Testcases that cannot be resolved, and files with no owner, are left untouched. + +## JUnit Owners Action + +The `junit` subcommand is also packaged as an action, so annotating a report in CI does not +require installing the CLI yourself. Add it between the step that runs your tests and the step +that uploads the report: + +```yaml +- name: 'Annotate test results with code owners' + uses: multimediallc/codeowners-plus/actions/junit-owners@v1.11.0 + with: + path: junit.xml + type: pytest +``` + +| Input | Default | Purpose | +|-------|---------|---------| +| `path` | *required* | Report(s) to annotate; separate several with whitespace or commas | +| `type` | *required* | Framework that produced the report: `pytest` or `jest` | +| `root` | `.` | Path to the Git repository the reports belong to | +| `prefix` | `''` | Path prefix for reports that name files relative to a subdirectory | +| `attribute` | `codeowners` | Attribute to write the owners to | + ## Contributing diff --git a/scripts/post-release.sh b/scripts/post-release.sh index 9061f37..213b747 100755 --- a/scripts/post-release.sh +++ b/scripts/post-release.sh @@ -2,7 +2,7 @@ set -eu -ACTIONS_FILE="action.yml" +ACTIONS_FILES=("action.yml" "actions/junit-owners/action.yml") CLI_TOOL_FILE="tools/cli/main.go" README_FILE="README.md" @@ -39,25 +39,27 @@ else git checkout -b "${BRANCH_NAME}" fi -echo "Updating ${ACTIONS_FILE}, ${CLI_TOOL_FILE}, and ${README_FILE}..." +echo "Updating ${ACTIONS_FILES[*]}, ${CLI_TOOL_FILE}, and ${README_FILE}..." # sed -i works differently on macOS and Linux. # For GNU sed (Linux), -i without an argument is fine. # For BSD sed (macOS), -i requires an argument (even if empty string for no backup). if sed --version 2>/dev/null | grep -q GNU; then # GNU sed - sed -i "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: ''|g" "${ACTIONS_FILE}" + sed -i "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: ''|g" "${ACTIONS_FILES[@]}" sed -i "s|Version: .*|Version: \"${DEV_TAG}\",|g" "${CLI_TOOL_FILE}" sed -i "s|codeowners-plus@.*|codeowners-plus@${VERSION_TAG}|g" "${README_FILE}" + sed -i "s|codeowners-plus/actions/\([a-z-]*\)@.*|codeowners-plus/actions/\1@${VERSION_TAG}|g" "${README_FILE}" else # BSD sed (macOS) - sed -i '' "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: ''|g" "${ACTIONS_FILE}" + sed -i '' "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: ''|g" "${ACTIONS_FILES[@]}" sed -i '' "s|Version: .*|Version: \"${DEV_TAG}\",|g" "${CLI_TOOL_FILE}" sed -i '' "s|codeowners-plus@.*|codeowners-plus@${VERSION_TAG}|g" "${README_FILE}" + sed -i '' "s|codeowners-plus/actions/\([a-z-]*\)@.*|codeowners-plus/actions/\1@${VERSION_TAG}|g" "${README_FILE}" fi gofmt -w tools/cli -echo "${ACTIONS_FILE}, ${CLI_TOOL_FILE}, and ${README_FILE} updated." +echo "${ACTIONS_FILES[*]}, ${CLI_TOOL_FILE}, and ${README_FILE} updated." echo "Committing changes..." -git add "${ACTIONS_FILE}" "${CLI_TOOL_FILE}" "${README_FILE}" +git add "${ACTIONS_FILES[@]}" "${CLI_TOOL_FILE}" "${README_FILE}" git commit -m "${VERSION_TAG}" echo "--- Post release process completed successfully! ---" diff --git a/scripts/prepare-release.sh b/scripts/prepare-release.sh index 953e78d..aec5849 100755 --- a/scripts/prepare-release.sh +++ b/scripts/prepare-release.sh @@ -2,7 +2,7 @@ set -eu -ACTIONS_FILE="action.yml" +ACTIONS_FILES=("action.yml" "actions/junit-owners/action.yml") CLI_TOOL_FILE="tools/cli/main.go" README_FILE="README.md" @@ -11,7 +11,7 @@ function usage() { echo "Example: $0 1.2.3" echo " This script will:" echo " 1. Create a new branch called 'release/v1.2.3'." - echo " 2. Update '${ACTIONS_FILE}', ${CLI_TOOL_FILE}, and ${README_FILE} to reference the new version." + echo " 2. Update ${ACTIONS_FILES[*]}, ${CLI_TOOL_FILE}, and ${README_FILE} to reference the new version." echo " 3. Commit the changes." echo " 4. Create a tag called 'v1.2.3'." exit 1 @@ -53,10 +53,12 @@ echo "--- Starting release process for version ${SEMANTIC_VERSION} ---" check_git_clean -if [ ! -f "${ACTIONS_FILE}" ]; then - echo "Error: ${ACTIONS_FILE} not found in the current directory. Make sure you are running this from the root." - exit 1 -fi +for actions_file in "${ACTIONS_FILES[@]}"; do + if [ ! -f "${actions_file}" ]; then + echo "Error: ${actions_file} not found. Make sure you are running this from the root." + exit 1 + fi +done echo "Creating branch '${BRANCH_NAME}'..." if git rev-parse --verify "${BRANCH_NAME}" >/dev/null 2>&1; then @@ -66,25 +68,27 @@ else git checkout -b "${BRANCH_NAME}" fi -echo "Updating ${ACTIONS_FILE}, ${CLI_TOOL_FILE}, and ${README_FILE} to replace 'latest' or old tag with '${VERSION_TAG}'..." +echo "Updating ${ACTIONS_FILES[*]}, ${CLI_TOOL_FILE}, and ${README_FILE} to replace 'latest' or old tag with '${VERSION_TAG}'..." # sed -i works differently on macOS and Linux. # For GNU sed (Linux), -i without an argument is fine. # For BSD sed (macOS), -i requires an argument (even if empty string for no backup). if sed --version 2>/dev/null | grep -q GNU; then # GNU sed - sed -i "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: '${VERSION_TAG}'|g" "${ACTIONS_FILE}" + sed -i "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: '${VERSION_TAG}'|g" "${ACTIONS_FILES[@]}" sed -i "s|Version: .*|Version: \"${VERSION_TAG}\",|g" "${CLI_TOOL_FILE}" sed -i "s|codeowners-plus@.*|codeowners-plus@${VERSION_TAG}|g" "${README_FILE}" + sed -i "s|codeowners-plus/actions/\([a-z-]*\)@.*|codeowners-plus/actions/\1@${VERSION_TAG}|g" "${README_FILE}" else # BSD sed (macOS) - sed -i '' "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: '${VERSION_TAG}'|g" "${ACTIONS_FILE}" + sed -i '' "s|RELEASE_VERSION: '.*'|RELEASE_VERSION: '${VERSION_TAG}'|g" "${ACTIONS_FILES[@]}" sed -i '' "s|Version: .*|Version: \"${VERSION_TAG}\",|g" "${CLI_TOOL_FILE}" sed -i '' "s|codeowners-plus@.*|codeowners-plus@${VERSION_TAG}|g" "${README_FILE}" + sed -i '' "s|codeowners-plus/actions/\([a-z-]*\)@.*|codeowners-plus/actions/\1@${VERSION_TAG}|g" "${README_FILE}" fi gofmt -w tools/cli -echo "${ACTIONS_FILE}, ${CLI_TOOL_FILE}, and ${README_FILE} updated." +echo "${ACTIONS_FILES[*]}, ${CLI_TOOL_FILE}, and ${README_FILE} updated." echo "Committing changes..." -git add "${ACTIONS_FILE}" "${CLI_TOOL_FILE}" "${README_FILE}" +git add "${ACTIONS_FILES[@]}" "${CLI_TOOL_FILE}" "${README_FILE}" git commit -m "${VERSION_TAG}" echo "Creating tag '${VERSION_TAG}'..." diff --git a/tools/cli/main.go b/tools/cli/main.go index a29440c..c9a9e29 100644 --- a/tools/cli/main.go +++ b/tools/cli/main.go @@ -210,6 +210,69 @@ func main() { return generateOwnershipMap(repo, mapBy) }, }, + { + Name: "junit", + Aliases: []string{"j"}, + Usage: "Annotate JUnit XML reports with code owners", + UsageText: "codeowners-cli junit [options] [report2.xml]...\n or: cat reports.txt | codeowners-cli junit [options]", + Description: "Annotate JUnit XML reports with code owners. Each is traced back to the file that defines it and the owners of that file are written onto the element as an attribute, so that whatever consumes the report can group test results by ownership. Frameworks identify the file behind a test differently, so --type names the one that produced the report: it selects the right strategy and skips the wrong one.", + Flags: []cli.Flag{ + &cli.StringFlag{ + Name: "root", + Aliases: []string{"r", "repo"}, + Value: "./", + Usage: "Path to local Git repo", + Destination: &repo, + }, + &cli.StringFlag{ + Name: "type", + Aliases: []string{"t"}, + Required: true, + Usage: "Framework that produced the report. Allowed values are: pytest and jest", + }, + &cli.StringFlag{ + Name: "prefix", + Aliases: []string{"p"}, + Value: "", + Usage: "Path prefix to prepend to test file paths, for reports that name files relative to a subdirectory", + }, + &cli.StringFlag{ + Name: "attribute", + Aliases: []string{"a"}, + Value: "codeowners", + Usage: "Attribute to write the owners to (a matching `Count` attribute is written alongside it)", + }, + &cli.BoolFlag{ + Name: "in-place", + Aliases: []string{"i"}, + Value: false, + Usage: "Rewrite the reports in place instead of writing to stdout", + }, + }, + Action: func(ctx context.Context, cmd *cli.Command) error { + targets, err := getTargets(cmd) + if err != nil { + return err + } + + if len(targets) == 0 { + return fmt.Errorf("no target reports provided (either as arguments or from stdin)") + } + + reportType, err := validateReportType(cmd.String("type")) + if err != nil { + return err + } + + return annotateJUnit(targets, junitOpts{ + root: repo, + prefix: cmd.String("prefix"), + attribute: cmd.String("attribute"), + reportType: reportType, + inPlace: cmd.Bool("in-place"), + }) + }, + }, }, } From 98ff8897997b915b3ed73c0caf5f7315dc0d1c4b Mon Sep 17 00:00:00 2001 From: ivan Date: Wed, 9 Sep 2026 15:30:53 -0700 Subject: [PATCH 2/7] add junit files --- README.md | 2 +- actions/junit-owners/action.yml | 127 ++++++++++ scripts/install-cli.sh | 102 ++++++++ tools/cli/junit.go | 326 +++++++++++++++++++++++++ tools/cli/junit_test.go | 406 ++++++++++++++++++++++++++++++++ 5 files changed, 962 insertions(+), 1 deletion(-) create mode 100644 actions/junit-owners/action.yml create mode 100755 scripts/install-cli.sh create mode 100644 tools/cli/junit.go create mode 100644 tools/cli/junit_test.go diff --git a/README.md b/README.md index d515389..88777f2 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-83.6%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.8%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) diff --git a/actions/junit-owners/action.yml b/actions/junit-owners/action.yml new file mode 100644 index 0000000..7842008 --- /dev/null +++ b/actions/junit-owners/action.yml @@ -0,0 +1,127 @@ +name: 'Codeowners Plus JUnit Owners' +description: 'Annotate JUnit XML test reports with the code owners of each test file' +author: 'Hans Baker' +branding: + icon: 'tag' + color: 'blue' +inputs: + path: + description: 'JUnit XML report(s) to annotate. Separate multiple reports with whitespace or newlines.' + required: true + type: + description: 'Framework that produced the report: pytest or jest. Selects the resolution strategy, and determines whether the resolved path is written back to the `file` attribute.' + required: true + root: + description: 'Path to the Git repository the reports belong to' + required: false + default: '.' + prefix: + description: 'Path prefix to prepend to test file paths, for reports that name files relative to a subdirectory (for example a workspace root)' + required: false + default: '' + attribute: + description: 'Attribute to write the owners to. A matching `Count` attribute is written alongside it.' + required: false + default: 'codeowners' + +runs: + using: 'composite' + steps: + - name: 'Resolve codeowners-cli binary' + id: resolve + shell: bash + env: + # The release tag this commit belongs to. Non-empty only in release + # commits: set by scripts/prepare-release.sh and cleared by + # scripts/post-release.sh. When set, the action downloads that + # release's prebuilt binary; when empty (any non-release ref) it + # builds from the checked-out source. + RELEASE_VERSION: '' + run: | + set -euo pipefail + # GITHUB_ACTION_PATH is this action's directory; the module root, which + # holds go.mod and scripts/, is two levels up. + ACTION_PATH=$(cd "${GITHUB_ACTION_PATH}/../.." && pwd) + { + echo "release-version=${RELEASE_VERSION}" + echo "bin=${RUNNER_TEMP:-/tmp}/codeowners-plus-cli/codeowners-cli" + echo "action-path=${ACTION_PATH}" + } >>"$GITHUB_OUTPUT" + + # RELEASE_VERSION not set -> not a release: build from source (cached). + - name: 'Restore cached built binary' + id: buildcache + if: steps.resolve.outputs.release-version == '' + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ${{ steps.resolve.outputs.bin }} + key: codeowners-cli-build-${{ github.action_ref || github.sha }}-${{ runner.os }}-${{ runner.arch }} + + - name: 'Set up Go' + if: steps.resolve.outputs.release-version == '' && steps.buildcache.outputs.cache-hit != 'true' + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 + with: + go-version-file: ${{ steps.resolve.outputs.action-path }}/go.mod + cache-dependency-path: ${{ steps.resolve.outputs.action-path }}/go.sum + + - name: 'Build codeowners-cli from source' + if: steps.resolve.outputs.release-version == '' && steps.buildcache.outputs.cache-hit != 'true' + shell: bash + env: + ACTION_PATH: ${{ steps.resolve.outputs.action-path }} + BIN: ${{ steps.resolve.outputs.bin }} + run: | + set -euo pipefail + mkdir -p "$(dirname "${BIN}")" + cd "${ACTION_PATH}" + CGO_ENABLED=0 \ + go build -trimpath -buildvcs=false -ldflags="-s -w" -o "${BIN}" ./tools/cli + + # RELEASE_VERSION set -> a release: download + verify the prebuilt binary (cached). + - name: 'Restore cached release binary' + id: bincache + if: steps.resolve.outputs.release-version != '' + uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 + with: + path: ${{ steps.resolve.outputs.bin }} + key: codeowners-cli-${{ steps.resolve.outputs.release-version }}-${{ runner.os }}-${{ runner.arch }} + + - name: 'Download codeowners-cli release binary' + if: steps.resolve.outputs.release-version != '' && steps.bincache.outputs.cache-hit != 'true' + shell: bash + env: + REPO: ${{ github.action_repository }} + ACTION_PATH: ${{ steps.resolve.outputs.action-path }} + TAG: ${{ steps.resolve.outputs.release-version }} + BIN: ${{ steps.resolve.outputs.bin }} + run: '"${ACTION_PATH}/scripts/install-cli.sh"' + + - name: 'Annotate JUnit reports' + shell: bash + env: + BIN: ${{ steps.resolve.outputs.bin }} + INPUT_PATH: ${{ inputs.path }} + INPUT_TYPE: ${{ inputs.type }} + INPUT_ROOT: ${{ inputs.root }} + INPUT_PREFIX: ${{ inputs.prefix }} + INPUT_ATTRIBUTE: ${{ inputs.attribute }} + run: | + set -euo pipefail + + args=(junit --in-place --root "${INPUT_ROOT}" --type "${INPUT_TYPE}" --attribute "${INPUT_ATTRIBUTE}") + if [ -n "${INPUT_PREFIX}" ]; then + args+=(--prefix "${INPUT_PREFIX}") + fi + # `path` accepts several values; commas and whitespace are equivalent + # separators so that a YAML list, a multiline block or a + # comma-separated string all work. + reports=() + for report in ${INPUT_PATH//,/ }; do + reports+=("${report}") + done + if [ "${#reports[@]}" -eq 0 ]; then + echo "Error: no reports given in 'path'." >&2 + exit 1 + fi + + "${BIN}" "${args[@]}" "${reports[@]}" diff --git a/scripts/install-cli.sh b/scripts/install-cli.sh new file mode 100755 index 0000000..a2d53ae --- /dev/null +++ b/scripts/install-cli.sh @@ -0,0 +1,102 @@ +#! /usr/bin/env bash +# Installs the prebuilt codeowners-cli binary for the current platform, +# verified against the release's checksums.txt. +# +# The companion script install-action.sh does the same for the GitHub Action +# binary. They are kept separate because the two release archives follow +# different goreleaser naming templates. +# +# Local use (all env vars optional): +# scripts/install-cli.sh # latest release -> ./codeowners-cli +# VERSION=v1.11.0 scripts/install-cli.sh # a specific release +# BIN=/usr/local/bin/codeowners-cli scripts/install-cli.sh +# curl -fsSL https://raw.githubusercontent.com/multimediallc/codeowners-plus/main/scripts/install-cli.sh | bash +# +# Overrides: REPO, VERSION (or TAG), OS, ARCH, BIN. The junit-owners action +# passes REPO/TAG/BIN; OS and ARCH are detected here so the script is +# self-contained. + +set -eu + +REPO="${REPO:-multimediallc/codeowners-plus}" +BIN="${BIN:-./codeowners-cli}" +TAG="${TAG:-${VERSION:-}}" + +# Detect OS unless overridden. The CLI archive title-cases the goreleaser +# {{ .Os }} token, so these are capitalized. +OS="${OS:-}" +if [ -z "${OS}" ]; then + case "$(uname -s)" in + Linux) OS="Linux" ;; + Darwin) OS="Darwin" ;; + *) + echo "Error: unsupported OS '$(uname -s)' (supported: Linux, Darwin)." >&2 + exit 1 + ;; + esac +fi + +# Detect ARCH unless overridden. The CLI archive spells amd64 as x86_64. +ARCH="${ARCH:-}" +if [ -z "${ARCH}" ]; then + case "$(uname -m)" in + x86_64 | amd64) ARCH="x86_64" ;; + arm64 | aarch64) ARCH="arm64" ;; + *) + echo "Error: unsupported arch '$(uname -m)' (supported: x86_64, arm64)." >&2 + exit 1 + ;; + esac +fi + +# Default to the latest release when no version was requested. +if [ -z "${TAG}" ]; then + TAG="$(curl -fsSL "https://api.github.com/repos/${REPO}/releases/latest" \ + | awk -F'"' '/"tag_name":/ {print $4; exit}')" + if [ -z "${TAG}" ]; then + echo "Error: could not determine the latest release of ${REPO}." >&2 + exit 1 + fi +fi + +# The CLI archive embeds the version without its leading "v". +asset="codeowners-cli_${TAG#v}_${OS}_${ARCH}.tar.gz" +binname="codeowners-cli" +base="https://github.com/${REPO}/releases/download/${TAG}" +tmp="$(mktemp -d)" +trap 'rm -rf "${tmp}"' EXIT + +echo "Downloading ${asset} from ${REPO} release ${TAG}" >&2 +curl -fsSL --retry 3 -o "${tmp}/${asset}" "${base}/${asset}" +curl -fsSL --retry 3 -o "${tmp}/checksums.txt" "${base}/checksums.txt" + +echo "Verifying ${asset} against checksums.txt" >&2 +expected="$(awk -v a="${asset}" '$2 == a {print $1}' "${tmp}/checksums.txt")" +if [ -z "${expected}" ]; then + echo "Error: ${asset} not found in checksums.txt" >&2 + exit 1 +fi +# Guard against a malformed digest: ' -c' treats an improperly +# formatted line as a skipped (passing) entry rather than a failure. +if ! printf '%s' "${expected}" | grep -Eq '^[0-9a-f]{64}$'; then + echo "Error: invalid checksum for ${asset} in checksums.txt" >&2 + exit 1 +fi +# sha256sum is GNU coreutils (Linux); macOS only ships shasum. +if command -v sha256sum >/dev/null 2>&1; then + verify=(sha256sum -c -) +else + verify=(shasum -a 256 -c -) +fi +if ! echo "${expected} ${tmp}/${asset}" | "${verify[@]}"; then + echo "Error: downloaded ${asset} does not match its release checksum" >&2 + exit 1 +fi + +echo "Extracting ${binname} from ${asset}" >&2 +tar -xzf "${tmp}/${asset}" -C "${tmp}" "${binname}" + +mkdir -p "$(dirname "${BIN}")" +mv "${tmp}/${binname}" "${BIN}" +chmod +x "${BIN}" +echo "Installed ${binname} ${TAG} to ${BIN}" >&2 diff --git a/tools/cli/junit.go b/tools/cli/junit.go new file mode 100644 index 0000000..f28d99b --- /dev/null +++ b/tools/cli/junit.go @@ -0,0 +1,326 @@ +package main + +import ( + "bytes" + "encoding/xml" + "fmt" + "io" + "os" + "path/filepath" + "slices" + "strconv" + "strings" + + "github.com/multimediallc/codeowners-plus/pkg/codeowners" + f "github.com/multimediallc/codeowners-plus/pkg/functional" +) + +// ReportType names the framework that produced a report. Frameworks differ in +// how they identify the file behind a test, so knowing the producer lets the +// right strategy be used and, just as importantly, the wrong one be skipped. +type ReportType string + +const ( + // TypePytest reads `classname` as a dotted module path; pytest omits the + // `file` attribute under its default xunit2 family. + TypePytest ReportType = "pytest" + // TypeJest reads the `file` attribute; jest's `classname` holds the text of + // the describe block, which is prose rather than a path. + TypeJest ReportType = "jest" +) + +var allowedReportTypes = []string{string(TypePytest), string(TypeJest)} + +func validateReportType(reportType string) (ReportType, error) { + if !slices.Contains(allowedReportTypes, reportType) { + return "", fmt.Errorf("invalid type %s. Must be one of %s", reportType, strings.Join(allowedReportTypes, ", ")) + } + return ReportType(reportType), nil +} + +// writesFile reports whether the resolved path is written back to the `file` +// attribute. Only pytest does: it has no `file` attribute to begin with, so the +// write is purely additive, whereas overwriting one a framework already set +// changes the meaning of a field its consumers may rely on. +func (t ReportType) writesFile() bool { + return t == TypePytest +} + +// extensions returns the file extensions to try when reading `classname` as a +// dotted module path. It is empty for types that never read it that way. +func (t ReportType) extensions() []string { + if !t.usesClassname() { + return nil + } + return []string{".py"} +} + +// usesClassname reports whether `classname` may be read as a dotted module +// path. Doing so for jest would be actively harmful: a describe block named +// something like "chatconnection.reconnectlimiter" looks exactly like a module +// path and could resolve to an unrelated file. +func (t ReportType) usesClassname() bool { + return t != TypeJest +} + +// ownerSeparator joins the owners of a file owned by more than one. A comma is +// unambiguous because a GitHub user or team name cannot contain one. +const ownerSeparator = "," + +type junitOpts struct { + root string + prefix string + attribute string + reportType ReportType + inPlace bool +} + +// fileResolver maps a element back to the repo-relative path of the +// file that defines it, memoizing the filesystem lookups it does along the way. +type fileResolver struct { + root string + prefix string + reportType ReportType + exts []string + cache map[string]bool +} + +func newFileResolver(root, prefix string, reportType ReportType) *fileResolver { + return &fileResolver{ + root: root, + prefix: prefix, + reportType: reportType, + exts: reportType.extensions(), + cache: make(map[string]bool), + } +} + +func (r *fileResolver) exists(rel string) bool { + if rel == "" || strings.HasPrefix(rel, "../") { + return false + } + if found, ok := r.cache[rel]; ok { + return found + } + stat, err := os.Stat(filepath.Join(r.root, rel)) + found := err == nil && !stat.IsDir() + r.cache[rel] = found + return found +} + +// candidates returns the repo-relative paths to try for a path taken from a +// report, most-specific first. A report may express paths relative to a +// subdirectory (jest names files relative to its own root), so the prefix is +// tried first, then the path as given, which keeps mixed reports working. +func (r *fileResolver) candidates(path string) []string { + if path == "" { + return nil + } + if filepath.IsAbs(path) { + rel, err := filepath.Rel(r.root, path) + if err != nil { + return nil + } + return []string{filepath.ToSlash(rel)} + } + clean := filepath.ToSlash(filepath.Clean(path)) + if r.prefix == "" { + return []string{clean} + } + return []string{filepath.ToSlash(filepath.Join(r.prefix, clean)), clean} +} + +// resolve locates the source file for a testcase, using its `file` attribute +// when the framework provides one (jest-junit's addFileAttribute, among +// others) and otherwise, where the report type allows it, reading `classname` +// as a dotted module path. +// +// The dotted form is what pytest emits, where a classname is either the module +// itself ("abuse.tests.test_abuse") or the module plus the test class +// ("abuse.tests.test_abuse.TestAbuse"), so trailing segments are trimmed until +// a real file is found. +func (r *fileResolver) resolve(file, classname string) string { + for _, candidate := range r.candidates(file) { + if r.exists(candidate) { + return candidate + } + } + + if classname == "" || !r.reportType.usesClassname() { + return "" + } + parts := strings.Split(classname, ".") + for len(parts) > 0 { + for _, candidate := range r.candidates(strings.Join(parts, "/")) { + for _, ext := range r.exts { + if r.exists(candidate + ext) { + return candidate + ext + } + } + } + parts = parts[:len(parts)-1] + } + return "" +} + +func attrValue(attrs []xml.Attr, name string) string { + for _, a := range attrs { + if a.Name.Local == name { + return a.Value + } + } + return "" +} + +func setAttrValue(attrs []xml.Attr, name, value string) []xml.Attr { + for i, a := range attrs { + if a.Name.Local == name { + attrs[i].Value = value + return attrs + } + } + return append(attrs, xml.Attr{Name: xml.Name{Local: name}, Value: value}) +} + +// collectTestFiles decodes a report and returns the resolved file for each +// , in document order, with "" for any that could not be resolved. +func collectTestFiles(raw []byte, r *fileResolver) ([]string, error) { + decoder := xml.NewDecoder(bytes.NewReader(raw)) + files := make([]string, 0) + for { + token, err := decoder.Token() + if err == io.EOF { + break + } + if err != nil { + return nil, err + } + start, ok := token.(xml.StartElement) + if !ok || start.Name.Local != "testcase" { + continue + } + files = append(files, r.resolve(attrValue(start.Attr, "file"), attrValue(start.Attr, "classname"))) + } + return files, nil +} + +// rewrite streams the report back out, adding ownership attributes to each +// . Tokens are copied through untouched, so formatting, comments and +// failure output survive the round trip. +func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts) ([]byte, error) { + out := &bytes.Buffer{} + decoder := xml.NewDecoder(bytes.NewReader(raw)) + encoder := xml.NewEncoder(out) + i := 0 + for { + token, err := decoder.Token() + if err == io.EOF { + break + } + if err != nil { + return nil, err + } + if start, ok := token.(xml.StartElement); ok && start.Name.Local == "testcase" { + if file := files[i]; file != "" { + if o.reportType.writesFile() { + start.Attr = setAttrValue(start.Attr, "file", file) + } + if fileOwners := owners[file]; len(fileOwners) > 0 { + start.Attr = setAttrValue(start.Attr, o.attribute, strings.Join(fileOwners, ownerSeparator)) + start.Attr = setAttrValue(start.Attr, o.attribute+"Count", strconv.Itoa(len(fileOwners))) + } + } + i++ + token = start + } + if err := encoder.EncodeToken(token); err != nil { + return nil, err + } + } + if err := encoder.Close(); err != nil { + return nil, err + } + return out.Bytes(), nil +} + +// annotateJUnit writes the owners of each test's source file onto its +// element, so that whatever consumes the report downstream can +// group results by ownership. +func annotateJUnit(paths []string, o junitOpts) error { + if repoStat, err := os.Lstat(o.root); err != nil || !repoStat.IsDir() { + return fmt.Errorf("root is not a directory: %s", o.root) + } + if gitStat, err := os.Stat(filepath.Join(o.root, ".git")); err != nil || !gitStat.IsDir() { + return fmt.Errorf("root is not a Git repository: %s", o.root) + } + if o.attribute == "" { + return fmt.Errorf("attribute name cannot be empty") + } + + type report struct { + path string + raw []byte + files []string + } + + resolver := newFileResolver(o.root, o.prefix, o.reportType) + reports := make([]*report, 0, len(paths)) + resolved := make(map[string]struct{}) + total, unresolved := 0, 0 + + for _, path := range paths { + raw, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("error reading %s: %w", path, err) + } + files, err := collectTestFiles(raw, resolver) + if err != nil { + return fmt.Errorf("error parsing %s: %w", path, err) + } + for _, file := range files { + total++ + if file == "" { + unresolved++ + continue + } + resolved[file] = struct{}{} + } + reports = append(reports, &report{path: path, raw: raw, files: files}) + } + + testFiles := make([]string, 0, len(resolved)) + for file := range resolved { + testFiles = append(testFiles, file) + } + slices.Sort(testFiles) + + diffFiles := f.Map(testFiles, func(file string) codeowners.DiffFile { + return codeowners.DiffFile{FileName: file} + }) + ownersMap, err := codeowners.New(o.root, diffFiles, &codeowners.FilesystemReader{}, io.Discard) + if err != nil { + return fmt.Errorf("error reading codeowners config: %w", err) + } + fileToOwners := mapFilesToOwners(ownersMap) + + for _, r := range reports { + annotated, err := rewrite(r.raw, r.files, fileToOwners, o) + if err != nil { + return fmt.Errorf("error rewriting %s: %w", r.path, err) + } + if !o.inPlace { + fmt.Println(string(annotated)) + continue + } + mode := os.FileMode(0o644) + if stat, err := os.Stat(r.path); err == nil { + mode = stat.Mode().Perm() + } + if err := os.WriteFile(r.path, annotated, mode); err != nil { + return fmt.Errorf("error writing %s: %w", r.path, err) + } + } + + _, _ = fmt.Fprintf(os.Stderr, "codeowners: annotated %d of %d testcases (%d unresolved)\n", total-unresolved, total, unresolved) + return nil +} diff --git a/tools/cli/junit_test.go b/tools/cli/junit_test.go new file mode 100644 index 0000000..fb253a9 --- /dev/null +++ b/tools/cli/junit_test.go @@ -0,0 +1,406 @@ +package main + +import ( + "encoding/xml" + "os" + "path/filepath" + "slices" + "strings" + "testing" +) + +// writeReport puts a JUnit report in a temp dir and returns its path. +func writeReport(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "report.xml") + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatalf("Failed to write report: %v", err) + } + return path +} + +// testcaseAttrs reads a report back and returns the attributes of each +// , keyed by the test name. +func testcaseAttrs(t *testing.T, path string) map[string]map[string]string { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + decoder := xml.NewDecoder(strings.NewReader(string(raw))) + found := make(map[string]map[string]string) + for { + token, err := decoder.Token() + if err != nil { + break + } + start, ok := token.(xml.StartElement) + if !ok || start.Name.Local != "testcase" { + continue + } + attrs := make(map[string]string) + for _, a := range start.Attr { + attrs[a.Name.Local] = a.Value + } + found[attrs["name"]] = attrs + } + return found +} + +func defaultOpts(root string, reportType ReportType) junitOpts { + return junitOpts{ + root: root, + attribute: "codeowners", + reportType: reportType, + inPlace: true, + } +} + +func TestValidateReportType(t *testing.T) { + tt := []struct { + name string + input string + expected ReportType + expectedErr bool + }{ + {name: "pytest", input: "pytest", expected: TypePytest}, + {name: "jest", input: "jest", expected: TypeJest}, + {name: "auto is no longer accepted", input: "auto", expectedErr: true}, + {name: "unknown framework", input: "mocha", expectedErr: true}, + {name: "empty", input: "", expectedErr: true}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got, err := validateReportType(tc.input) + if (err != nil) != tc.expectedErr { + t.Errorf("validateReportType() error = %v, expectedErr %v", err, tc.expectedErr) + return + } + if got != tc.expected { + t.Errorf("validateReportType() = %v, want %v", got, tc.expected) + } + }) + } +} + +func TestReportTypeDefaults(t *testing.T) { + tt := []struct { + reportType ReportType + writesFile bool + useClassname bool + exts []string + }{ + {reportType: TypePytest, writesFile: true, useClassname: true, exts: []string{".py"}}, + {reportType: TypeJest, writesFile: false, useClassname: false, exts: nil}, + } + + for _, tc := range tt { + t.Run(string(tc.reportType), func(t *testing.T) { + if got := tc.reportType.writesFile(); got != tc.writesFile { + t.Errorf("writesFile() = %v, want %v", got, tc.writesFile) + } + if got := tc.reportType.usesClassname(); got != tc.useClassname { + t.Errorf("usesClassname() = %v, want %v", got, tc.useClassname) + } + if got := tc.reportType.extensions(); !slices.Equal(got, tc.exts) { + t.Errorf("extensions() = %v, want %v", got, tc.exts) + } + }) + } +} + +func TestAnnotateJUnitJestDoesNotReadClassnameAsPath(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // "internal.util" is prose here, but it looks exactly like a module path + // and a real internal/util.go exists. A jest report must not resolve it. + report := writeReport(t, ` + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if got, ok := cases["describe_block_that_looks_like_a_path"]["codeowners"]; ok { + t.Errorf("classname should not resolve to a path for jest, got %q", got) + } + // The file attribute still resolves normally. + if got := cases["has_a_real_file"]["codeowners"]; got != "@frontend-team" { + t.Errorf("codeowners = %q, want %q", got, "@frontend-team") + } +} + +func TestAnnotateJUnitByFileAttribute(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if got := cases["renders"]["codeowners"]; got != "@frontend-team" { + t.Errorf("renders codeowners = %q, want %q", got, "@frontend-team") + } + if got := cases["renders"]["codeownersCount"]; got != "1" { + t.Errorf("renders codeownersCount = %q, want %q", got, "1") + } + if got := cases["helps"]["codeowners"]; got != "@backend-team,@security-team" { + t.Errorf("helps codeowners = %q, want %q", got, "@backend-team,@security-team") + } + if got := cases["helps"]["codeownersCount"]; got != "2" { + t.Errorf("helps codeownersCount = %q, want %q", got, "2") + } +} + +func TestAnnotateJUnitByClassname(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // A dotted classname is either the module itself or the module plus the + // test class, and both must resolve to the same file. + report := writeReport(t, ` + + + +`) + + if err := os.WriteFile(filepath.Join(testRepo, "internal", "util.py"), []byte("# python"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + for _, name := range []string{"module_level", "class_level"} { + if got := cases[name]["codeowners"]; got != "@backend-team,@security-team" { + t.Errorf("%s codeowners = %q, want %q", name, got, "@backend-team,@security-team") + } + } +} + +func TestAnnotateJUnitPrefix(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // The first names its file relative to the prefix, the second relative to + // the repo root; a mixed report must resolve both. + report := writeReport(t, ` + + + +`) + + opts := defaultOpts(testRepo, TypeJest) + opts.prefix = "frontend" + if err := annotateJUnit([]string{report}, opts); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + for _, name := range []string{"prefixed", "rooted"} { + if got := cases[name]["codeowners"]; got != "@frontend-team" { + t.Errorf("%s codeowners = %q, want %q", name, got, "@frontend-team") + } + } +} + +func TestAnnotateJUnitPytestWritesFile(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + + +`) + + if err := os.WriteFile(filepath.Join(testRepo, "internal", "util.py"), []byte("# python"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + for _, name := range []string{"from_classname", "from_class"} { + if got := cases[name]["file"]; got != "internal/util.py" { + t.Errorf("%s file = %q, want %q", name, got, "internal/util.py") + } + } +} + +func TestAnnotateJUnitLeavesUnresolvedTestcasesAlone(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if _, ok := cases["missing"]["codeowners"]; ok { + t.Error("unresolvable testcase should not be annotated") + } + // The file resolves but has no owner, so there is nothing to write. + if _, ok := cases["unowned"]["codeowners"]; ok { + t.Error("unowned testcase should not be annotated") + } +} + +func TestAnnotateJUnitPreservesReportContent(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + + +stack <trace> here + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + raw, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + out := string(raw) + for _, expected := range []string{ + ``, + ``, + `stack <trace> here`, + `tests="1"`, + `codeowners="@frontend-team"`, + } { + if !strings.Contains(out, expected) { + t.Errorf("annotated report missing %q\ngot: %s", expected, out) + } + } +} + +func TestAnnotateJUnitCustomAttribute(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + +`) + + opts := defaultOpts(testRepo, TypeJest) + opts.attribute = "owners" + if err := annotateJUnit([]string{report}, opts); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if got := cases["helps"]["owners"]; got != "@backend-team,@security-team" { + t.Errorf("owners = %q, want %q", got, "@backend-team,@security-team") + } + if got := cases["helps"]["ownersCount"]; got != "2" { + t.Errorf("ownersCount = %q, want %q", got, "2") + } + if _, ok := cases["helps"]["codeowners"]; ok { + t.Error("default attribute should not be written when overridden") + } +} + +func TestAnnotateJUnitMultipleReportsShareOneLookup(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + first := writeReport(t, ` +`) + second := writeReport(t, ` +`) + + if err := annotateJUnit([]string{first, second}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + if got := testcaseAttrs(t, first)["one"]["codeowners"]; got != "@frontend-team" { + t.Errorf("first report codeowners = %q, want %q", got, "@frontend-team") + } + if got := testcaseAttrs(t, second)["two"]["codeowners"]; got != "@backend-team,@security-team" { + t.Errorf("second report codeowners = %q, want %q", got, "@backend-team,@security-team") + } +} + +func TestAnnotateJUnitErrors(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + tt := []struct { + name string + paths []string + opts func(junitOpts) junitOpts + }{ + { + name: "root is not a directory", + paths: []string{writeReport(t, "")}, + opts: func(o junitOpts) junitOpts { + o.root = filepath.Join(testRepo, "main.go") + return o + }, + }, + { + name: "root is not a git repository", + paths: []string{writeReport(t, "")}, + opts: func(o junitOpts) junitOpts { + o.root = t.TempDir() + return o + }, + }, + { + name: "report does not exist", + paths: []string{filepath.Join(testRepo, "no-such-report.xml")}, + opts: func(o junitOpts) junitOpts { return o }, + }, + { + name: "report is not valid xml", + paths: []string{writeReport(t, "")}, + opts: func(o junitOpts) junitOpts { return o }, + }, + { + name: "empty attribute name", + paths: []string{writeReport(t, "")}, + opts: func(o junitOpts) junitOpts { + o.attribute = "" + return o + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if err := annotateJUnit(tc.paths, tc.opts(defaultOpts(testRepo, TypeJest))); err == nil { + t.Error("annotateJUnit() expected an error, got nil") + } + }) + } +} From 8ca6af84c912fd1958f0256267f2c485175e9dd5 Mon Sep 17 00:00:00 2001 From: ivan Date: Wed, 9 Sep 2026 16:01:27 -0700 Subject: [PATCH 3/7] fix --- README.md | 4 +- actions/junit-owners/action.yml | 11 ++- tools/cli/junit.go | 100 ++++++++++++++++---- tools/cli/junit_test.go | 159 ++++++++++++++++++++++++++++++++ 4 files changed, 254 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 88777f2..a335fbd 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-83.8%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.9%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -471,7 +471,7 @@ Available subcommands are: `junit` writes the owners of each test's source file onto its `` element: ```bash -codeowners-cli junit --in-place junit.xml +codeowners-cli junit --in-place --type pytest junit.xml ``` ```xml diff --git a/actions/junit-owners/action.yml b/actions/junit-owners/action.yml index 7842008..48896c8 100644 --- a/actions/junit-owners/action.yml +++ b/actions/junit-owners/action.yml @@ -55,7 +55,11 @@ runs: uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: ${{ steps.resolve.outputs.bin }} - key: codeowners-cli-build-${{ github.action_ref || github.sha }}-${{ runner.os }}-${{ runner.arch }} + # The ref alone is not enough: it is constant for a floating pin such + # as @main or @v1, so the cache would hit forever and upstream fixes + # would never be rebuilt. Hashing the sources keys the cache to what is + # actually being compiled. + key: codeowners-cli-build-${{ hashFiles(format('{0}/tools/cli/**', steps.resolve.outputs.action-path), format('{0}/pkg/**', steps.resolve.outputs.action-path), format('{0}/go.sum', steps.resolve.outputs.action-path)) }}-${{ runner.os }}-${{ runner.arch }} - name: 'Set up Go' if: steps.resolve.outputs.release-version == '' && steps.buildcache.outputs.cache-hit != 'true' @@ -115,10 +119,15 @@ runs: # `path` accepts several values; commas and whitespace are equivalent # separators so that a YAML list, a multiline block or a # comma-separated string all work. + # Split on whitespace only: globbing is disabled so that a path + # containing a wildcard is passed through as written rather than + # silently expanding, or vanishing when it matches nothing. + set -f reports=() for report in ${INPUT_PATH//,/ }; do reports+=("${report}") done + set +f if [ "${#reports[@]}" -eq 0 ]; then echo "Error: no reports given in 'path'." >&2 exit 1 diff --git a/tools/cli/junit.go b/tools/cli/junit.go index f28d99b..954d102 100644 --- a/tools/cli/junit.go +++ b/tools/cli/junit.go @@ -10,6 +10,7 @@ import ( "slices" "strconv" "strings" + "unicode" "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" @@ -137,8 +138,8 @@ func (r *fileResolver) candidates(path string) []string { // // The dotted form is what pytest emits, where a classname is either the module // itself ("abuse.tests.test_abuse") or the module plus the test class -// ("abuse.tests.test_abuse.TestAbuse"), so trailing segments are trimmed until -// a real file is found. +// ("abuse.tests.test_abuse.TestAbuse"), so trailing class segments are trimmed +// until a real file is found. func (r *fileResolver) resolve(file, classname string) string { for _, candidate := range r.candidates(file) { if r.exists(candidate) { @@ -150,7 +151,7 @@ func (r *fileResolver) resolve(file, classname string) string { return "" } parts := strings.Split(classname, ".") - for len(parts) > 0 { + for { for _, candidate := range r.candidates(strings.Join(parts, "/")) { for _, ext := range r.exts { if r.exists(candidate + ext) { @@ -158,9 +159,26 @@ func (r *fileResolver) resolve(file, classname string) string { } } } + // Only class segments may be trimmed. Trimming a module segment would + // walk up into the enclosing package, where an unrelated file of the + // same name would have its owners stamped onto this test. + if len(parts) < 2 || !isClassSegment(parts[len(parts)-1]) { + return "" + } parts = parts[:len(parts)-1] } - return "" +} + +// isClassSegment reports whether a dotted-path segment looks like a test class +// rather than a module. Modules are lower case by convention (PEP 8) and +// pytest only collects classes matching its `python_classes` prefix, which is +// capitalised by default. +func isClassSegment(segment string) bool { + if segment == "" { + return false + } + first := rune(segment[0]) + return unicode.IsUpper(first) } func attrValue(attrs []xml.Attr, name string) string { @@ -207,40 +225,44 @@ func collectTestFiles(raw []byte, r *fileResolver) ([]string, error) { // rewrite streams the report back out, adding ownership attributes to each // . Tokens are copied through untouched, so formatting, comments and // failure output survive the round trip. -func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts) ([]byte, error) { +func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts) ([]byte, int, error) { out := &bytes.Buffer{} decoder := xml.NewDecoder(bytes.NewReader(raw)) encoder := xml.NewEncoder(out) - i := 0 + i, annotated := 0, 0 for { token, err := decoder.Token() if err == io.EOF { break } if err != nil { - return nil, err + return nil, 0, err } if start, ok := token.(xml.StartElement); ok && start.Name.Local == "testcase" { if file := files[i]; file != "" { - if o.reportType.writesFile() { + // The write is only ever additive: a path the framework set + // itself is left alone, since its consumers may rely on the + // root it is relative to. + if o.reportType.writesFile() && attrValue(start.Attr, "file") == "" { start.Attr = setAttrValue(start.Attr, "file", file) } if fileOwners := owners[file]; len(fileOwners) > 0 { start.Attr = setAttrValue(start.Attr, o.attribute, strings.Join(fileOwners, ownerSeparator)) start.Attr = setAttrValue(start.Attr, o.attribute+"Count", strconv.Itoa(len(fileOwners))) + annotated++ } } i++ token = start } if err := encoder.EncodeToken(token); err != nil { - return nil, err + return nil, 0, err } } if err := encoder.Close(); err != nil { - return nil, err + return nil, 0, err } - return out.Bytes(), nil + return out.Bytes(), annotated, nil } // annotateJUnit writes the owners of each test's source file onto its @@ -253,10 +275,22 @@ func annotateJUnit(paths []string, o junitOpts) error { if gitStat, err := os.Stat(filepath.Join(o.root, ".git")); err != nil || !gitStat.IsDir() { return fmt.Errorf("root is not a Git repository: %s", o.root) } - if o.attribute == "" { - return fmt.Errorf("attribute name cannot be empty") + if !isXMLName(o.attribute) { + return fmt.Errorf("attribute is not a valid XML name: %q", o.attribute) + } + + if !o.inPlace && len(paths) > 1 { + return fmt.Errorf("writing to stdout supports a single report; use --in-place for %d reports", len(paths)) } + // A report may name its files absolutely, and those can only be made + // repo-relative against an absolute root. + root, err := filepath.Abs(o.root) + if err != nil { + return fmt.Errorf("error resolving root %s: %w", o.root, err) + } + o.root = root + type report struct { path string raw []byte @@ -273,6 +307,10 @@ func annotateJUnit(paths []string, o junitOpts) error { if err != nil { return fmt.Errorf("error reading %s: %w", path, err) } + // The encoder rejects an XML declaration that is not the first token, + // so a byte order mark or leading whitespace has to go before the + // report can be streamed back out. + raw = bytes.TrimLeft(bytes.TrimPrefix(raw, []byte("\xef\xbb\xbf")), " \t\r\n") files, err := collectTestFiles(raw, resolver) if err != nil { return fmt.Errorf("error parsing %s: %w", path, err) @@ -303,24 +341,52 @@ func annotateJUnit(paths []string, o junitOpts) error { } fileToOwners := mapFilesToOwners(ownersMap) + annotated := 0 for _, r := range reports { - annotated, err := rewrite(r.raw, r.files, fileToOwners, o) + out, count, err := rewrite(r.raw, r.files, fileToOwners, o) if err != nil { return fmt.Errorf("error rewriting %s: %w", r.path, err) } + annotated += count if !o.inPlace { - fmt.Println(string(annotated)) + fmt.Println(string(out)) continue } mode := os.FileMode(0o644) if stat, err := os.Stat(r.path); err == nil { mode = stat.Mode().Perm() } - if err := os.WriteFile(r.path, annotated, mode); err != nil { + if err := os.WriteFile(r.path, out, mode); err != nil { return fmt.Errorf("error writing %s: %w", r.path, err) } } - _, _ = fmt.Fprintf(os.Stderr, "codeowners: annotated %d of %d testcases (%d unresolved)\n", total-unresolved, total, unresolved) + resolvedCount := total - unresolved + _, _ = fmt.Fprintf(os.Stderr, "codeowners: annotated %d of %d testcases (%d resolved, %d unresolved)\n", + annotated, total, resolvedCount, unresolved) + // Resolving nothing at all is a misconfiguration rather than a repository + // without owners: the wrong --type or --prefix produces exactly this, and + // would otherwise rewrite every report unchanged and report success. + if total > 0 && resolvedCount == 0 { + return fmt.Errorf("no testcase could be traced back to a file in the repository; check --type and --prefix") + } return nil } + +// isXMLName reports whether a string is usable as an XML attribute name. An +// invalid one would produce a report no parser can read, and with --in-place +// the original is already gone by the time anything notices. +func isXMLName(name string) bool { + if name == "" { + return false + } + for i, r := range name { + switch { + case r == '_' || unicode.IsLetter(r): + case i > 0 && (r == '-' || r == '.' || unicode.IsDigit(r)): + default: + return false + } + } + return true +} diff --git a/tools/cli/junit_test.go b/tools/cli/junit_test.go index fb253a9..48d5bda 100644 --- a/tools/cli/junit_test.go +++ b/tools/cli/junit_test.go @@ -351,6 +351,157 @@ func TestAnnotateJUnitMultipleReportsShareOneLookup(t *testing.T) { } } +func TestIsXMLName(t *testing.T) { + tt := []struct { + name string + input string + expected bool + }{ + {name: "simple", input: "codeowners", expected: true}, + {name: "underscore start", input: "_owners", expected: true}, + {name: "digits and dashes after first", input: "owners-2.a", expected: true}, + {name: "empty", input: "", expected: false}, + {name: "contains a space", input: "code owners", expected: false}, + {name: "starts with a digit", input: "2owners", expected: false}, + {name: "contains a quote", input: `own"ers`, expected: false}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if got := isXMLName(tc.input); got != tc.expected { + t.Errorf("isXMLName(%q) = %v, want %v", tc.input, got, tc.expected) + } + }) + } +} + +func TestAnnotateJUnitResolvesAbsoluteFileAttribute(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // A relative root must still resolve an absolute path in the report. + report := writeReport(t, ` + + +`) + + opts := defaultOpts(testRepo, TypeJest) + opts.root = testRepo + "/." + if err := annotateJUnit([]string{report}, opts); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + if got := testcaseAttrs(t, report)["absolute"]["codeowners"]; got != "@frontend-team" { + t.Errorf("codeowners = %q, want %q", got, "@frontend-team") + } +} + +func TestAnnotateJUnitDoesNotOverwriteExistingFileAttribute(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + if err := os.WriteFile(filepath.Join(testRepo, "internal", "util.py"), []byte("# python"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + report := writeReport(t, ` + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if got := cases["keeps"]["file"]; got != "internal/util.py" { + t.Errorf("existing file attribute = %q, want it untouched", got) + } + if got := cases["gains"]["file"]; got != "internal/util.py" { + t.Errorf("missing file attribute = %q, want it written", got) + } +} + +func TestAnnotateJUnitDoesNotTrimPastTheModule(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // internal/util.py does not exist, so the only candidates left after + // trimming are the unrelated internal/ package and a top-level internal.py. + if err := os.WriteFile(filepath.Join(testRepo, "internal.py"), []byte("# unrelated"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + report := writeReport(t, ` + + +`) + + err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)) + if err == nil { + t.Fatal("annotateJUnit() should fail when nothing resolves") + } + if _, ok := testcaseAttrs(t, report)["module_is_missing"]["codeowners"]; ok { + t.Error("a missing module must not be attributed to an unrelated file") + } +} + +func TestAnnotateJUnitStripsByteOrderMark(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, "\xef\xbb\xbf"+` + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + if got := testcaseAttrs(t, report)["bom"]["codeowners"]; got != "@frontend-team" { + t.Errorf("codeowners = %q, want %q", got, "@frontend-team") + } +} + +func TestAnnotateJUnitCountsOnlyWhatItWrote(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // unowned/file.txt resolves but has no owner, so nothing is written for it. + report := writeReport(t, ` + + + +`) + + raw, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + resolved := []string{"frontend/app.js", "unowned/file.txt"} + owners := map[string][]string{"frontend/app.js": {"@frontend-team"}} + _, annotated, err := rewrite(raw, resolved, owners, defaultOpts(testRepo, TypeJest)) + if err != nil { + t.Fatalf("rewrite() error = %v", err) + } + if annotated != 1 { + t.Errorf("rewrite() annotated = %d, want 1 (the unowned file carries no attribute)", annotated) + } +} + +func TestAnnotateJUnitRejectsMultipleReportsToStdout(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + first := writeReport(t, ``) + second := writeReport(t, ``) + + opts := defaultOpts(testRepo, TypeJest) + opts.inPlace = false + if err := annotateJUnit([]string{first, second}, opts); err == nil { + t.Error("annotateJUnit() should refuse several reports without --in-place") + } +} + func TestAnnotateJUnitErrors(t *testing.T) { testRepo, cleanup := setupTestRepo(t) defer cleanup() @@ -394,6 +545,14 @@ func TestAnnotateJUnitErrors(t *testing.T) { return o }, }, + { + name: "attribute is not a valid xml name", + paths: []string{writeReport(t, "")}, + opts: func(o junitOpts) junitOpts { + o.attribute = "code owners" + return o + }, + }, } for _, tc := range tt { From 7daaebeba89151bd9e7f5a0581495b596917a331 Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 10 Sep 2026 10:35:08 -0700 Subject: [PATCH 4/7] code review --- README.md | 2 +- actions/junit-owners/action.yml | 29 +++++- tools/cli/junit.go | 54 +++++------ tools/cli/junit_test.go | 167 +++++++++++++++++++++++++++++++- 4 files changed, 216 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index a335fbd..dc1a89c 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-83.9%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-84.0%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) diff --git a/actions/junit-owners/action.yml b/actions/junit-owners/action.yml index 48896c8..323dc5b 100644 --- a/actions/junit-owners/action.yml +++ b/actions/junit-owners/action.yml @@ -42,10 +42,33 @@ runs: # GITHUB_ACTION_PATH is this action's directory; the module root, which # holds go.mod and scripts/, is two levels up. ACTION_PATH=$(cd "${GITHUB_ACTION_PATH}/../.." && pwd) + + # The build cache has to be keyed on what is actually compiled. The ref + # alone is constant for a floating pin such as @main or @v1, so the + # cache would hit forever and upstream fixes would never be rebuilt. + # hashFiles() cannot do this: it only matches inside GITHUB_WORKSPACE, + # and a remote `uses:` puts the action under _actions/, outside it, + # where it returns an empty string and every revision shares one key. + # sha256sum is GNU coreutils (Linux); macOS runners only ship shasum. + if command -v sha256sum >/dev/null 2>&1; then + sum() { sha256sum "$@"; } + else + sum() { shasum -a 256 "$@"; } + fi + # Paths are relative to ACTION_PATH so the digest does not change with + # the checkout location, and go.mod/go.sum are included because they + # determine the toolchain and dependencies the binary is built from. + SRC_HASH=$( + cd "${ACTION_PATH}" && + { find tools/cli pkg -type f -name '*.go'; echo go.mod; echo go.sum; } | + LC_ALL=C sort | xargs sum | sum | cut -d' ' -f1 + ) + { echo "release-version=${RELEASE_VERSION}" echo "bin=${RUNNER_TEMP:-/tmp}/codeowners-plus-cli/codeowners-cli" echo "action-path=${ACTION_PATH}" + echo "src-hash=${SRC_HASH}" } >>"$GITHUB_OUTPUT" # RELEASE_VERSION not set -> not a release: build from source (cached). @@ -55,11 +78,7 @@ runs: uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 with: path: ${{ steps.resolve.outputs.bin }} - # The ref alone is not enough: it is constant for a floating pin such - # as @main or @v1, so the cache would hit forever and upstream fixes - # would never be rebuilt. Hashing the sources keys the cache to what is - # actually being compiled. - key: codeowners-cli-build-${{ hashFiles(format('{0}/tools/cli/**', steps.resolve.outputs.action-path), format('{0}/pkg/**', steps.resolve.outputs.action-path), format('{0}/go.sum', steps.resolve.outputs.action-path)) }}-${{ runner.os }}-${{ runner.arch }} + key: codeowners-cli-build-${{ steps.resolve.outputs.src-hash }}-${{ runner.os }}-${{ runner.arch }} - name: 'Set up Go' if: steps.resolve.outputs.release-version == '' && steps.buildcache.outputs.cache-hit != 'true' diff --git a/tools/cli/junit.go b/tools/cli/junit.go index 954d102..13de9b6 100644 --- a/tools/cli/junit.go +++ b/tools/cli/junit.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" "unicode" + "unicode/utf8" "github.com/multimediallc/codeowners-plus/pkg/codeowners" f "github.com/multimediallc/codeowners-plus/pkg/functional" @@ -64,10 +65,10 @@ func (t ReportType) usesClassname() bool { return t != TypeJest } -// ownerSeparator joins the owners of a file owned by more than one. A comma is -// unambiguous because a GitHub user or team name cannot contain one. const ownerSeparator = "," +const countSuffix = "Count" + type junitOpts struct { root string prefix string @@ -134,12 +135,7 @@ func (r *fileResolver) candidates(path string) []string { // resolve locates the source file for a testcase, using its `file` attribute // when the framework provides one (jest-junit's addFileAttribute, among // others) and otherwise, where the report type allows it, reading `classname` -// as a dotted module path. -// -// The dotted form is what pytest emits, where a classname is either the module -// itself ("abuse.tests.test_abuse") or the module plus the test class -// ("abuse.tests.test_abuse.TestAbuse"), so trailing class segments are trimmed -// until a real file is found. +// as a dotted module path. Trailing class segments in classname are trimmed ("abuse.tests.test_abuse.TestAbuse") func (r *fileResolver) resolve(file, classname string) string { for _, candidate := range r.candidates(file) { if r.exists(candidate) { @@ -174,10 +170,7 @@ func (r *fileResolver) resolve(file, classname string) string { // pytest only collects classes matching its `python_classes` prefix, which is // capitalised by default. func isClassSegment(segment string) bool { - if segment == "" { - return false - } - first := rune(segment[0]) + first, _ := utf8.DecodeRuneInString(segment) return unicode.IsUpper(first) } @@ -190,6 +183,15 @@ func attrValue(attrs []xml.Attr, name string) string { return "" } +func removeAttrValue(attrs []xml.Attr, name string) []xml.Attr { + for i, a := range attrs { + if a.Name.Local == name { + return append(attrs[:i], attrs[i+1:]...) + } + } + return attrs +} + func setAttrValue(attrs []xml.Attr, name, value string) []xml.Attr { for i, a := range attrs { if a.Name.Local == name { @@ -239,6 +241,13 @@ func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts return nil, 0, err } if start, ok := token.(xml.StartElement); ok && start.Name.Local == "testcase" { + // A report may already carry attributes from an earlier run. + // Clearing them first keeps re-annotation idempotent: a test whose + // file has since become unowned, or can no longer be resolved at + // all, must not be left attributed to its former owners. + start.Attr = removeAttrValue(start.Attr, o.attribute) + start.Attr = removeAttrValue(start.Attr, o.attribute+countSuffix) + if file := files[i]; file != "" { // The write is only ever additive: a path the framework set // itself is left alone, since its consumers may rely on the @@ -248,7 +257,7 @@ func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts } if fileOwners := owners[file]; len(fileOwners) > 0 { start.Attr = setAttrValue(start.Attr, o.attribute, strings.Join(fileOwners, ownerSeparator)) - start.Attr = setAttrValue(start.Attr, o.attribute+"Count", strconv.Itoa(len(fileOwners))) + start.Attr = setAttrValue(start.Attr, o.attribute+countSuffix, strconv.Itoa(len(fileOwners))) annotated++ } } @@ -265,9 +274,6 @@ func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts return out.Bytes(), annotated, nil } -// annotateJUnit writes the owners of each test's source file onto its -// element, so that whatever consumes the report downstream can -// group results by ownership. func annotateJUnit(paths []string, o junitOpts) error { if repoStat, err := os.Lstat(o.root); err != nil || !repoStat.IsDir() { return fmt.Errorf("root is not a directory: %s", o.root) @@ -326,6 +332,10 @@ func annotateJUnit(paths []string, o junitOpts) error { reports = append(reports, &report{path: path, raw: raw, files: files}) } + if total > 0 && len(resolved) == 0 { + return fmt.Errorf("no testcase could be traced back to a file in the repository (%d testcases, all unresolved); check --type and --prefix", total) + } + testFiles := make([]string, 0, len(resolved)) for file := range resolved { testFiles = append(testFiles, file) @@ -361,21 +371,11 @@ func annotateJUnit(paths []string, o junitOpts) error { } } - resolvedCount := total - unresolved _, _ = fmt.Fprintf(os.Stderr, "codeowners: annotated %d of %d testcases (%d resolved, %d unresolved)\n", - annotated, total, resolvedCount, unresolved) - // Resolving nothing at all is a misconfiguration rather than a repository - // without owners: the wrong --type or --prefix produces exactly this, and - // would otherwise rewrite every report unchanged and report success. - if total > 0 && resolvedCount == 0 { - return fmt.Errorf("no testcase could be traced back to a file in the repository; check --type and --prefix") - } + annotated, total, total-unresolved, unresolved) return nil } -// isXMLName reports whether a string is usable as an XML attribute name. An -// invalid one would produce a report no parser can read, and with --in-place -// the original is already gone by the time anything notices. func isXMLName(name string) bool { if name == "" { return false diff --git a/tools/cli/junit_test.go b/tools/cli/junit_test.go index 48d5bda..79a3ce2 100644 --- a/tools/cli/junit_test.go +++ b/tools/cli/junit_test.go @@ -9,7 +9,6 @@ import ( "testing" ) -// writeReport puts a JUnit report in a temp dir and returns its path. func writeReport(t *testing.T, content string) string { t.Helper() path := filepath.Join(t.TempDir(), "report.xml") @@ -436,13 +435,23 @@ func TestAnnotateJUnitDoesNotTrimPastTheModule(t *testing.T) { `) - err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)) - if err == nil { + before, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err == nil { t.Fatal("annotateJUnit() should fail when nothing resolves") } if _, ok := testcaseAttrs(t, report)["module_is_missing"]["codeowners"]; ok { t.Error("a missing module must not be attributed to an unrelated file") } + after, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + if string(before) != string(after) { + t.Error("a report must be left untouched when nothing resolves") + } } func TestAnnotateJUnitStripsByteOrderMark(t *testing.T) { @@ -502,6 +511,158 @@ func TestAnnotateJUnitRejectsMultipleReportsToStdout(t *testing.T) { } } +func TestIsClassSegment(t *testing.T) { + tt := []struct { + name string + input string + expected bool + }{ + {name: "test class", input: "TestFoo", expected: true}, + {name: "module", input: "test_foo", expected: false}, + {name: "empty", input: "", expected: false}, + // The leading byte of a multi-byte character is itself a code point + // that unicode.IsUpper reports as upper case for much of Latin-1, so + // these must be decoded rather than indexed. + {name: "lower case accented module", input: "\u00f3micron", expected: false}, + {name: "lower case umlaut module", input: "\u00fcber", expected: false}, + {name: "lower case cyrillic module", input: "\u0442\u0435\u0441\u0442", expected: false}, + {name: "upper case accented class", input: "\u00d3micron", expected: true}, + {name: "upper case cyrillic class", input: "\u0422\u0435\u0441\u0442", expected: true}, + {name: "caseless script", input: "\u65e5\u672c\u8a9e", expected: false}, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if got := isClassSegment(tc.input); got != tc.expected { + t.Errorf("isClassSegment(%q) = %v, want %v", tc.input, got, tc.expected) + } + }) + } +} + +func TestAnnotateJUnitDoesNotTrimAccentedModule(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // "\u00f3micron" is a package, not a class, so a missing module beneath it must + // not fall through and take the owners of \u00f3micron.py. + if err := os.MkdirAll(filepath.Join(testRepo, "\u00f3micron"), 0755); err != nil { + t.Fatalf("Failed to create directory: %v", err) + } + if err := os.WriteFile(filepath.Join(testRepo, "\u00f3micron.py"), []byte("# unrelated"), 0644); err != nil { + t.Fatalf("Failed to write test file: %v", err) + } + report := writeReport(t, ` + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypePytest)); err == nil { + t.Fatal("annotateJUnit() should fail when nothing resolves") + } + if _, ok := testcaseAttrs(t, report)["t"]["codeowners"]; ok { + t.Error("an accented package segment must not be trimmed as if it were a class") + } +} + +func TestAnnotateJUnitLeavesReportIntactWhenNothingResolves(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + +`) + before, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + + // The wrong prefix resolves nothing. The run must abort before writing, + // leaving the annotation an earlier correct run produced. + opts := defaultOpts(testRepo, TypeJest) + opts.prefix = "wrong" + if err := annotateJUnit([]string{report}, opts); err == nil { + t.Fatal("annotateJUnit() should fail when a bad prefix resolves nothing") + } + + after, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + if string(before) != string(after) { + t.Errorf("a failed run must not rewrite the report\nbefore: %s\nafter: %s", before, after) + } + if got := testcaseAttrs(t, report)["annotated"]["codeowners"]; got != "@frontend-team" { + t.Errorf("existing annotation was lost: codeowners = %q", got) + } +} + +func TestAnnotateJUnitClearsStaleAttributes(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // Every testcase arrives already annotated by an earlier run. Only the + // first still has an owner; the second resolves to a file that no longer + // has one, and the third no longer resolves at all. + report := writeReport(t, ` + + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + cases := testcaseAttrs(t, report) + if got := cases["still_owned"]["codeowners"]; got != "@frontend-team" { + t.Errorf("still_owned codeowners = %q, want %q", got, "@frontend-team") + } + if got := cases["still_owned"]["codeownersCount"]; got != "1" { + t.Errorf("still_owned codeownersCount = %q, want %q", got, "1") + } + for _, name := range []string{"now_unowned", "now_unresolved"} { + if got, ok := cases[name]["codeowners"]; ok { + t.Errorf("%s kept a stale codeowners attribute: %q", name, got) + } + if got, ok := cases[name]["codeownersCount"]; ok { + t.Errorf("%s kept a stale codeownersCount attribute: %q", name, got) + } + } +} + +func TestAnnotateJUnitIsIdempotent(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + report := writeReport(t, ` + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("first annotateJUnit() error = %v", err) + } + first, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("second annotateJUnit() error = %v", err) + } + second, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + + if string(first) != string(second) { + t.Errorf("re-annotating changed the report\nfirst: %s\nsecond: %s", first, second) + } +} + func TestAnnotateJUnitErrors(t *testing.T) { testRepo, cleanup := setupTestRepo(t) defer cleanup() From 6cd1b57d2be50d53574b40f037c8a0003748a072 Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 10 Sep 2026 13:08:28 -0700 Subject: [PATCH 5/7] remove --attribute --- README.md | 4 +- actions/junit-owners/action.yml | 7 +--- scripts/install-cli.sh | 1 - tools/cli/junit.go | 35 ++++------------- tools/cli/junit_test.go | 68 --------------------------------- tools/cli/main.go | 7 ---- 6 files changed, 10 insertions(+), 112 deletions(-) diff --git a/README.md b/README.md index dc1a89c..27adedc 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Code Ownership & Review Assignment Tool - GitHub CODEOWNERS but better [![Go Report Card](https://goreportcard.com/badge/github.com/multimediallc/codeowners-plus)](https://goreportcard.com/report/github.com/multimediallc/codeowners-plus?kill_cache=1) [![Tests](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml/badge.svg)](https://github.com/multimediallc/codeowners-plus/actions/workflows/go.yml) -![Coverage](https://img.shields.io/badge/Coverage-84.0%25-brightgreen) +![Coverage](https://img.shields.io/badge/Coverage-83.9%25-brightgreen) [![License](https://img.shields.io/badge/License-BSD%203--Clause-blue.svg)](https://opensource.org/licenses/BSD-3-Clause) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-2.1-4baaaa.svg)](CODE_OF_CONDUCT.md) @@ -525,7 +525,6 @@ Useful options: | `--in-place`, `-i` | Rewrite the report in place instead of writing to stdout | | `--type`, `-t` | **Required.** Framework that produced the report: `pytest` or `jest` | | `--prefix`, `-p` | Path prefix for reports that name files relative to a subdirectory | -| `--attribute`, `-a` | Attribute to write the owners to (default `codeowners`) | Testcases that cannot be resolved, and files with no owner, are left untouched. @@ -549,7 +548,6 @@ that uploads the report: | `type` | *required* | Framework that produced the report: `pytest` or `jest` | | `root` | `.` | Path to the Git repository the reports belong to | | `prefix` | `''` | Path prefix for reports that name files relative to a subdirectory | -| `attribute` | `codeowners` | Attribute to write the owners to | ## Contributing diff --git a/actions/junit-owners/action.yml b/actions/junit-owners/action.yml index 323dc5b..440b86a 100644 --- a/actions/junit-owners/action.yml +++ b/actions/junit-owners/action.yml @@ -19,10 +19,6 @@ inputs: description: 'Path prefix to prepend to test file paths, for reports that name files relative to a subdirectory (for example a workspace root)' required: false default: '' - attribute: - description: 'Attribute to write the owners to. A matching `Count` attribute is written alongside it.' - required: false - default: 'codeowners' runs: using: 'composite' @@ -127,11 +123,10 @@ runs: INPUT_TYPE: ${{ inputs.type }} INPUT_ROOT: ${{ inputs.root }} INPUT_PREFIX: ${{ inputs.prefix }} - INPUT_ATTRIBUTE: ${{ inputs.attribute }} run: | set -euo pipefail - args=(junit --in-place --root "${INPUT_ROOT}" --type "${INPUT_TYPE}" --attribute "${INPUT_ATTRIBUTE}") + args=(junit --in-place --root "${INPUT_ROOT}" --type "${INPUT_TYPE}") if [ -n "${INPUT_PREFIX}" ]; then args+=(--prefix "${INPUT_PREFIX}") fi diff --git a/scripts/install-cli.sh b/scripts/install-cli.sh index a2d53ae..96575e0 100755 --- a/scripts/install-cli.sh +++ b/scripts/install-cli.sh @@ -10,7 +10,6 @@ # scripts/install-cli.sh # latest release -> ./codeowners-cli # VERSION=v1.11.0 scripts/install-cli.sh # a specific release # BIN=/usr/local/bin/codeowners-cli scripts/install-cli.sh -# curl -fsSL https://raw.githubusercontent.com/multimediallc/codeowners-plus/main/scripts/install-cli.sh | bash # # Overrides: REPO, VERSION (or TAG), OS, ARCH, BIN. The junit-owners action # passes REPO/TAG/BIN; OS and ARCH are detected here so the script is diff --git a/tools/cli/junit.go b/tools/cli/junit.go index 13de9b6..8b0505c 100644 --- a/tools/cli/junit.go +++ b/tools/cli/junit.go @@ -67,18 +67,18 @@ func (t ReportType) usesClassname() bool { const ownerSeparator = "," -const countSuffix = "Count" +const ( + ownersAttr = "codeowners" + ownersCountAttr = ownersAttr + "Count" +) type junitOpts struct { root string prefix string - attribute string reportType ReportType inPlace bool } -// fileResolver maps a element back to the repo-relative path of the -// file that defines it, memoizing the filesystem lookups it does along the way. type fileResolver struct { root string prefix string @@ -245,8 +245,8 @@ func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts // Clearing them first keeps re-annotation idempotent: a test whose // file has since become unowned, or can no longer be resolved at // all, must not be left attributed to its former owners. - start.Attr = removeAttrValue(start.Attr, o.attribute) - start.Attr = removeAttrValue(start.Attr, o.attribute+countSuffix) + start.Attr = removeAttrValue(start.Attr, ownersAttr) + start.Attr = removeAttrValue(start.Attr, ownersCountAttr) if file := files[i]; file != "" { // The write is only ever additive: a path the framework set @@ -256,8 +256,8 @@ func rewrite(raw []byte, files []string, owners map[string][]string, o junitOpts start.Attr = setAttrValue(start.Attr, "file", file) } if fileOwners := owners[file]; len(fileOwners) > 0 { - start.Attr = setAttrValue(start.Attr, o.attribute, strings.Join(fileOwners, ownerSeparator)) - start.Attr = setAttrValue(start.Attr, o.attribute+countSuffix, strconv.Itoa(len(fileOwners))) + start.Attr = setAttrValue(start.Attr, ownersAttr, strings.Join(fileOwners, ownerSeparator)) + start.Attr = setAttrValue(start.Attr, ownersCountAttr, strconv.Itoa(len(fileOwners))) annotated++ } } @@ -281,10 +281,6 @@ func annotateJUnit(paths []string, o junitOpts) error { if gitStat, err := os.Stat(filepath.Join(o.root, ".git")); err != nil || !gitStat.IsDir() { return fmt.Errorf("root is not a Git repository: %s", o.root) } - if !isXMLName(o.attribute) { - return fmt.Errorf("attribute is not a valid XML name: %q", o.attribute) - } - if !o.inPlace && len(paths) > 1 { return fmt.Errorf("writing to stdout supports a single report; use --in-place for %d reports", len(paths)) } @@ -375,18 +371,3 @@ func annotateJUnit(paths []string, o junitOpts) error { annotated, total, total-unresolved, unresolved) return nil } - -func isXMLName(name string) bool { - if name == "" { - return false - } - for i, r := range name { - switch { - case r == '_' || unicode.IsLetter(r): - case i > 0 && (r == '-' || r == '.' || unicode.IsDigit(r)): - default: - return false - } - } - return true -} diff --git a/tools/cli/junit_test.go b/tools/cli/junit_test.go index 79a3ce2..fe1fbd8 100644 --- a/tools/cli/junit_test.go +++ b/tools/cli/junit_test.go @@ -49,7 +49,6 @@ func testcaseAttrs(t *testing.T, path string) map[string]map[string]string { func defaultOpts(root string, reportType ReportType) junitOpts { return junitOpts{ root: root, - attribute: "codeowners", reportType: reportType, inPlace: true, } @@ -302,33 +301,6 @@ func TestAnnotateJUnitPreservesReportContent(t *testing.T) { } } -func TestAnnotateJUnitCustomAttribute(t *testing.T) { - testRepo, cleanup := setupTestRepo(t) - defer cleanup() - - report := writeReport(t, ` - - -`) - - opts := defaultOpts(testRepo, TypeJest) - opts.attribute = "owners" - if err := annotateJUnit([]string{report}, opts); err != nil { - t.Fatalf("annotateJUnit() error = %v", err) - } - - cases := testcaseAttrs(t, report) - if got := cases["helps"]["owners"]; got != "@backend-team,@security-team" { - t.Errorf("owners = %q, want %q", got, "@backend-team,@security-team") - } - if got := cases["helps"]["ownersCount"]; got != "2" { - t.Errorf("ownersCount = %q, want %q", got, "2") - } - if _, ok := cases["helps"]["codeowners"]; ok { - t.Error("default attribute should not be written when overridden") - } -} - func TestAnnotateJUnitMultipleReportsShareOneLookup(t *testing.T) { testRepo, cleanup := setupTestRepo(t) defer cleanup() @@ -350,30 +322,6 @@ func TestAnnotateJUnitMultipleReportsShareOneLookup(t *testing.T) { } } -func TestIsXMLName(t *testing.T) { - tt := []struct { - name string - input string - expected bool - }{ - {name: "simple", input: "codeowners", expected: true}, - {name: "underscore start", input: "_owners", expected: true}, - {name: "digits and dashes after first", input: "owners-2.a", expected: true}, - {name: "empty", input: "", expected: false}, - {name: "contains a space", input: "code owners", expected: false}, - {name: "starts with a digit", input: "2owners", expected: false}, - {name: "contains a quote", input: `own"ers`, expected: false}, - } - - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - if got := isXMLName(tc.input); got != tc.expected { - t.Errorf("isXMLName(%q) = %v, want %v", tc.input, got, tc.expected) - } - }) - } -} - func TestAnnotateJUnitResolvesAbsoluteFileAttribute(t *testing.T) { testRepo, cleanup := setupTestRepo(t) defer cleanup() @@ -698,22 +646,6 @@ func TestAnnotateJUnitErrors(t *testing.T) { paths: []string{writeReport(t, "")}, opts: func(o junitOpts) junitOpts { return o }, }, - { - name: "empty attribute name", - paths: []string{writeReport(t, "")}, - opts: func(o junitOpts) junitOpts { - o.attribute = "" - return o - }, - }, - { - name: "attribute is not a valid xml name", - paths: []string{writeReport(t, "")}, - opts: func(o junitOpts) junitOpts { - o.attribute = "code owners" - return o - }, - }, } for _, tc := range tt { diff --git a/tools/cli/main.go b/tools/cli/main.go index c9a9e29..ca32b31 100644 --- a/tools/cli/main.go +++ b/tools/cli/main.go @@ -236,12 +236,6 @@ func main() { Value: "", Usage: "Path prefix to prepend to test file paths, for reports that name files relative to a subdirectory", }, - &cli.StringFlag{ - Name: "attribute", - Aliases: []string{"a"}, - Value: "codeowners", - Usage: "Attribute to write the owners to (a matching `Count` attribute is written alongside it)", - }, &cli.BoolFlag{ Name: "in-place", Aliases: []string{"i"}, @@ -267,7 +261,6 @@ func main() { return annotateJUnit(targets, junitOpts{ root: repo, prefix: cmd.String("prefix"), - attribute: cmd.String("attribute"), reportType: reportType, inPlace: cmd.Bool("in-place"), }) From bbf606d5e8cc162bc7138d091d790b7f708db300 Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 10 Sep 2026 13:41:11 -0700 Subject: [PATCH 6/7] fix prefix fields --- tools/cli/junit.go | 10 ++-- tools/cli/junit_test.go | 102 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/tools/cli/junit.go b/tools/cli/junit.go index 8b0505c..3c876c8 100644 --- a/tools/cli/junit.go +++ b/tools/cli/junit.go @@ -174,9 +174,13 @@ func isClassSegment(segment string) bool { return unicode.IsUpper(first) } +func isUnqualified(a xml.Attr, name string) bool { + return a.Name.Space == "" && a.Name.Local == name +} + func attrValue(attrs []xml.Attr, name string) string { for _, a := range attrs { - if a.Name.Local == name { + if isUnqualified(a, name) { return a.Value } } @@ -185,7 +189,7 @@ func attrValue(attrs []xml.Attr, name string) string { func removeAttrValue(attrs []xml.Attr, name string) []xml.Attr { for i, a := range attrs { - if a.Name.Local == name { + if isUnqualified(a, name) { return append(attrs[:i], attrs[i+1:]...) } } @@ -194,7 +198,7 @@ func removeAttrValue(attrs []xml.Attr, name string) []xml.Attr { func setAttrValue(attrs []xml.Attr, name, value string) []xml.Attr { for i, a := range attrs { - if a.Name.Local == name { + if isUnqualified(a, name) { attrs[i].Value = value return attrs } diff --git a/tools/cli/junit_test.go b/tools/cli/junit_test.go index fe1fbd8..284d5bb 100644 --- a/tools/cli/junit_test.go +++ b/tools/cli/junit_test.go @@ -546,6 +546,108 @@ func TestAnnotateJUnitLeavesReportIntactWhenNothingResolves(t *testing.T) { } } +func TestIsUnqualified(t *testing.T) { + tt := []struct { + name string + attr xml.Attr + expected bool + }{ + { + name: "plain attribute", + attr: xml.Attr{Name: xml.Name{Local: "codeowners"}}, + expected: true, + }, + { + name: "prefixed attribute", + attr: xml.Attr{Name: xml.Name{Space: "vendor", Local: "codeowners"}}, + expected: false, + }, + { + name: "namespaced attribute", + attr: xml.Attr{Name: xml.Name{Space: "http://example.com/v", Local: "codeowners"}}, + expected: false, + }, + { + name: "different name", + attr: xml.Attr{Name: xml.Name{Local: "classname"}}, + expected: false, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + if got := isUnqualified(tc.attr, "codeowners"); got != tc.expected { + t.Errorf("isUnqualified(%v, %q) = %v, want %v", tc.attr.Name, "codeowners", got, tc.expected) + } + }) + } +} + +func TestAnnotateJUnitPreservesNamespacedAttributes(t *testing.T) { + testRepo, cleanup := setupTestRepo(t) + defer cleanup() + + // Someone else's vendor:codeowners must survive untouched, both where this + // tool writes its own attribute and where it clears a stale one. + report := writeReport(t, ` + + + +`) + + if err := annotateJUnit([]string{report}, defaultOpts(testRepo, TypeJest)); err != nil { + t.Fatalf("annotateJUnit() error = %v", err) + } + + raw, err := os.ReadFile(report) + if err != nil { + t.Fatalf("Failed to read report: %v", err) + } + + // The encoder rewrites namespace prefixes, so assert on the decoded + // attribute rather than on the serialised form. + type attrs struct{ plain, namespaced string } + found := make(map[string]attrs) + decoder := xml.NewDecoder(strings.NewReader(string(raw))) + for { + token, err := decoder.Token() + if err != nil { + break + } + start, ok := token.(xml.StartElement) + if !ok || start.Name.Local != "testcase" { + continue + } + var name string + var got attrs + for _, a := range start.Attr { + switch { + case isUnqualified(a, "name"): + name = a.Value + case isUnqualified(a, "codeowners"): + got.plain = a.Value + case a.Name.Space != "" && a.Name.Local == "codeowners": + got.namespaced = a.Value + } + } + found[name] = got + } + + if got := found["owned"].namespaced; got != "@them" { + t.Errorf("owned: vendor:codeowners = %q, want %q", got, "@them") + } + if got := found["owned"].plain; got != "@frontend-team" { + t.Errorf("owned: codeowners = %q, want %q", got, "@frontend-team") + } + // The unowned testcase loses its own stale attribute but keeps the vendor one. + if got := found["unowned"].namespaced; got != "@them" { + t.Errorf("unowned: vendor:codeowners = %q, want it preserved", got) + } + if got := found["unowned"].plain; got != "" { + t.Errorf("unowned: codeowners = %q, want it cleared", got) + } +} + func TestAnnotateJUnitClearsStaleAttributes(t *testing.T) { testRepo, cleanup := setupTestRepo(t) defer cleanup() From 6a6973e45b35dbdb4f6656b38bc5ea89027353ab Mon Sep 17 00:00:00 2001 From: ivan Date: Thu, 10 Sep 2026 14:41:21 -0700 Subject: [PATCH 7/7] clean up action --- actions/junit-owners/action.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/actions/junit-owners/action.yml b/actions/junit-owners/action.yml index 440b86a..ee513bf 100644 --- a/actions/junit-owners/action.yml +++ b/actions/junit-owners/action.yml @@ -1,9 +1,5 @@ name: 'Codeowners Plus JUnit Owners' description: 'Annotate JUnit XML test reports with the code owners of each test file' -author: 'Hans Baker' -branding: - icon: 'tag' - color: 'blue' inputs: path: description: 'JUnit XML report(s) to annotate. Separate multiple reports with whitespace or newlines.'