From 24ff72c8dc0d2b177635eeb8494473d263365616 Mon Sep 17 00:00:00 2001 From: "Alfaneti, Tinotenda" Date: Wed, 10 Sep 2025 15:57:20 +0100 Subject: [PATCH 1/5] #173 feat: configurable regexp for coverage-ignore --- .testcoverage.example.yml | 4 ++ README.md | 25 ++++++++ pkg/testcoverage/check.go | 19 +++--- pkg/testcoverage/check_test.go | 73 +++++++++++++++++++++++ pkg/testcoverage/config.go | 23 ++++---- pkg/testcoverage/config_test.go | 4 +- pkg/testcoverage/coverage/cover.go | 74 +++++++++++++++++------- pkg/testcoverage/coverage/cover_test.go | 35 +++++++++++ pkg/testcoverage/coverage/export_test.go | 15 +++-- pkg/testcoverage/coverage/types.go | 25 ++++++-- pkg/testcoverage/report.go | 47 ++++++++++++++- pkg/testcoverage/types.go | 29 ++++++---- 12 files changed, 306 insertions(+), 67 deletions(-) diff --git a/.testcoverage.example.yml b/.testcoverage.example.yml index 1292f48..6af690b 100644 --- a/.testcoverage.example.yml +++ b/.testcoverage.example.yml @@ -41,6 +41,10 @@ exclude: - \.pb\.go$ # excludes all protobuf generated files - ^pkg/bar # exclude package `pkg/bar` +# (optional; default false) +# When true, requires all coverage-ignore annotations to include explanatory comments +force-annotation-comment: false + # If specified, saves the current test coverage breakdown to this file. # # Typically, this breakdown is generated only for main (base) branches and diff --git a/README.md b/README.md index d8618f7..d6ad31f 100644 --- a/README.md +++ b/README.md @@ -175,6 +175,31 @@ func bar() { // coverage-ignore } ``` +#### Requiring Explanations for Ignored Coverage + +You can enforce that all `coverage-ignore` annotations must include explanatory comments. This helps ensure that developers document why specific code blocks are excluded from coverage. + +To enable this feature, add the following to your configuration: + +```yml +# Settings for coverage-ignore annotations +# When true, requires all coverage-ignore annotations to include explanatory comments +force-annotation-comment: false +``` + +When enabled, annotations without explanations will cause the check to fail with detailed error messages. + +Examples of valid annotations with explanations: +```go +if err != nil { // coverage-ignore - this error is handled by the caller + return err +} + +func handleSpecialCase() { // coverage-ignore - tested in integration tests + // ... +} +``` + ## Generate Coverage Badge You can easily generate a stylish coverage badge for your repository and embed it in your markdown files. Here’s an example badge: ![coverage](https://raw.githubusercontent.com/vladopajic/go-test-coverage/badges/.badges/main/coverage.svg) diff --git a/pkg/testcoverage/check.go b/pkg/testcoverage/check.go index 85623b1..3fe3af3 100644 --- a/pkg/testcoverage/check.go +++ b/pkg/testcoverage/check.go @@ -92,9 +92,10 @@ func reportForHuman(w io.Writer, result AnalyzeResult) string { func GenerateCoverageStats(cfg Config) ([]coverage.Stats, error) { return coverage.GenerateCoverageStats(coverage.Config{ //nolint:wrapcheck // err wrapped above - Profiles: strings.Split(cfg.Profile, ","), - ExcludePaths: cfg.Exclude.Paths, - SourceDir: cfg.SourceDir, + Profiles: strings.Split(cfg.Profile, ","), + ExcludePaths: cfg.Exclude.Paths, + SourceDir: cfg.SourceDir, + ForceAnnotationComment: cfg.ForceAnnotationComment, }) } @@ -112,11 +113,13 @@ func Analyze(cfg Config, current, base []coverage.Stats) AnalyzeResult { PackagesBelowThreshold: checkCoverageStatsBelowThreshold( makePackageStats(current), thr.Package, overrideRules, ), - FilesWithUncoveredLines: coverage.StatsFilterWithUncoveredLines(current), - TotalStats: coverage.StatsCalcTotal(current), - HasBaseBreakdown: len(base) > 0, - Diff: calculateStatsDiff(current, base), - DiffPercentage: TotalPercentageDiff(current, base), + FilesWithUncoveredLines: coverage.StatsFilterWithUncoveredLines(current), + FilesWithMissingExplanations: coverage.StatsFilterWithMissingExplanations(current), + TotalStats: coverage.StatsCalcTotal(current), + HasBaseBreakdown: len(base) > 0, + Diff: calculateStatsDiff(current, base), + DiffPercentage: TotalPercentageDiff(current, base), + RequireIgnoreExplanation: cfg.ForceAnnotationComment, } } diff --git a/pkg/testcoverage/check_test.go b/pkg/testcoverage/check_test.go index e610441..2b465a4 100644 --- a/pkg/testcoverage/check_test.go +++ b/pkg/testcoverage/check_test.go @@ -265,6 +265,42 @@ func TestCheck(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "failed to load base coverage breakdown") }) + + t.Run("valid profile - fail when missing explanations", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + cfg := Config{ + Profile: profileOK, + SourceDir: sourceDir, + ForceAnnotationComment: true, + } + pass, err := Check(buf, cfg) + assert.False(t, pass) + assert.NoError(t, err) + + // Should report missing explanations + assert.Contains(t, buf.String(), "Files with missing explanations for coverage-ignore") + assert.Contains(t, buf.String(), "Missing explanation for coverage-ignore") + }) + + t.Run("valid profile - pass when not checking for explanations", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + cfg := Config{ + Profile: profileOK, + SourceDir: sourceDir, + ForceAnnotationComment: false, + } + pass, err := Check(buf, cfg) + assert.True(t, pass) + assert.NoError(t, err) + + // Should not report missing explanations + assert.NotContains(t, buf.String(), "Files with missing explanations for coverage-ignore") + assert.NotContains(t, buf.String(), "Missing explanation for coverage-ignore") + }) } func TestCheckDiff(t *testing.T) { @@ -612,6 +648,43 @@ func Test_Analyze(t *testing.T) { assert.True(t, result.MeetsDiffThreshold()) assert.Equal(t, 0.01, result.DiffPercentage) //nolint:testifylint //relax }) + + t.Run("missing explanations for coverage-ignore", func(t *testing.T) { + t.Parallel() + + // Create a stat with missing explanations + stats := []coverage.Stats{ + { + Name: prefix + "/foo.go", + Total: 100, + Covered: 100, + AnnotationsWithoutComments: []coverage.Extent{ + { + StartLine: 10, + EndLine: 10, + }, + }, + }, + } + + // When explanations are not required, the check should pass + cfg := Config{ + ForceAnnotationComment: false, + } + result := Analyze(cfg, stats, nil) + assert.True(t, result.Pass()) + assert.NotEmpty(t, result.FilesWithMissingExplanations) + assert.False(t, result.RequireIgnoreExplanation) + + // When explanations are required, the check should fail + cfg = Config{ + ForceAnnotationComment: true, + } + result = Analyze(cfg, stats, nil) + assert.False(t, result.Pass()) + assert.NotEmpty(t, result.FilesWithMissingExplanations) + assert.True(t, result.RequireIgnoreExplanation) + }) } func TestLoadBaseCoverageBreakdown(t *testing.T) { diff --git a/pkg/testcoverage/config.go b/pkg/testcoverage/config.go index 929b9c1..1090931 100644 --- a/pkg/testcoverage/config.go +++ b/pkg/testcoverage/config.go @@ -24,17 +24,18 @@ var ( ) type Config struct { - Profile string `yaml:"profile"` - Debug bool `yaml:"-"` - LocalPrefixDeprecated string `yaml:"-"` - SourceDir string `yaml:"-"` - Threshold Threshold `yaml:"threshold"` - Override []Override `yaml:"override,omitempty"` - Exclude Exclude `yaml:"exclude"` - BreakdownFileName string `yaml:"breakdown-file-name"` - GithubActionOutput bool `yaml:"github-action-output"` - Diff Diff `yaml:"diff"` - Badge Badge `yaml:"-"` + Profile string `yaml:"profile"` + Debug bool `yaml:"-"` + LocalPrefixDeprecated string `yaml:"-"` + SourceDir string `yaml:"-"` + Threshold Threshold `yaml:"threshold"` + Override []Override `yaml:"override,omitempty"` + Exclude Exclude `yaml:"exclude"` + BreakdownFileName string `yaml:"breakdown-file-name"` + GithubActionOutput bool `yaml:"github-action-output"` + Diff Diff `yaml:"diff"` + Badge Badge `yaml:"-"` + ForceAnnotationComment bool `yaml:"force-annotation-comment"` } type Threshold struct { diff --git a/pkg/testcoverage/config_test.go b/pkg/testcoverage/config_test.go index 5bb3b55..6cb88e9 100644 --- a/pkg/testcoverage/config_test.go +++ b/pkg/testcoverage/config_test.go @@ -251,7 +251,8 @@ func nonZeroConfig() Config { BaseBreakdownFileName: "breakdown.testcoverage", Threshold: ptr(-1.01), }, - GithubActionOutput: true, + GithubActionOutput: true, + ForceAnnotationComment: false, } } @@ -265,6 +266,7 @@ threshold: override: - threshold: 99 path: pathToFile +force-annotation-comment: false exclude: paths: - path1 diff --git a/pkg/testcoverage/coverage/cover.go b/pkg/testcoverage/coverage/cover.go index 9d0566a..5f12dc0 100644 --- a/pkg/testcoverage/coverage/cover.go +++ b/pkg/testcoverage/coverage/cover.go @@ -20,10 +20,19 @@ import ( const IgnoreText = "coverage-ignore" +// IgnoreAnnotation represents a coverage-ignore annotation in the code +type IgnoreAnnotation struct { + Extent extent + HasExplanation bool + FilePath string + LineNumber int +} + type Config struct { - Profiles []string - ExcludePaths []string - SourceDir string + Profiles []string + ExcludePaths []string + SourceDir string + ForceAnnotationComment bool } func GenerateCoverageStats(cfg Config) ([]Stats, error) { @@ -52,7 +61,7 @@ func GenerateCoverageStats(cfg Config) ([]Stats, error) { continue // this file is excluded } - s, err := coverageForFile(profile, fi) + s, err := coverageForFile(profile, fi, cfg.ForceAnnotationComment) if err != nil { return nil, err } @@ -78,25 +87,28 @@ func GenerateCoverageStats(cfg Config) ([]Stats, error) { return fileStats, nil } -func coverageForFile(profile *cover.Profile, fi fileInfo) (Stats, error) { +func coverageForFile(profile *cover.Profile, fi fileInfo, forceComment bool) (Stats, error) { source, err := os.ReadFile(fi.path) - if err != nil { // coverage-ignore + if err != nil { // coverage-ignore - handle file read errors return Stats{}, fmt.Errorf("failed reading file source [%s]: %w", fi.path, err) } funcs, blocks, err := findFuncsAndBlocks(source) - if err != nil { // coverage-ignore + if err != nil { // coverage-ignore - handle parsing errors return Stats{}, err } - annotations, err := findAnnotations(source) - if err != nil { // coverage-ignore + annotations, withoutComment, err := findAnnotationsWithComment(source, forceComment) + if err != nil { // coverage-ignore - handle annotation parsing errors return Stats{}, err } s := sumCoverage(profile, funcs, blocks, annotations) s.Name = fi.name + s.AnnotationsWithoutComments = make([]Extent, len(withoutComment)) + copy(s.AnnotationsWithoutComments, withoutComment) + return s, nil } @@ -251,23 +263,48 @@ func findFilePathMatchingSearch(files *[]fileInfo, search string) string { return path } -func findAnnotations(source []byte) ([]extent, error) { +// findAnnotationsWithExplanation finds coverage-ignore annotations and checks for explanations +func findAnnotationsWithComment(source []byte, forceComment bool) ([]extent, []extent, error) { fset := token.NewFileSet() node, err := parser.ParseFile(fset, "", source, parser.ParseComments) if err != nil { - return nil, fmt.Errorf("can't parse comments: %w", err) + return nil, nil, fmt.Errorf("can't parse comments: %w", err) } - var res []extent + var validAnnotations []extent + + var annotationsWithoutComment []extent for _, c := range node.Comments { - if strings.Contains(c.Text(), IgnoreText) { - res = append(res, newExtent(fset, c)) + if !strings.Contains(c.Text(), IgnoreText) { + continue // does not have annotation continue to next comment + } + + if forceComment { + if hasComment(c.Text()) { + validAnnotations = append(validAnnotations, newExtent(fset, c)) + } else { + annotationsWithoutComment = append(annotationsWithoutComment, newExtent(fset, c)) + } + } else { + validAnnotations = append(validAnnotations, newExtent(fset, c)) } } - return res, nil + return validAnnotations, annotationsWithoutComment, nil +} + +// hasComment checks if the coverage-ignore annotation has an explanation comment +func hasComment(text string) bool { + // coverage-ignore should be followed by additional text to be considered an explanation + trimmedComment := strings.TrimSpace(text) + return len(trimmedComment) > len(IgnoreText) +} + +func findAnnotations(source []byte) ([]extent, error) { + extents, _, err := findAnnotationsWithComment(source, false) + return extents, err } func findFuncsAndBlocks(source []byte) ([]extent, []extent, error) { @@ -317,12 +354,7 @@ func (v *visitor) addBlock(n ast.Node) { v.blocks = append(v.blocks, newExtent(v.fset, n)) } -type extent struct { - StartLine int - StartCol int - EndLine int - EndCol int -} +type extent = Extent func newExtent(fset *token.FileSet, n ast.Node) extent { start := fset.Position(n.Pos()) diff --git a/pkg/testcoverage/coverage/cover_test.go b/pkg/testcoverage/coverage/cover_test.go index 61c55f5..9a75ca7 100644 --- a/pkg/testcoverage/coverage/cover_test.go +++ b/pkg/testcoverage/coverage/cover_test.go @@ -143,6 +143,41 @@ func Test_findAnnotations(t *testing.T) { assert.Equal(t, []int{3, 5}, pluckStartLine(comments)) } +func Test_findAnnotationsWithComment(t *testing.T) { + t.Parallel() + + _, _, err := FindAnnotationsWithComment(nil, true) + assert.Error(t, err) + + _, _, err = FindAnnotationsWithComment([]byte{}, true) + assert.Error(t, err) + + const source = ` + package foo + func foo() int { // coverage-ignore + a := 0 + // This is a regular comment + for i := range 10 { // coverage-ignore - this has an explanation + a += i + } + if a > 5 { // coverage-ignore + a = 5 + } + return a + } + ` + + validAnnotations, withoutComment, err := FindAnnotationsWithComment([]byte(source), true) + assert.NoError(t, err) + assert.Equal(t, []int{6}, pluckStartLine(validAnnotations)) + assert.Equal(t, []int{3, 9}, pluckStartLine(withoutComment)) + + validAnnotations, withoutComment, err = FindAnnotationsWithComment([]byte(source), false) + assert.NoError(t, err) + assert.Equal(t, []int{3, 6, 9}, pluckStartLine(validAnnotations)) + assert.Empty(t, withoutComment) +} + func Test_findFuncs(t *testing.T) { t.Parallel() diff --git a/pkg/testcoverage/coverage/export_test.go b/pkg/testcoverage/coverage/export_test.go index 34be596..591b524 100644 --- a/pkg/testcoverage/coverage/export_test.go +++ b/pkg/testcoverage/coverage/export_test.go @@ -1,12 +1,11 @@ package coverage var ( - FindFileCreator = findFileCreator - FindAnnotations = findAnnotations - FindFuncsAndBlocks = findFuncsAndBlocks - ParseProfiles = parseProfiles - SumCoverage = sumCoverage - FindGoModFile = findGoModFile + FindFileCreator = findFileCreator + FindAnnotations = findAnnotations + FindAnnotationsWithComment = findAnnotationsWithComment + FindFuncsAndBlocks = findFuncsAndBlocks + ParseProfiles = parseProfiles + SumCoverage = sumCoverage + FindGoModFile = findGoModFile ) - -type Extent = extent diff --git a/pkg/testcoverage/coverage/types.go b/pkg/testcoverage/coverage/types.go index 80aaef7..559c48c 100644 --- a/pkg/testcoverage/coverage/types.go +++ b/pkg/testcoverage/coverage/types.go @@ -10,12 +10,20 @@ import ( "strings" ) +type Extent struct { + StartLine int + StartCol int + EndLine int + EndCol int +} + type Stats struct { - Name string - Total int64 - Covered int64 - Threshold int - UncoveredLines []int + Name string + Total int64 + Covered int64 + Threshold int + UncoveredLines []int + AnnotationsWithoutComments []Extent } func (s Stats) UncoveredLinesCount() int { @@ -149,6 +157,13 @@ func StatsFilterWithCoveredLines(stats []Stats) []Stats { }) } +// StatsFilterWithMissingExplanations returns stats that have missing explanations +func StatsFilterWithMissingExplanations(stats []Stats) []Stats { + return filter(stats, func(s Stats) bool { + return len(s.AnnotationsWithoutComments) > 0 + }) +} + func filter[T any](slice []T, predicate func(T) bool) []T { var result []T diff --git a/pkg/testcoverage/report.go b/pkg/testcoverage/report.go index 12eacb3..c5793fd 100644 --- a/pkg/testcoverage/report.go +++ b/pkg/testcoverage/report.go @@ -20,6 +20,7 @@ func ReportForHuman(w io.Writer, result AnalyzeResult) { reportCoverage(out, result) reportUncoveredLines(out, result) + reportMissingExplanations(out, result) reportDiff(out, result) } @@ -87,6 +88,34 @@ func reportUncoveredLines(w io.Writer, result AnalyzeResult) { fmt.Fprintf(tabber, "\n") } +func reportMissingExplanations(w io.Writer, result AnalyzeResult) { + if !result.RequireIgnoreExplanation || len(result.FilesWithMissingExplanations) == 0 { + return + } + + tabber := tabwriter.NewWriter(w, 1, 8, 2, '\t', 0) //nolint:mnd // relax + defer tabber.Flush() + + fmt.Fprintf(tabber, "\nFiles with missing explanations for coverage-ignore annotations:") + fmt.Fprintf(tabber, "\n file:\tline numbers:") + + for _, stats := range result.FilesWithMissingExplanations { + if len(stats.AnnotationsWithoutComments) > 0 { + fmt.Fprintf(tabber, "\n %s\t", stats.Name) + + separator := "" + for _, ann := range stats.AnnotationsWithoutComments { + fmt.Fprintf(tabber, "%s%d", separator, ann.StartLine) + separator = ", " + } + } + } + + fmt.Fprintf(tabber, "\n") + + fmt.Fprintf(w, "Missing explanation for coverage-ignore: add an explanation\n") +} + //nolint:lll // relax func reportDiff(w io.Writer, result AnalyzeResult) { if !result.HasBaseBreakdown { @@ -126,7 +155,7 @@ func reportDiff(w io.Writer, result AnalyzeResult) { fmt.Fprintf(tabber, "\n") } -func ReportForGithubAction(w io.Writer, result AnalyzeResult) { +func ReportForGithubAction(w io.Writer, result AnalyzeResult) { //nolint:maintidx // relax out := bufio.NewWriter(w) defer out.Flush() @@ -163,6 +192,22 @@ func ReportForGithubAction(w io.Writer, result AnalyzeResult) { ) reportError(title, msg) } + + // Report missing explanations for coverage-ignore annotations + if result.RequireIgnoreExplanation { + for _, stats := range result.FilesWithMissingExplanations { + if len(stats.AnnotationsWithoutComments) > 0 { + for _, ann := range stats.AnnotationsWithoutComments { + title := "Missing explanation for coverage-ignore" + msg := title + ": add an explanation after the coverage-ignore annotation" + + file := stats.Name + lineNumber := ann.StartLine + fmt.Fprintf(out, "::error file=%s,title=%s,line=%d::%s\n", file, title, lineNumber, msg) + } + } + } + } } func reportGHWarning(out io.Writer, title, msg string) { // coverage-ignore diff --git a/pkg/testcoverage/types.go b/pkg/testcoverage/types.go index 2ff9b77..9fc310b 100644 --- a/pkg/testcoverage/types.go +++ b/pkg/testcoverage/types.go @@ -10,24 +10,29 @@ import ( ) type AnalyzeResult struct { - Threshold Threshold - DiffThreshold *float64 - FilesBelowThreshold []coverage.Stats - PackagesBelowThreshold []coverage.Stats - FilesWithUncoveredLines []coverage.Stats - TotalStats coverage.Stats - HasBaseBreakdown bool - Diff []FileCoverageDiff - DiffPercentage float64 - HasFileOverrides bool - HasPackageOverrides bool + Threshold Threshold + DiffThreshold *float64 + FilesBelowThreshold []coverage.Stats + PackagesBelowThreshold []coverage.Stats + FilesWithUncoveredLines []coverage.Stats + FilesWithMissingExplanations []coverage.Stats + TotalStats coverage.Stats + HasBaseBreakdown bool + Diff []FileCoverageDiff + DiffPercentage float64 + HasFileOverrides bool + HasPackageOverrides bool + RequireIgnoreExplanation bool } func (r *AnalyzeResult) Pass() bool { + hasNoMissingExplanations := !r.RequireIgnoreExplanation || len(r.FilesWithMissingExplanations) == 0 + return r.MeetsTotalCoverage() && len(r.FilesBelowThreshold) == 0 && len(r.PackagesBelowThreshold) == 0 && - r.MeetsDiffThreshold() + r.MeetsDiffThreshold() && + hasNoMissingExplanations } func (r *AnalyzeResult) MeetsDiffThreshold() bool { From 4f01e64c5a175896cdb250bd1cd85c5119fee68e Mon Sep 17 00:00:00 2001 From: "Alfaneti, Tinotenda" Date: Fri, 19 Sep 2025 23:42:38 +0100 Subject: [PATCH 2/5] #173 feat: Update README and make annotation datatype to []int --- README.md | 29 ++++-------------------- pkg/testcoverage/check_test.go | 13 ++++------- pkg/testcoverage/coverage/cover.go | 21 ++++++++--------- pkg/testcoverage/coverage/export_test.go | 2 ++ pkg/testcoverage/coverage/types.go | 9 +------- pkg/testcoverage/report.go | 4 ++-- 6 files changed, 23 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index d6ad31f..8e317fe 100644 --- a/README.md +++ b/README.md @@ -125,6 +125,10 @@ exclude: - \.pb\.go$ # excludes all protobuf generated files - ^pkg/bar # exclude package `pkg/bar` +# (optional; default false) +# When true, requires all coverage-ignore annotations to include explanatory comments +force-annotation-comment: false + # If specified, saves the current test coverage breakdown to this file. # # Typically, this breakdown is generated only for main (base) branches and @@ -175,31 +179,6 @@ func bar() { // coverage-ignore } ``` -#### Requiring Explanations for Ignored Coverage - -You can enforce that all `coverage-ignore` annotations must include explanatory comments. This helps ensure that developers document why specific code blocks are excluded from coverage. - -To enable this feature, add the following to your configuration: - -```yml -# Settings for coverage-ignore annotations -# When true, requires all coverage-ignore annotations to include explanatory comments -force-annotation-comment: false -``` - -When enabled, annotations without explanations will cause the check to fail with detailed error messages. - -Examples of valid annotations with explanations: -```go -if err != nil { // coverage-ignore - this error is handled by the caller - return err -} - -func handleSpecialCase() { // coverage-ignore - tested in integration tests - // ... -} -``` - ## Generate Coverage Badge You can easily generate a stylish coverage badge for your repository and embed it in your markdown files. Here’s an example badge: ![coverage](https://raw.githubusercontent.com/vladopajic/go-test-coverage/badges/.badges/main/coverage.svg) diff --git a/pkg/testcoverage/check_test.go b/pkg/testcoverage/check_test.go index 2b465a4..3029ad5 100644 --- a/pkg/testcoverage/check_test.go +++ b/pkg/testcoverage/check_test.go @@ -655,15 +655,10 @@ func Test_Analyze(t *testing.T) { // Create a stat with missing explanations stats := []coverage.Stats{ { - Name: prefix + "/foo.go", - Total: 100, - Covered: 100, - AnnotationsWithoutComments: []coverage.Extent{ - { - StartLine: 10, - EndLine: 10, - }, - }, + Name: prefix + "/foo.go", + Total: 100, + Covered: 100, + AnnotationsWithoutComments: []int{10}, }, } diff --git a/pkg/testcoverage/coverage/cover.go b/pkg/testcoverage/coverage/cover.go index 5f12dc0..8a25ef2 100644 --- a/pkg/testcoverage/coverage/cover.go +++ b/pkg/testcoverage/coverage/cover.go @@ -20,14 +20,6 @@ import ( const IgnoreText = "coverage-ignore" -// IgnoreAnnotation represents a coverage-ignore annotation in the code -type IgnoreAnnotation struct { - Extent extent - HasExplanation bool - FilePath string - LineNumber int -} - type Config struct { Profiles []string ExcludePaths []string @@ -106,8 +98,10 @@ func coverageForFile(profile *cover.Profile, fi fileInfo, forceComment bool) (St s := sumCoverage(profile, funcs, blocks, annotations) s.Name = fi.name - s.AnnotationsWithoutComments = make([]Extent, len(withoutComment)) - copy(s.AnnotationsWithoutComments, withoutComment) + s.AnnotationsWithoutComments = make([]int, len(withoutComment)) + for i, extent := range withoutComment { + s.AnnotationsWithoutComments[i] = extent.StartLine + } return s, nil } @@ -354,7 +348,12 @@ func (v *visitor) addBlock(n ast.Node) { v.blocks = append(v.blocks, newExtent(v.fset, n)) } -type extent = Extent +type extent struct { + StartLine int + StartCol int + EndLine int + EndCol int +} func newExtent(fset *token.FileSet, n ast.Node) extent { start := fset.Position(n.Pos()) diff --git a/pkg/testcoverage/coverage/export_test.go b/pkg/testcoverage/coverage/export_test.go index 591b524..21170dd 100644 --- a/pkg/testcoverage/coverage/export_test.go +++ b/pkg/testcoverage/coverage/export_test.go @@ -9,3 +9,5 @@ var ( SumCoverage = sumCoverage FindGoModFile = findGoModFile ) + +type Extent = extent diff --git a/pkg/testcoverage/coverage/types.go b/pkg/testcoverage/coverage/types.go index 559c48c..789ccc1 100644 --- a/pkg/testcoverage/coverage/types.go +++ b/pkg/testcoverage/coverage/types.go @@ -10,20 +10,13 @@ import ( "strings" ) -type Extent struct { - StartLine int - StartCol int - EndLine int - EndCol int -} - type Stats struct { Name string Total int64 Covered int64 Threshold int UncoveredLines []int - AnnotationsWithoutComments []Extent + AnnotationsWithoutComments []int } func (s Stats) UncoveredLinesCount() int { diff --git a/pkg/testcoverage/report.go b/pkg/testcoverage/report.go index c5793fd..036f2c1 100644 --- a/pkg/testcoverage/report.go +++ b/pkg/testcoverage/report.go @@ -105,7 +105,7 @@ func reportMissingExplanations(w io.Writer, result AnalyzeResult) { separator := "" for _, ann := range stats.AnnotationsWithoutComments { - fmt.Fprintf(tabber, "%s%d", separator, ann.StartLine) + fmt.Fprintf(tabber, "%s%d", separator, ann) separator = ", " } } @@ -202,7 +202,7 @@ func ReportForGithubAction(w io.Writer, result AnalyzeResult) { //nolint:maintid msg := title + ": add an explanation after the coverage-ignore annotation" file := stats.Name - lineNumber := ann.StartLine + lineNumber := ann fmt.Fprintf(out, "::error file=%s,title=%s,line=%d::%s\n", file, title, lineNumber, msg) } } From 20f15e9aedfb786b6dacdf3b41d2ef127d8b4a59 Mon Sep 17 00:00:00 2001 From: "Alfaneti, Tinotenda" Date: Sat, 20 Sep 2025 00:03:33 +0100 Subject: [PATCH 3/5] #173 feat: Simplify by removing RequireIgnoreExplanation --- pkg/testcoverage/check.go | 8 ++++++-- pkg/testcoverage/check_test.go | 6 +----- pkg/testcoverage/report.go | 24 ++++++++++-------------- pkg/testcoverage/types.go | 5 +---- 4 files changed, 18 insertions(+), 25 deletions(-) diff --git a/pkg/testcoverage/check.go b/pkg/testcoverage/check.go index 3fe3af3..e8742e8 100644 --- a/pkg/testcoverage/check.go +++ b/pkg/testcoverage/check.go @@ -104,6 +104,11 @@ func Analyze(cfg Config, current, base []coverage.Stats) AnalyzeResult { overrideRules := compileOverridePathRules(cfg) hasFileOverrides, hasPackageOverrides := detectOverrides(cfg.Override) + var filesWithMissingExplanations []coverage.Stats + if cfg.ForceAnnotationComment { + filesWithMissingExplanations = coverage.StatsFilterWithMissingExplanations(current) + } + return AnalyzeResult{ Threshold: thr, DiffThreshold: cfg.Diff.Threshold, @@ -114,12 +119,11 @@ func Analyze(cfg Config, current, base []coverage.Stats) AnalyzeResult { makePackageStats(current), thr.Package, overrideRules, ), FilesWithUncoveredLines: coverage.StatsFilterWithUncoveredLines(current), - FilesWithMissingExplanations: coverage.StatsFilterWithMissingExplanations(current), + FilesWithMissingExplanations: filesWithMissingExplanations, TotalStats: coverage.StatsCalcTotal(current), HasBaseBreakdown: len(base) > 0, Diff: calculateStatsDiff(current, base), DiffPercentage: TotalPercentageDiff(current, base), - RequireIgnoreExplanation: cfg.ForceAnnotationComment, } } diff --git a/pkg/testcoverage/check_test.go b/pkg/testcoverage/check_test.go index 3029ad5..8779ef9 100644 --- a/pkg/testcoverage/check_test.go +++ b/pkg/testcoverage/check_test.go @@ -281,7 +281,6 @@ func TestCheck(t *testing.T) { // Should report missing explanations assert.Contains(t, buf.String(), "Files with missing explanations for coverage-ignore") - assert.Contains(t, buf.String(), "Missing explanation for coverage-ignore") }) t.Run("valid profile - pass when not checking for explanations", func(t *testing.T) { @@ -299,7 +298,6 @@ func TestCheck(t *testing.T) { // Should not report missing explanations assert.NotContains(t, buf.String(), "Files with missing explanations for coverage-ignore") - assert.NotContains(t, buf.String(), "Missing explanation for coverage-ignore") }) } @@ -668,8 +666,7 @@ func Test_Analyze(t *testing.T) { } result := Analyze(cfg, stats, nil) assert.True(t, result.Pass()) - assert.NotEmpty(t, result.FilesWithMissingExplanations) - assert.False(t, result.RequireIgnoreExplanation) + assert.Empty(t, result.FilesWithMissingExplanations) // When explanations are required, the check should fail cfg = Config{ @@ -678,7 +675,6 @@ func Test_Analyze(t *testing.T) { result = Analyze(cfg, stats, nil) assert.False(t, result.Pass()) assert.NotEmpty(t, result.FilesWithMissingExplanations) - assert.True(t, result.RequireIgnoreExplanation) }) } diff --git a/pkg/testcoverage/report.go b/pkg/testcoverage/report.go index 036f2c1..f4591c7 100644 --- a/pkg/testcoverage/report.go +++ b/pkg/testcoverage/report.go @@ -89,7 +89,7 @@ func reportUncoveredLines(w io.Writer, result AnalyzeResult) { } func reportMissingExplanations(w io.Writer, result AnalyzeResult) { - if !result.RequireIgnoreExplanation || len(result.FilesWithMissingExplanations) == 0 { + if len(result.FilesWithMissingExplanations) == 0 { return } @@ -112,8 +112,6 @@ func reportMissingExplanations(w io.Writer, result AnalyzeResult) { } fmt.Fprintf(tabber, "\n") - - fmt.Fprintf(w, "Missing explanation for coverage-ignore: add an explanation\n") } //nolint:lll // relax @@ -194,17 +192,15 @@ func ReportForGithubAction(w io.Writer, result AnalyzeResult) { //nolint:maintid } // Report missing explanations for coverage-ignore annotations - if result.RequireIgnoreExplanation { - for _, stats := range result.FilesWithMissingExplanations { - if len(stats.AnnotationsWithoutComments) > 0 { - for _, ann := range stats.AnnotationsWithoutComments { - title := "Missing explanation for coverage-ignore" - msg := title + ": add an explanation after the coverage-ignore annotation" - - file := stats.Name - lineNumber := ann - fmt.Fprintf(out, "::error file=%s,title=%s,line=%d::%s\n", file, title, lineNumber, msg) - } + for _, stats := range result.FilesWithMissingExplanations { + if len(stats.AnnotationsWithoutComments) > 0 { + for _, ann := range stats.AnnotationsWithoutComments { + title := "Missing explanation for coverage-ignore" + msg := title + ": add an explanation after the coverage-ignore annotation" + + file := stats.Name + lineNumber := ann + fmt.Fprintf(out, "::error file=%s,title=%s,line=%d::%s\n", file, title, lineNumber, msg) } } } diff --git a/pkg/testcoverage/types.go b/pkg/testcoverage/types.go index 9fc310b..f4496d5 100644 --- a/pkg/testcoverage/types.go +++ b/pkg/testcoverage/types.go @@ -22,17 +22,14 @@ type AnalyzeResult struct { DiffPercentage float64 HasFileOverrides bool HasPackageOverrides bool - RequireIgnoreExplanation bool } func (r *AnalyzeResult) Pass() bool { - hasNoMissingExplanations := !r.RequireIgnoreExplanation || len(r.FilesWithMissingExplanations) == 0 - return r.MeetsTotalCoverage() && len(r.FilesBelowThreshold) == 0 && len(r.PackagesBelowThreshold) == 0 && r.MeetsDiffThreshold() && - hasNoMissingExplanations + len(r.FilesWithMissingExplanations) == 0 } func (r *AnalyzeResult) MeetsDiffThreshold() bool { From 96a39cbe4528f51c667c7d2c2f59ec5666c1d19c Mon Sep 17 00:00:00 2001 From: "Alfaneti, Tinotenda" Date: Sat, 20 Sep 2025 00:13:41 +0100 Subject: [PATCH 4/5] #173 feat: Update to use early return pattern --- pkg/testcoverage/report.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/testcoverage/report.go b/pkg/testcoverage/report.go index f4591c7..6fcbf5a 100644 --- a/pkg/testcoverage/report.go +++ b/pkg/testcoverage/report.go @@ -100,14 +100,16 @@ func reportMissingExplanations(w io.Writer, result AnalyzeResult) { fmt.Fprintf(tabber, "\n file:\tline numbers:") for _, stats := range result.FilesWithMissingExplanations { - if len(stats.AnnotationsWithoutComments) > 0 { - fmt.Fprintf(tabber, "\n %s\t", stats.Name) + if len(stats.AnnotationsWithoutComments) == 0 { + continue + } - separator := "" - for _, ann := range stats.AnnotationsWithoutComments { - fmt.Fprintf(tabber, "%s%d", separator, ann) - separator = ", " - } + fmt.Fprintf(tabber, "\n %s\t", stats.Name) + + separator := "" + for _, ann := range stats.AnnotationsWithoutComments { + fmt.Fprintf(tabber, "%s%d", separator, ann) + separator = ", " } } From 07034d5fafce8d92bb969a5ca623410698168c80 Mon Sep 17 00:00:00 2001 From: "Alfaneti, Tinotenda" Date: Sat, 20 Sep 2025 00:53:22 +0100 Subject: [PATCH 5/5] #173 feat: Address pr review comments - remove findAnnotationsWithComments --- pkg/testcoverage/coverage/cover.go | 17 ++--- pkg/testcoverage/coverage/cover_test.go | 14 ++-- pkg/testcoverage/coverage/export_test.go | 13 ++-- pkg/testcoverage/report.go | 2 +- pkg/testcoverage/report_test.go | 97 ++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 26 deletions(-) diff --git a/pkg/testcoverage/coverage/cover.go b/pkg/testcoverage/coverage/cover.go index 8a25ef2..d608192 100644 --- a/pkg/testcoverage/coverage/cover.go +++ b/pkg/testcoverage/coverage/cover.go @@ -81,17 +81,17 @@ func GenerateCoverageStats(cfg Config) ([]Stats, error) { func coverageForFile(profile *cover.Profile, fi fileInfo, forceComment bool) (Stats, error) { source, err := os.ReadFile(fi.path) - if err != nil { // coverage-ignore - handle file read errors + if err != nil { // coverage-ignore return Stats{}, fmt.Errorf("failed reading file source [%s]: %w", fi.path, err) } funcs, blocks, err := findFuncsAndBlocks(source) - if err != nil { // coverage-ignore - handle parsing errors + if err != nil { // coverage-ignore return Stats{}, err } - annotations, withoutComment, err := findAnnotationsWithComment(source, forceComment) - if err != nil { // coverage-ignore - handle annotation parsing errors + annotations, withoutComment, err := findAnnotations(source, forceComment) + if err != nil { // coverage-ignore return Stats{}, err } @@ -257,8 +257,8 @@ func findFilePathMatchingSearch(files *[]fileInfo, search string) string { return path } -// findAnnotationsWithExplanation finds coverage-ignore annotations and checks for explanations -func findAnnotationsWithComment(source []byte, forceComment bool) ([]extent, []extent, error) { +// findAnnotations finds coverage-ignore annotations and checks for explanations +func findAnnotations(source []byte, forceComment bool) ([]extent, []extent, error) { fset := token.NewFileSet() node, err := parser.ParseFile(fset, "", source, parser.ParseComments) @@ -296,11 +296,6 @@ func hasComment(text string) bool { return len(trimmedComment) > len(IgnoreText) } -func findAnnotations(source []byte) ([]extent, error) { - extents, _, err := findAnnotationsWithComment(source, false) - return extents, err -} - func findFuncsAndBlocks(source []byte) ([]extent, []extent, error) { fset := token.NewFileSet() diff --git a/pkg/testcoverage/coverage/cover_test.go b/pkg/testcoverage/coverage/cover_test.go index 9a75ca7..c9caf31 100644 --- a/pkg/testcoverage/coverage/cover_test.go +++ b/pkg/testcoverage/coverage/cover_test.go @@ -121,10 +121,10 @@ func Test_findFile(t *testing.T) { func Test_findAnnotations(t *testing.T) { t.Parallel() - _, err := FindAnnotations(nil) + _, _, err := FindAnnotations(nil, false) assert.Error(t, err) - _, err = FindAnnotations([]byte{}) + _, _, err = FindAnnotations([]byte{}, false) assert.Error(t, err) const source = ` @@ -138,7 +138,7 @@ func Test_findAnnotations(t *testing.T) { } ` - comments, err := FindAnnotations([]byte(source)) + comments, _, err := FindAnnotations([]byte(source), false) assert.NoError(t, err) assert.Equal(t, []int{3, 5}, pluckStartLine(comments)) } @@ -146,10 +146,10 @@ func Test_findAnnotations(t *testing.T) { func Test_findAnnotationsWithComment(t *testing.T) { t.Parallel() - _, _, err := FindAnnotationsWithComment(nil, true) + _, _, err := FindAnnotations(nil, true) assert.Error(t, err) - _, _, err = FindAnnotationsWithComment([]byte{}, true) + _, _, err = FindAnnotations([]byte{}, true) assert.Error(t, err) const source = ` @@ -167,12 +167,12 @@ func Test_findAnnotationsWithComment(t *testing.T) { } ` - validAnnotations, withoutComment, err := FindAnnotationsWithComment([]byte(source), true) + validAnnotations, withoutComment, err := FindAnnotations([]byte(source), true) assert.NoError(t, err) assert.Equal(t, []int{6}, pluckStartLine(validAnnotations)) assert.Equal(t, []int{3, 9}, pluckStartLine(withoutComment)) - validAnnotations, withoutComment, err = FindAnnotationsWithComment([]byte(source), false) + validAnnotations, withoutComment, err = FindAnnotations([]byte(source), false) assert.NoError(t, err) assert.Equal(t, []int{3, 6, 9}, pluckStartLine(validAnnotations)) assert.Empty(t, withoutComment) diff --git a/pkg/testcoverage/coverage/export_test.go b/pkg/testcoverage/coverage/export_test.go index 21170dd..34be596 100644 --- a/pkg/testcoverage/coverage/export_test.go +++ b/pkg/testcoverage/coverage/export_test.go @@ -1,13 +1,12 @@ package coverage var ( - FindFileCreator = findFileCreator - FindAnnotations = findAnnotations - FindAnnotationsWithComment = findAnnotationsWithComment - FindFuncsAndBlocks = findFuncsAndBlocks - ParseProfiles = parseProfiles - SumCoverage = sumCoverage - FindGoModFile = findGoModFile + FindFileCreator = findFileCreator + FindAnnotations = findAnnotations + FindFuncsAndBlocks = findFuncsAndBlocks + ParseProfiles = parseProfiles + SumCoverage = sumCoverage + FindGoModFile = findGoModFile ) type Extent = extent diff --git a/pkg/testcoverage/report.go b/pkg/testcoverage/report.go index 6fcbf5a..364e966 100644 --- a/pkg/testcoverage/report.go +++ b/pkg/testcoverage/report.go @@ -155,7 +155,7 @@ func reportDiff(w io.Writer, result AnalyzeResult) { fmt.Fprintf(tabber, "\n") } -func ReportForGithubAction(w io.Writer, result AnalyzeResult) { //nolint:maintidx // relax +func ReportForGithubAction(w io.Writer, result AnalyzeResult) { out := bufio.NewWriter(w) defer out.Flush() diff --git a/pkg/testcoverage/report_test.go b/pkg/testcoverage/report_test.go index 06921a5..9314e01 100644 --- a/pkg/testcoverage/report_test.go +++ b/pkg/testcoverage/report_test.go @@ -285,6 +285,103 @@ func Test_ReportForGithubAction(t *testing.T) { assertNotContainStats(t, buf.String(), MakePackageStats(statsNoError)) assertNotContainStats(t, buf.String(), statsNoError) }) + + t.Run("missing explanation annotations", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + result := AnalyzeResult{ + FilesWithMissingExplanations: []coverage.Stats{ + { + Name: "test.go", + AnnotationsWithoutComments: []int{10, 20}, + }, + }, + } + ReportForGithubAction(buf, result) + + output := buf.String() + assert.Contains(t, output, "file=test.go,title=Missing explanation for coverage-ignore,line=10") + assert.Contains(t, output, "file=test.go,title=Missing explanation for coverage-ignore,line=20") + assert.Contains(t, output, "add an explanation after the coverage-ignore annotation") + }) + + t.Run("missing explanation annotations - empty list", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + result := AnalyzeResult{ + FilesWithMissingExplanations: []coverage.Stats{ + { + Name: "test.go", + AnnotationsWithoutComments: []int{}, + }, + }, + } + ReportForGithubAction(buf, result) + + output := buf.String() + assert.NotContains(t, output, "Missing explanation for coverage-ignore") + }) +} + +func Test_ReportMissingExplanations(t *testing.T) { + t.Parallel() + + t.Run("with missing explanations", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + result := AnalyzeResult{ + FilesWithMissingExplanations: []coverage.Stats{ + { + Name: "test.go", + AnnotationsWithoutComments: []int{10, 20}, + }, + }, + } + + ReportForHuman(buf, result) + + output := buf.String() + assert.Contains(t, output, "Files with missing explanations for coverage-ignore annotations:") + assert.Contains(t, output, "test.go") + assert.Contains(t, output, "10, 20") + }) + + t.Run("with empty annotations list", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + result := AnalyzeResult{ + FilesWithMissingExplanations: []coverage.Stats{ + { + Name: "test.go", + AnnotationsWithoutComments: []int{}, + }, + }, + } + + ReportForHuman(buf, result) + + output := buf.String() + // Should not contain the file with empty annotations + assert.NotContains(t, output, "test.go\t") + }) + + t.Run("with no missing explanations", func(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + result := AnalyzeResult{ + FilesWithMissingExplanations: []coverage.Stats{}, + } + + ReportForHuman(buf, result) + + output := buf.String() + assert.NotContains(t, output, "Files with missing explanations for coverage-ignore annotations:") + }) } //nolint:paralleltest // must not be parallel because it uses env