feat: force comment on coverage-ignore annotation (#222)

Author: tino-alfaneti

.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 

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 

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,
 	})
 }
 
@@ -103,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,
@@ -112,11 +118,12 @@ 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: filesWithMissingExplanations,
+		TotalStats:                   coverage.StatsCalcTotal(current),
+		HasBaseBreakdown:             len(base) > 0,
+		Diff:                         calculateStatsDiff(current, base),
+		DiffPercentage:               TotalPercentageDiff(current, base),
 	}
 }
 

pkg/testcoverage/check_test.go

@@ -265,6 +265,40 @@ 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")
+	})
+
+	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")
+	})
 }
 
 func TestCheckDiff(t *testing.T) {
@@ -612,6 +646,36 @@ 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: []int{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.Empty(t, result.FilesWithMissingExplanations)
+
+		// 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)
+	})
 }
 
 func TestLoadBaseCoverageBreakdown(t *testing.T) {

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 {

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

pkg/testcoverage/coverage/cover.go

@@ -21,9 +21,10 @@ import (
 const IgnoreText = "coverage-ignore"
 
 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 +53,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,7 +79,7 @@ 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
 		return Stats{}, fmt.Errorf("failed reading file source [%s]: %w", fi.path, err)
@@ -89,14 +90,19 @@ func coverageForFile(profile *cover.Profile, fi fileInfo) (Stats, error) {
 		return Stats{}, err
 	}
 
-	annotations, err := findAnnotations(source)
+	annotations, withoutComment, err := findAnnotations(source, forceComment)
 	if err != nil { // coverage-ignore
 		return Stats{}, err
 	}
 
 	s := sumCoverage(profile, funcs, blocks, annotations)
 	s.Name = fi.name
 
+	s.AnnotationsWithoutComments = make([]int, len(withoutComment))
+	for i, extent := range withoutComment {
+		s.AnnotationsWithoutComments[i] = extent.StartLine
+	}
+
 	return s, nil
 }
 
@@ -251,23 +257,43 @@ func findFilePathMatchingSearch(files *[]fileInfo, search string) string {
 	return path
 }
 
-func findAnnotations(source []byte) ([]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)
 	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 findFuncsAndBlocks(source []byte) ([]extent, []extent, error) {

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,11 +138,46 @@ 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))
 }
 
+func Test_findAnnotationsWithComment(t *testing.T) {
+	t.Parallel()
+
+	_, _, err := FindAnnotations(nil, true)
+	assert.Error(t, err)
+
+	_, _, err = FindAnnotations([]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 := 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 = FindAnnotations([]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()
 

pkg/testcoverage/coverage/types.go

@@ -11,11 +11,12 @@ import (
 )
 
 type Stats struct {
-	Name           string
-	Total          int64
-	Covered        int64
-	Threshold      int
-	UncoveredLines []int
+	Name                       string
+	Total                      int64
+	Covered                    int64
+	Threshold                  int
+	UncoveredLines             []int
+	AnnotationsWithoutComments []int
 }
 
 func (s Stats) UncoveredLinesCount() int {
@@ -149,6 +150,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