From 64a54390b24e5a00092ebad3c5c183edccb556db Mon Sep 17 00:00:00 2001 From: Vishal Vaibhav Date: Fri, 19 Jun 2026 11:46:32 +0530 Subject: [PATCH 1/2] feat: move-diff-logic-within-the-plugin Committer: codeyogico --- .github/workflows/pr-coverage.yml | 39 +++--- AGENTS.md | 11 +- README.md | 11 ++ internal/plugin/githubdiff/diff.go | 74 ++++++++++++ internal/plugin/githubdiff/diff_test.go | 88 ++++++++++++++ internal/plugin/runner.go | 28 +++++ internal/plugin/runner_diffsource_test.go | 111 ++++++++++++++++++ .../unifieddiff/changed_source_loader.go | 12 ++ .../unifieddiff/changed_source_loader_test.go | 83 +++++++++++++ internal/test/mocks/property_getter.go | 24 ++++ 10 files changed, 463 insertions(+), 18 deletions(-) create mode 100644 internal/plugin/githubdiff/diff.go create mode 100644 internal/plugin/githubdiff/diff_test.go create mode 100644 internal/plugin/runner_diffsource_test.go create mode 100644 internal/plugin/sourcelines/unifieddiff/changed_source_loader_test.go diff --git a/.github/workflows/pr-coverage.yml b/.github/workflows/pr-coverage.yml index b3cbf7e..7cfc12b 100644 --- a/.github/workflows/pr-coverage.yml +++ b/.github/workflows/pr-coverage.yml @@ -20,8 +20,10 @@ jobs: steps: - name: Check out the repo uses: actions/checkout@v4 - with: - fetch-depth: 0 # need the base branch present to diff against it + # No fetch-depth needed: the plugin now fetches the PR diff from the + # GitHub API (PARAMETER_DIFF_SOURCE=github), so we don't diff against the + # base branch locally. Checkout is only here to build the image and run + # the tests that produce the coverage report. - name: Set up Go uses: actions/setup-go@v5 @@ -44,6 +46,10 @@ jobs: - name: Report coverage on changed lines env: + # Fetch the PR diff straight from the GitHub API instead of piping in a + # local `git diff`. Note the API diff covers ALL changed files, not just + # *.go, so non-Go edits (yml/md) show up as "untracked changed lines". + PARAMETER_DIFF_SOURCE: github PARAMETER_COVERAGE_TYPE: cobertura PARAMETER_COVERAGE_FILE: coverage.xml # Must equal the cobertura path (the dir go test ran in). @@ -53,18 +59,19 @@ jobs: BUILD_PULL_REQUEST_NUMBER: ${{ github.event.pull_request.number }} REPOSITORY_ORG: ${{ github.repository_owner }} REPOSITORY_NAME: ${{ github.event.repository.name }} + # The container still mounts the workspace so it can read coverage.xml; + # the diff no longer comes from stdin, so `-i` and the pipe are gone. run: | - git fetch --no-tags origin "${{ github.base_ref }}" - git --no-pager diff --unified=0 "origin/${{ github.base_ref }}" -- '*.go' \ - | docker run --rm -i \ - -e PARAMETER_COVERAGE_TYPE \ - -e PARAMETER_COVERAGE_FILE \ - -e PARAMETER_SOURCE_DIRS \ - -e PARAMETER_GH_API_KEY \ - -e BUILD_PULL_REQUEST_NUMBER \ - -e REPOSITORY_ORG \ - -e REPOSITORY_NAME \ - -v "${{ github.workspace }}:${{ github.workspace }}" \ - -w "${{ github.workspace }}" \ - --entrypoint /plugin \ - pr-code-coverage:ci + docker run --rm \ + -e PARAMETER_DIFF_SOURCE \ + -e PARAMETER_COVERAGE_TYPE \ + -e PARAMETER_COVERAGE_FILE \ + -e PARAMETER_SOURCE_DIRS \ + -e PARAMETER_GH_API_KEY \ + -e BUILD_PULL_REQUEST_NUMBER \ + -e REPOSITORY_ORG \ + -e REPOSITORY_NAME \ + -v "${{ github.workspace }}:${{ github.workspace }}" \ + -w "${{ github.workspace }}" \ + --entrypoint /plugin \ + pr-code-coverage:ci diff --git a/AGENTS.md b/AGENTS.md index bb47610..8d49758 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,11 +46,12 @@ The CI (`.github/workflows/test.yml`) runs build, test, `make format`, and `make Entry point `main.go` → `plugin.NewRunner().Run(os.LookupEnv, os.Stdin, os.Stdout)`. The runner (`internal/plugin/runner.go`) reads **config from env vars** (`PARAMETER_*`, -`BUILD_PULL_REQUEST_NUMBER`, `REPOSITORY_ORG`, `REPOSITORY_NAME`), the **diff from stdin**, and the +`BUILD_PULL_REQUEST_NUMBER`, `REPOSITORY_ORG`, `REPOSITORY_NAME`), the **diff** (from stdin by +default, or fetched from the GitHub API — see `PARAMETER_DIFF_SOURCE` below), and the **coverage report from the file** at `PARAMETER_COVERAGE_FILE`. ``` -stdin (unified diff) ──► sourcelines/unifieddiff ──► []domain.SourceLine +diff (unified) ──► sourcelines/unifieddiff ──► []domain.SourceLine │ {Module,SrcDir,Pkg,FileName,LineNumber,LineValue} coverage file ──► coverage.Loader.Load() ──► coverage.Report │ @@ -65,6 +66,12 @@ coverage file ──► coverage.Loader.Load() ──► coverage.Report Key packages: - `internal/plugin/sourcelines/unifieddiff/changed_source_loader.go` — parses the unified diff into changed `SourceLine`s. `PARAMETER_SOURCE_DIRS` controls how a path prefix is split into `SrcDir`/`Pkg`. + Handles both `--unified=0` diffs (the stdin/Vela path, no context lines) and diffs that carry context + lines (e.g. from the GitHub API) — context lines advance the new-file line counter but aren't recorded. +- `internal/plugin/githubdiff/diff.go` — alternative diff source. When `PARAMETER_DIFF_SOURCE=github`, + the runner fetches the PR diff from `GET /repos/{owner}/{repo}/pulls/{n}` with the + `application/vnd.github.v3.diff` media type instead of reading stdin. Default is `stdin` + (unchanged behavior). The `github` mode requires `PARAMETER_GH_API_KEY` + the three build-context vars. - `internal/plugin/coverage/` — `report.go` defines the two interfaces every format implements: `Loader.Load(file) (Report, error)` and `Report.GetCoverageData(module, sourceDir, pkg, fileName, lineNumber) (*CoverageData, bool)`. - `internal/plugin/calculator/calculator.go` — joins changed lines to coverage data. diff --git a/README.md b/README.md index 8fe364b..4c14382 100644 --- a/README.md +++ b/README.md @@ -312,6 +312,16 @@ git --no-pager diff --unified=0 "origin/$BASE_REF" -- '*.go' | docker run --rm - A working GitHub Actions example lives in [`.github/workflows/pr-coverage.yml`](.github/workflows/pr-coverage.yml). +**No git checkout?** Set `PARAMETER_DIFF_SOURCE=github` and the plugin fetches the PR's diff straight from the GitHub API instead of reading stdin — so you don't need the repo checked out or `git` available, just the coverage file and a token. This uses the same diff GitHub shows reviewers (computed against the merge base), so it can differ slightly from a local `git diff origin/`. It requires `PARAMETER_GH_API_KEY`, `BUILD_PULL_REQUEST_NUMBER`, `REPOSITORY_ORG`, and `REPOSITORY_NAME`. + +``` +docker run --rm \ + -e PARAMETER_DIFF_SOURCE=github \ + -e PARAMETER_COVERAGE_TYPE -e PARAMETER_COVERAGE_FILE -e PARAMETER_SOURCE_DIRS \ + -e PARAMETER_GH_API_KEY -e BUILD_PULL_REQUEST_NUMBER -e REPOSITORY_ORG -e REPOSITORY_NAME \ + ghcr.io/target/pull-request-code-coverage:latest +``` + --- ## Parameters @@ -326,6 +336,7 @@ A working GitHub Actions example lives in [`.github/workflows/pr-coverage.yml`]( | `module` | `PARAMETER_MODULE` | no | _(empty)_ | sub-module path prefix to strip, for multi-module projects (e.g. a Gradle multi-project build) | | `gh_api_key` | `PARAMETER_GH_API_KEY` (or `PLUGIN_GH_API_KEY`) | no | | token used to post the PR comment. If unset, no comment is posted (console only) | | `gh_api_base_url` | `PARAMETER_GH_API_BASE_URL` | no | `https://api.github.com` | GitHub API root. For GitHub Enterprise, use the full root including `/api/v3` | +| `diff_source` | `PARAMETER_DIFF_SOURCE` | no | `stdin` | where the PR diff comes from: `stdin` (pipe a `git diff` in, the default) or `github` (fetch the PR diff from the GitHub API — needs no git checkout; requires `gh_api_key` and the three build-context values) | | `debug` | `PARAMETER_DEBUG` | no | `false` | enable debug logging | **Build context** — provided automatically by Vela; set these yourself on other CIs to enable the PR comment. diff --git a/internal/plugin/githubdiff/diff.go b/internal/plugin/githubdiff/diff.go new file mode 100644 index 0000000..43ad137 --- /dev/null +++ b/internal/plugin/githubdiff/diff.go @@ -0,0 +1,74 @@ +// Package githubdiff fetches a pull request's unified diff directly from the +// GitHub REST API, so the plugin can determine what a PR changed without a git +// checkout. It is an alternative to reading the diff piped in on stdin. +package githubdiff + +import ( + "bytes" + "fmt" + "io" + "strings" + + "github.com/pkg/errors" + "github.com/target/pull-request-code-coverage/internal/plugin/pluginhttp" +) + +const httpResponseOK = 200 + +// Loader retrieves the diff for a single pull request from the GitHub API. +type Loader struct { + apiKey string + apiBaseURL string + pr string + owner string + repo string + httpClient pluginhttp.Client +} + +func NewLoader(apiKey string, apiBaseURL string, pr string, owner string, repo string, httpClient pluginhttp.Client) *Loader { + return &Loader{ + apiKey: apiKey, + apiBaseURL: apiBaseURL, + pr: pr, + owner: owner, + repo: repo, + httpClient: httpClient, + } +} + +// Load requests the pull request diff using the `application/vnd.github.v3.diff` +// media type. GitHub returns the same unified diff it shows reviewers — computed +// against the merge base and carrying context lines — which the unified-diff +// parser handles. The whole response is read into memory and returned as a +// reader so the caller can treat it exactly like the stdin diff. +func (l *Loader) Load() (io.Reader, error) { + url := fmt.Sprintf("%v/repos/%v/%v/pulls/%v", strings.TrimRight(l.apiBaseURL, "/"), l.owner, l.repo, l.pr) + + req, newErr := l.httpClient.NewRequest("GET", url, nil) + if newErr != nil { + return nil, errors.Wrap(newErr, "Failed creating request to github") + } + + req.Header.Add("Authorization", "token "+l.apiKey) + req.Header.Add("Accept", "application/vnd.github.v3.diff") + + resp, doErr := l.httpClient.Do(req) + if doErr != nil { + return nil, errors.Wrap(doErr, "Failed calling github") + } + + defer func() { + _ = resp.Body.Close() + }() + + if resp.StatusCode != httpResponseOK { + return nil, errors.Errorf("Failed calling github: bad status code: %v", resp.StatusCode) + } + + body, readErr := io.ReadAll(resp.Body) + if readErr != nil { + return nil, errors.Wrap(readErr, "Failed reading diff response from github") + } + + return bytes.NewReader(body), nil +} diff --git a/internal/plugin/githubdiff/diff_test.go b/internal/plugin/githubdiff/diff_test.go new file mode 100644 index 0000000..ae30bd5 --- /dev/null +++ b/internal/plugin/githubdiff/diff_test.go @@ -0,0 +1,88 @@ +package githubdiff + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/target/pull-request-code-coverage/internal/plugin/pluginhttp" +) + +func TestLoader_Load_BuildsRequestAndReturnsDiff(t *testing.T) { + mockClient := &pluginhttp.MockClient{} + request := httptest.NewRequest("GET", "http://anywhere", nil) + + mockClient. + On("NewRequest", "GET", "https://api.github.com/repos/some_org/some_repo/pulls/123", nil). + Return(request, nil) + mockClient. + On("Do", request). + Return(&http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader("THE DIFF"))}, nil) + + reader, err := NewLoader("SOME_API_KEY", "https://api.github.com", "123", "some_org", "some_repo", mockClient).Load() + + assert.NoError(t, err) + + body, _ := io.ReadAll(reader) + assert.Equal(t, "THE DIFF", string(body)) + + assert.Equal(t, "token SOME_API_KEY", request.Header.Get("Authorization")) + assert.Equal(t, "application/vnd.github.v3.diff", request.Header.Get("Accept")) + + mockClient.AssertExpectations(t) +} + +func TestLoader_Load_TrimsTrailingSlashOnBaseURL(t *testing.T) { + mockClient := &pluginhttp.MockClient{} + request := httptest.NewRequest("GET", "http://anywhere", nil) + + mockClient. + On("NewRequest", "GET", "https://git.example.com/api/v3/repos/o/r/pulls/9", nil). + Return(request, nil) + mockClient. + On("Do", request). + Return(&http.Response{StatusCode: 200, Body: io.NopCloser(strings.NewReader(""))}, nil) + + _, err := NewLoader("k", "https://git.example.com/api/v3/", "9", "o", "r", mockClient).Load() + + assert.NoError(t, err) + mockClient.AssertExpectations(t) +} + +func TestLoader_Load_FailedNewRequest(t *testing.T) { + mockClient := &pluginhttp.MockClient{} + mockClient.On("NewRequest", mock.Anything, mock.Anything, mock.Anything).Return(nil, errors.New("boom")) + + _, err := NewLoader("k", "https://api.github.com", "1", "o", "r", mockClient).Load() + + assert.EqualError(t, err, "Failed creating request to github: boom") +} + +func TestLoader_Load_FailedDo(t *testing.T) { + mockClient := &pluginhttp.MockClient{} + request := httptest.NewRequest("GET", "http://anywhere", nil) + + mockClient.On("NewRequest", mock.Anything, mock.Anything, mock.Anything).Return(request, nil) + mockClient.On("Do", request).Return(nil, errors.New("boom")) + + _, err := NewLoader("k", "https://api.github.com", "1", "o", "r", mockClient).Load() + + assert.EqualError(t, err, "Failed calling github: boom") +} + +func TestLoader_Load_BadStatus(t *testing.T) { + mockClient := &pluginhttp.MockClient{} + request := httptest.NewRequest("GET", "http://anywhere", nil) + + mockClient.On("NewRequest", mock.Anything, mock.Anything, mock.Anything).Return(request, nil) + mockClient.On("Do", request).Return(&http.Response{StatusCode: 404, Body: io.NopCloser(strings.NewReader(""))}, nil) + + _, err := NewLoader("k", "https://api.github.com", "1", "o", "r", mockClient).Load() + + assert.EqualError(t, err, "Failed calling github: bad status code: 404") +} diff --git a/internal/plugin/runner.go b/internal/plugin/runner.go index 6f7fd53..70743a6 100644 --- a/internal/plugin/runner.go +++ b/internal/plugin/runner.go @@ -14,6 +14,7 @@ import ( "github.com/target/pull-request-code-coverage/internal/plugin/coverage/jacoco" "github.com/target/pull-request-code-coverage/internal/plugin/coverage/lcov" "github.com/target/pull-request-code-coverage/internal/plugin/coverage/pythoncov" + "github.com/target/pull-request-code-coverage/internal/plugin/githubdiff" "github.com/target/pull-request-code-coverage/internal/plugin/pluginhttp" "github.com/target/pull-request-code-coverage/internal/plugin/pluginjson" "github.com/target/pull-request-code-coverage/internal/plugin/reporter" @@ -97,6 +98,33 @@ func (*DefaultRunner) Run(propertyGetter func(string) (string, bool), changedSou return errors.Wrap(loadCoverageErr, "Failed loading coverage report") } + diffSource, found := propertyGetter("PARAMETER_DIFF_SOURCE") + if !found || diffSource == "" { + logrus.Info("PARAMETER_DIFF_SOURCE was missing, defaulting to stdin") + diffSource = "stdin" + } + + switch diffSource { + case "stdin": + // changedSourceLinesSource already points at the piped-in diff (stdin); + // nothing to do. This is the original, default behavior. + case "github": + if !ghAPIKeyFound || !repoPRFound || !repoOwnerFound || !repoNameFound { + return errors.New("PARAMETER_DIFF_SOURCE=github requires a GitHub API key (PARAMETER_GH_API_KEY), BUILD_PULL_REQUEST_NUMBER, REPOSITORY_ORG and REPOSITORY_NAME") + } + + logrus.Info("PARAMETER_DIFF_SOURCE is github, fetching diff from the GitHub API") + + diffReader, fetchErr := githubdiff.NewLoader(ghAPIKey, ghAPIBaseURL, repoPR, repoOwner, repoName, &pluginhttp.DefaultClient{}).Load() + if fetchErr != nil { + return errors.Wrap(fetchErr, "Failed fetching diff from github") + } + + changedSourceLinesSource = diffReader + default: + return errors.Errorf("Unknown PARAMETER_DIFF_SOURCE %q (expected \"stdin\" or \"github\")", diffSource) + } + changedLines, changedLinesErr := unifieddiff.NewChangedSourceLinesLoader(module, sourceDirs).Load(changedSourceLinesSource) if changedLinesErr != nil { return errors.Wrap(changedLinesErr, "Failed loading changed lines") diff --git a/internal/plugin/runner_diffsource_test.go b/internal/plugin/runner_diffsource_test.go new file mode 100644 index 0000000..064502c --- /dev/null +++ b/internal/plugin/runner_diffsource_test.go @@ -0,0 +1,111 @@ +package plugin + +import ( + "bytes" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/target/pull-request-code-coverage/internal/test/mocks" +) + +// When PARAMETER_DIFF_SOURCE is unset the runner must behave exactly as before: +// read the diff from the reader it was given (stdin). Covered implicitly by the +// existing golden tests; this asserts the explicit "stdin" value is equivalent. +func TestDefaultRunner_Run_DiffSourceStdin_Explicit(t *testing.T) { + propGetter := mocks.NewMockPropertyGetter() + + propGetter.On("GetProperty", "PARAMETER_COVERAGE_FILE").Return("../test/jacocoTestReport.xml", true) + propGetter.On("GetProperty", "PARAMETER_COVERAGE_TYPE").Return("jacoco", true) + propGetter.On("GetProperty", "PARAMETER_SOURCE_DIRS").Return("src/main/java", true) + propGetter.On("GetProperty", "PARAMETER_MODULE").Return("category-search", true) + propGetter.On("GetProperty", "PARAMETER_DIFF_SOURCE").Return("stdin", true) + + var buf bytes.Buffer + + err := NewRunner().Run(propGetter.GetProperty, MustOpen(t, "../test/sample_unified.diff"), &buf) + assert.NoError(t, err) + assert.Contains(t, buf.String(), "Patch Coverage Report") + + propGetter.AssertExpectations(t) +} + +func TestDefaultRunner_Run_DiffSourceGithub_MissingCreds(t *testing.T) { + propGetter := mocks.NewMockPropertyGetter() + + propGetter.On("GetProperty", "PARAMETER_COVERAGE_FILE").Return("../test/jacocoTestReport.xml", true) + propGetter.On("GetProperty", "PARAMETER_COVERAGE_TYPE").Return("jacoco", true) + propGetter.On("GetProperty", "PARAMETER_SOURCE_DIRS").Return("src/main/java", true) + propGetter.On("GetProperty", "PARAMETER_DIFF_SOURCE").Return("github", true) + + err := NewRunner().Run(propGetter.GetProperty, strings.NewReader(""), os.Stdout) + assert.EqualError(t, err, "PARAMETER_DIFF_SOURCE=github requires a GitHub API key (PARAMETER_GH_API_KEY), BUILD_PULL_REQUEST_NUMBER, REPOSITORY_ORG and REPOSITORY_NAME") + + propGetter.AssertExpectations(t) +} + +func TestDefaultRunner_Run_DiffSourceUnknown(t *testing.T) { + propGetter := mocks.NewMockPropertyGetter() + + propGetter.On("GetProperty", "PARAMETER_COVERAGE_FILE").Return("../test/jacocoTestReport.xml", true) + propGetter.On("GetProperty", "PARAMETER_COVERAGE_TYPE").Return("jacoco", true) + propGetter.On("GetProperty", "PARAMETER_SOURCE_DIRS").Return("src/main/java", true) + propGetter.On("GetProperty", "PARAMETER_DIFF_SOURCE").Return("banana", true) + + err := NewRunner().Run(propGetter.GetProperty, strings.NewReader(""), os.Stdout) + assert.EqualError(t, err, "Unknown PARAMETER_DIFF_SOURCE \"banana\" (expected \"stdin\" or \"github\")") + + propGetter.AssertExpectations(t) +} + +// End-to-end: with PARAMETER_DIFF_SOURCE=github the runner fetches the diff from +// the GitHub API instead of stdin (an empty reader here) and produces the same +// report. The mock server serves the same diff fixture for the PR-diff GET and +// accepts the PR-comment POST, so the output matches the stdin golden exactly. +func TestDefaultRunner_Run_DiffSourceGithub_FetchesDiff(t *testing.T) { + diff, readErr := os.ReadFile("../test/example_go_unified.diff") + assert.NoError(t, readErr) + + var diffRequests int + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/repos/some_org/some_repo/pulls/123" { + diffRequests++ + assert.Equal(t, "application/vnd.github.v3.diff", r.Header.Get("Accept")) + assert.Equal(t, "token SOME_API_KEY", r.Header.Get("Authorization")) + w.WriteHeader(200) + _, _ = w.Write(diff) + return + } + + // The PR-comment POST from the github reporter. + w.WriteHeader(201) + })) + defer ts.Close() + + propGetter := mocks.NewMockPropertyGetter() + + propGetter.On("GetProperty", "PARAMETER_COVERAGE_FILE").Return("../test/example_go_coverage_with_source_dir.xml", true) + propGetter.On("GetProperty", "PARAMETER_COVERAGE_TYPE").Return("cobertura", true) + propGetter.On("GetProperty", "PARAMETER_SOURCE_DIRS").Return("/go/github.com/target/pull-request-code-coverage", true) + propGetter.On("GetProperty", "PARAMETER_GH_API_KEY").Return("SOME_API_KEY", true) + propGetter.On("GetProperty", "PARAMETER_GH_API_BASE_URL").Return(ts.URL, true) + propGetter.On("GetProperty", "BUILD_PULL_REQUEST_NUMBER").Return("123", true) + propGetter.On("GetProperty", "REPOSITORY_ORG").Return("some_org", true) + propGetter.On("GetProperty", "REPOSITORY_NAME").Return("some_repo", true) + propGetter.On("GetProperty", "PARAMETER_DIFF_SOURCE").Return("github", true) + + var buf bytes.Buffer + + // stdin is intentionally empty — the diff must come from the GitHub API. + err := NewRunner().Run(propGetter.GetProperty, strings.NewReader(""), &buf) + assert.NoError(t, err) + + assert.Equal(t, 1, diffRequests, "expected exactly one PR-diff fetch") + assert.Equal(t, "──────────────────────────────────────────────────────────────\n 📊 Patch Coverage Report — changed lines only\n──────────────────────────────────────────────────────────────\n\n Diff coverage: 97% 🟢 — 177 of 182 changed instructions covered\n\n Summary\n Covered instructions 97% (177)\n Missed instructions 3% (5)\n Tracked changed lines 8% (182)\n Untracked changed lines 92% (2216)\n\n Note: \"lines\" are the source lines you changed; \"instructions\" are the\n executable units the coverage tool counts inside them (one line can hold\n several, e.g. JaCoCo bytecode), so the two counts differ.\n\n Coverage by file (lowest coverage first)\n 0% 0 cov / 4 miss main.go\n 96% 27 cov / 1 miss internal/plugin/runner.go\n 100% 10 cov / 0 miss internal/plugin/calculator/calculator.go\n 100% 29 cov / 0 miss internal/plugin/coverage/jacoco/report.go\n 100% 19 cov / 0 miss internal/plugin/domain/domain.go\n 100% 25 cov / 0 miss internal/plugin/reporter/reporter.go\n 100% 64 cov / 0 miss internal/plugin/sourcelines/unifieddiff/changed_source_loader.go\n 100% 3 cov / 0 miss internal/test/mocks/property_getter.go\n (25 file(s) with no measurable lines omitted)\n\n Uncovered lines (5)\n - internal/plugin/runner.go:72\n func GetCoverageReportLoader(coverageType string, sourceDir string) coverage.Loader {\n - main.go:10\n \terr := plugin.NewRunner().Run(os.LookupEnv, os.Stdin, os.Stdout)\n - main.go:12\n \tif err != nil {\n - main.go:13\n \t\tlog.WithFields(log.Fields{\n - main.go:17\n \t\tos.Exit(1)\n\n──────────────────────────────────────────────────────────────\n", buf.String()) + + propGetter.AssertExpectations(t) +} diff --git a/internal/plugin/sourcelines/unifieddiff/changed_source_loader.go b/internal/plugin/sourcelines/unifieddiff/changed_source_loader.go index a485761..0302871 100644 --- a/internal/plugin/sourcelines/unifieddiff/changed_source_loader.go +++ b/internal/plugin/sourcelines/unifieddiff/changed_source_loader.go @@ -39,6 +39,7 @@ func (l *Loader) Load(inReader io.Reader) ([]domain.SourceLine, error) { var changedFileLine = regexp.MustCompile("^[+][+][+][ ]b?[/](.*)") var changedLineCounts = regexp.MustCompile("^[@][@][ ][-].*?[ ][+](.*?)[ ][@][@].*") var addedLine = regexp.MustCompile("^[+].*") +var contextLine = regexp.MustCompile("^[ ].*") var emptyStr = "" // nolint: gocyclo @@ -139,6 +140,17 @@ func getChangedLinesFromUnifiedDiff(unifiedDiffLines []string, module string, so Module: *currentModule, }) + currentRelativeLine++ + linesLeftInBlock-- + } else if linesLeftInBlock > 0 && contextLine.MatchString(line) { + + // A context line is unchanged code the diff shows for orientation. We + // don't record it (the PR didn't change it), but it still occupies a + // line in the new file and counts against the hunk's line budget, so + // advance both counters to keep subsequent changed-line numbers + // correct. Diffs produced with --unified=0 (the Vela/stdin path) have + // no context lines, so this branch is inert there; it only matters for + // diffs that carry context, such as those fetched from the GitHub API. currentRelativeLine++ linesLeftInBlock-- } diff --git a/internal/plugin/sourcelines/unifieddiff/changed_source_loader_test.go b/internal/plugin/sourcelines/unifieddiff/changed_source_loader_test.go new file mode 100644 index 0000000..54f6c91 --- /dev/null +++ b/internal/plugin/sourcelines/unifieddiff/changed_source_loader_test.go @@ -0,0 +1,83 @@ +package unifieddiff + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// unified=0 is what the git-diff/stdin path produces: no context lines, every +// hunk line is an addition. This pins the original behavior. +func TestLoad_Unified0_NoContextLines(t *testing.T) { + diff := strings.Join([]string{ + "diff --git a/foo.go b/foo.go", + "--- a/foo.go", + "+++ b/foo.go", + "@@ -10,0 +11,2 @@ func foo() {", + "+\ta := 1", + "+\tb := 2", + }, "\n") + + lines, err := NewChangedSourceLinesLoader("", []string{""}).Load(strings.NewReader(diff)) + + assert.NoError(t, err) + assert.Len(t, lines, 2) + assert.Equal(t, 11, lines[0].LineNumber) + assert.Equal(t, "\ta := 1", lines[0].LineValue) + assert.Equal(t, 12, lines[1].LineNumber) + assert.Equal(t, "\tb := 2", lines[1].LineValue) +} + +// unified=3 is what the GitHub API returns: changed lines surrounded by context +// lines. Only the added lines should be recorded, and their line numbers must +// account for the context lines that precede them. +func TestLoad_Unified3_WithContextLines(t *testing.T) { + diff := strings.Join([]string{ + "diff --git a/foo.go b/foo.go", + "index 1234567..89abcde 100644", + "--- a/foo.go", + "+++ b/foo.go", + "@@ -8,7 +8,8 @@ func foo() {", + " \tline8", + " \tline9", + " \tline10", + "-\told", + "+\tnewA", + "+\tnewB", + " \tline12", + " \tline13", + " \tline14", + }, "\n") + + lines, err := NewChangedSourceLinesLoader("", []string{""}).Load(strings.NewReader(diff)) + + assert.NoError(t, err) + assert.Len(t, lines, 2) + // Hunk starts at new-file line 8; three context lines (8,9,10) precede the + // additions, so the first added line is 11. + assert.Equal(t, 11, lines[0].LineNumber) + assert.Equal(t, "\tnewA", lines[0].LineValue) + assert.Equal(t, 12, lines[1].LineNumber) + assert.Equal(t, "\tnewB", lines[1].LineValue) +} + +// A blank context line is emitted as a single space; it must still be counted so +// later line numbers stay correct. +func TestLoad_Unified3_BlankContextLine(t *testing.T) { + diff := strings.Join([]string{ + "+++ b/foo.go", + "@@ -1,3 +1,4 @@", + " first", + " ", + "+added", + " last", + }, "\n") + + lines, err := NewChangedSourceLinesLoader("", []string{""}).Load(strings.NewReader(diff)) + + assert.NoError(t, err) + assert.Len(t, lines, 1) + assert.Equal(t, 3, lines[0].LineNumber) + assert.Equal(t, "added", lines[0].LineValue) +} diff --git a/internal/test/mocks/property_getter.go b/internal/test/mocks/property_getter.go index 708966b..da646b2 100644 --- a/internal/test/mocks/property_getter.go +++ b/internal/test/mocks/property_getter.go @@ -11,6 +11,30 @@ func NewMockPropertyGetter() *MockPropertyGetter { } func (m *MockPropertyGetter) GetProperty(s string) (string, bool) { + // Properties a test did not explicitly stub resolve to ("", false), the same + // as os.LookupEnv for an unset variable. This keeps each test focused on the + // properties it cares about and means looking up a newer optional property + // (e.g. PARAMETER_DIFF_SOURCE) does not panic in tests written before it + // existed. Stubbed properties still go through testify so AssertExpectations + // continues to verify them. + if !m.hasExpectedCall(s) { + return "", false + } + args := m.Called(s) return args.Get(0).(string), args.Bool(1) } + +func (m *MockPropertyGetter) hasExpectedCall(s string) bool { + for _, c := range m.ExpectedCalls { + if c.Method != "GetProperty" || len(c.Arguments) != 1 { + continue + } + + if name, ok := c.Arguments[0].(string); ok && name == s { + return true + } + } + + return false +} From 94152fdc8e4ecf63dcf7551f080ada21cf3f452b Mon Sep 17 00:00:00 2001 From: Vishal Vaibhav Date: Fri, 19 Jun 2026 12:06:25 +0530 Subject: [PATCH 2/2] feat: move-diff-logic-within-the-plugin Drop the Vela start.sh entrypoint and run the plugin binary directly (ENTRYPOINT ["/plugin"]); the diff is now produced in-plugin (PARAMETER_DIFF_SOURCE=github) or piped on stdin. Also run the coverage job on fork PRs, tolerating the read-only-token comment 403 via continue-on-error. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/pr-coverage.yml | 13 +++++++++---- Dockerfile | 6 ++++-- scripts/start.sh | 30 ------------------------------ 3 files changed, 13 insertions(+), 36 deletions(-) delete mode 100755 scripts/start.sh diff --git a/.github/workflows/pr-coverage.yml b/.github/workflows/pr-coverage.yml index 7cfc12b..f3ecb15 100644 --- a/.github/workflows/pr-coverage.yml +++ b/.github/workflows/pr-coverage.yml @@ -14,9 +14,10 @@ permissions: jobs: coverage: runs-on: ubuntu-latest - # Fork PRs only get a read-only GITHUB_TOKEN (can't comment) and no secrets, - # so restrict to same-repo PRs to avoid a guaranteed failure on forks. - if: github.event.pull_request.head.repo.full_name == github.repository + # Runs on fork PRs too. Fork PRs only get a read-only GITHUB_TOKEN and no + # secrets, so the plugin can't post the PR comment there — it just prints + # coverage to the job log (the reporter no-ops the comment when the token / + # PR context is missing). steps: - name: Check out the repo uses: actions/checkout@v4 @@ -45,6 +46,11 @@ jobs: run: docker build -t pr-code-coverage:ci . - name: Report coverage on changed lines + # On fork PRs the GITHUB_TOKEN is read-only: the plugin can read the diff + # but the PR-comment POST gets a 403 and errors. Tolerate that on forks so + # the run still goes green with coverage printed to the log; same-repo PRs + # keep failing loudly on real errors. + continue-on-error: ${{ github.event.pull_request.head.repo.full_name != github.repository }} env: # Fetch the PR diff straight from the GitHub API instead of piping in a # local `git diff`. Note the API diff covers ALL changed files, not just @@ -73,5 +79,4 @@ jobs: -e REPOSITORY_NAME \ -v "${{ github.workspace }}:${{ github.workspace }}" \ -w "${{ github.workspace }}" \ - --entrypoint /plugin \ pr-code-coverage:ci diff --git a/Dockerfile b/Dockerfile index 5214a90..40c8679 100644 --- a/Dockerfile +++ b/Dockerfile @@ -13,5 +13,7 @@ FROM alpine:latest COPY --from=builder /go/src/github.com/target/pull-request-code-coverage/bin/plugin / RUN apk --no-cache add ca-certificates git bash openssh-client WORKDIR /root/ -COPY scripts/start.sh / -CMD ["/start.sh"] \ No newline at end of file +# Run the plugin directly. With PARAMETER_DIFF_SOURCE=github it fetches the PR +# diff from the GitHub API; for the stdin path, pipe a `git diff` into the +# container (docker run -i ... | git diff ...). +ENTRYPOINT ["/plugin"] \ No newline at end of file diff --git a/scripts/start.sh b/scripts/start.sh deleted file mode 100755 index d87678a..0000000 --- a/scripts/start.sh +++ /dev/null @@ -1,30 +0,0 @@ -#!/usr/bin/env bash - -set -Eeuo pipefail - - - -if [[ ! -f ~/.netrc ]] -then - echo "~/.netrc does not exist, creating..." - - cat >~/.netrc <