diff --git a/internal/reporter/gitlab.go b/internal/reporter/gitlab.go index 7b2a5cb3..389cedb0 100644 --- a/internal/reporter/gitlab.go +++ b/internal/reporter/gitlab.go @@ -359,17 +359,17 @@ func reportToGitLabDiscussion(pending PendingComment, diffs []*gitlab.MergeReque switch { case !ok: // No diffLine for this line, most likely unmodified ?. - d.Position.NewLine = gitlab.Ptr(dl.new) - d.Position.OldLine = gitlab.Ptr(dl.old) + d.Position.NewLine = gitlab.Ptr(pending.line) + d.Position.OldLine = gitlab.Ptr(pending.line) case pending.anchor == checks.AnchorBefore: // Comment on removed line. d.Position.OldLine = gitlab.Ptr(dl.old) case ok && !dl.wasModified: - // Commend on unmodified line. + // Comment on unmodified line. d.Position.NewLine = gitlab.Ptr(dl.new) d.Position.OldLine = gitlab.Ptr(dl.old) default: - // Commend on new or modified line. + // Comment on new or modified line. d.Position.NewLine = gitlab.Ptr(dl.new) } @@ -392,10 +392,37 @@ type diffLine struct { } func diffLineFor(lines []diffLine, line int) (diffLine, bool) { - for _, dl := range lines { + if len(lines) == 0 { + return diffLine{}, false + } + + for i, dl := range lines { if dl.new == line { return dl, true } + // Calculate unmodified line that does not present in the diff + if dl.new > line { + lastLines := dl + if i > 0 { + lastLines = lines[i-1] + } + gap := line - lastLines.new + return diffLine{ + old: lastLines.old + gap, + new: line, + wasModified: false, + }, true + } + } + // Calculate unmodified line that is greater than the last diff line + lastLines := lines[len(lines)-1] + if line > lastLines.new { + gap := line - lastLines.new + return diffLine{ + old: lastLines.old + gap, + new: line, + wasModified: false, + }, true } return diffLine{}, false } diff --git a/internal/reporter/gitlab_test.go b/internal/reporter/gitlab_test.go index de83b167..89290e1a 100644 --- a/internal/reporter/gitlab_test.go +++ b/internal/reporter/gitlab_test.go @@ -2,9 +2,11 @@ package reporter_test import ( "context" + "io" "log/slog" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" @@ -396,3 +398,207 @@ func TestGitLabReporter(t *testing.T) { }) } } + +func TestGitLabReporterCommentLine(t *testing.T) { + type testCaseT struct { + description string + problemLine int + expectedNewLine int + expectedOldLine int + anchor checks.Anchor + } + + p := parser.NewParser(false) + mockRules, _ := p.Parse([]byte(` +- record: target is down + expr: up == 0 +- record: sum errors + expr: sum(errors) by (job) +`)) + + multipleDiffs := `@@ -15,6 +15,7 @@ spec: + annotations: + foo: bar + description: some description ++ runbook_url: https://runbook.url + summary: summary text + expr: up == 0 + for: 10m +@@ -141,6 +142,5 @@ spec: + description: some description + summary: some summary + expr: sum(errors) by (job) +- for: 15m + labels: + severity: warning` + multipleDiffs = strings.ReplaceAll(multipleDiffs, "\n", "\\n") + multipleDiffs = strings.ReplaceAll(multipleDiffs, "\t", "\\t") + + errorHandler := func(err error) error { return err } + + testCases := []testCaseT{ + { + description: "comment on new line", + problemLine: 18, + expectedNewLine: 18, + }, + { + description: "comment on removed line", + problemLine: 145, + expectedOldLine: 145, + anchor: checks.AnchorBefore, + }, + { + description: "unmodified line before existing line in the diff", + problemLine: 10, + expectedNewLine: 10, + expectedOldLine: 10, + }, + { + description: "unmodified line that exists in the diff", + problemLine: 16, + expectedNewLine: 16, + expectedOldLine: 16, + }, + { + description: "unmodified line after added line and exists in the diff", + problemLine: 19, + expectedNewLine: 19, + expectedOldLine: 18, + }, + { + description: "unmodified line after added line and not exists in the diff", + problemLine: 23, + expectedNewLine: 23, + expectedOldLine: 22, + }, + { + description: "unmodified line before removed line and exists in the diff", + problemLine: 143, + expectedNewLine: 143, + expectedOldLine: 142, + }, + { + description: "unmodified line after removed line and exists in the diff", + problemLine: 145, + expectedNewLine: 145, + expectedOldLine: 145, + }, + { + description: "unmodified line after removed line and not exists in the diff", + problemLine: 148, + expectedNewLine: 148, + expectedOldLine: 148, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + slog.SetDefault(slogt.New(t)) + + srv := httptest.NewServer(getHTTPHandlerForCommentingLines( + tc.expectedNewLine, tc.expectedOldLine, multipleDiffs, t)) + defer srv.Close() + + r, err := reporter.NewGitLabReporter( + "v0.0.0", + "fakeBranch", + srv.URL, + time.Second, + "fakeToken", + 123, + 10, + ) + if err == nil { + summary := reporter.NewSummary([]reporter.Report{ + { + Path: discovery.Path{ + Name: "foo.txt", + SymlinkTarget: "foo.txt", + }, + ModifiedLines: []int{2}, + Rule: mockRules[1], + Problem: checks.Problem{ + Lines: parser.LineRange{ + First: tc.problemLine, + Last: tc.problemLine, + }, + Reporter: "mock", + Text: "syntax error", + Details: "syntax details", + Severity: checks.Fatal, + Anchor: tc.anchor, + }, + }, + }) + err = reporter.Submit(context.Background(), summary, r) + } + require.NoError(t, errorHandler(err)) + }) + } +} + +func getHTTPHandlerForCommentingLines(expectedNewLine, expectedOldLine int, diff string, t *testing.T) http.HandlerFunc { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + switch r.URL.Path { + case "/api/v4/user": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"id": 123}`)) + } + case "/api/v4/projects/123/merge_requests": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"iid":1}]`)) + } + case "/api/v4/projects/123/merge_requests/1/diffs": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"diff":"` + diff + `","new_path":"foo.txt","old_path":"foo.txt"}]`)) + } + case "/api/v4/projects/123/merge_requests/1/versions": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[ + {"id": 2,"head_commit_sha": "head","base_commit_sha": "base","start_commit_sha": "start"}, + {"id": 1,"head_commit_sha": "head","base_commit_sha": "base","start_commit_sha": "start"} + ]`)) + } + case "/api/v4/projects/123/merge_requests/1/discussions": + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[]`)) + } else { + body, _ := io.ReadAll(r.Body) + b := strings.TrimSpace(strings.TrimRight(string(body), "\n\t\r")) + + str := "" + if expectedNewLine != 0 { + str = `"new_line":` + strconv.Itoa(expectedNewLine) + } + if expectedOldLine != 0 { + if str != "" { + str += "," + } + str += `"old_line":` + strconv.Itoa(expectedOldLine) + } + + expected := `{ + "body":":stop_sign: **Fatal** reported by [pint](https://cloudflare.github.io/pint/) **mock** check.\n\n------\n\nsyntax error\n\nsyntax details\n\n------\n\n:information_source: To see documentation covering this check and instructions on how to resolve it [click here](https://cloudflare.github.io/pint/checks/mock.html).\n", + "position":{ + "base_sha":"base", + "head_sha":"head", + "start_sha":"start", + "new_path":"foo.txt", + "old_path":"foo.txt", + "position_type":"text", + ` + str + ` + } + }` + expected = strings.ReplaceAll(expected, "\n", "") + expected = strings.ReplaceAll(expected, "\t", "") + if b != expected { + t.Errorf("Unexpected comment: %s", b) + t.FailNow() + } + _, _ = w.Write([]byte(`{}`)) + } + } + }) +}