Merge pull request #1145 from cloudflare/checkstyle

Add more test coverage for checkstyle

cmd/pint/ci.go

@@ -125,24 +125,20 @@ func actionCI(c *cli.Context) error {
 	}
 
 	reps := []reporter.Reporter{}
-	if c.String(checkStyleFlag) != "" {
-		f, fileErr := os.Create(c.String(checkStyleFlag))
-		if fileErr != nil {
-			return fileErr
-		}
-		// execute here so we can close the file right after
-		errRep := reporter.NewCheckStyleReporter(f).Submit(summary)
-		slog.Error("Error encountered", "error:", errRep)
-		cerr := f.Close()
-		if cerr != nil {
-			return cerr
-		}
-	}
 	if c.Bool(teamCityFlag) {
 		reps = append(reps, reporter.NewTeamCityReporter(os.Stderr))
 	} else {
 		reps = append(reps, reporter.NewConsoleReporter(os.Stderr, checks.Information))
 	}
+	if c.String(checkStyleFlag) != "" {
+		var f *os.File
+		f, err = os.Create(c.String(checkStyleFlag))
+		if err != nil {
+			return err
+		}
+		defer f.Close()
+		reps = append(reps, reporter.NewCheckStyleReporter(f))
+	}
 
 	if meta.cfg.Repository != nil && meta.cfg.Repository.BitBucket != nil {
 		token, ok := os.LookupEnv("BITBUCKET_AUTH_TOKEN")

cmd/pint/lint.go

@@ -114,17 +114,13 @@ func actionLint(c *cli.Context) error {
 	}
 
 	if c.String(checkStyleFlag) != "" {
-		f, fileErr := os.Create(c.String(checkStyleFlag))
-		if fileErr != nil {
-			return fileErr
-		}
-		// execute here so we can close the file right after
-		errRep := reporter.NewCheckStyleReporter(f).Submit(summary)
-		slog.Error("Error encountered", "error:", errRep)
-		cerr := f.Close()
-		if cerr != nil {
-			return cerr
+		var f *os.File
+		f, err = os.Create(c.String(checkStyleFlag))
+		if err != nil {
+			return err
 		}
+		defer f.Close()
+		reps = append(reps, reporter.NewCheckStyleReporter(f))
 	}
 
 	for _, rep := range reps {

cmd/pint/tests/0194_lint_checkstyle.txt

@@ -1,6 +1,5 @@
-env NO_COLOR=1
 ! exec pint --no-color lint --min-severity=info --checkstyle=checkstyle.xml rules
-cmp checkstyle.xml checkstyle_check.xml
+cmp checkstyle.xml checkstyle_expected.xml
 
 -- rules/0001.yml --
 groups:
@@ -10,11 +9,11 @@ groups:
     expr: up
   - alert: Example
     expr: sum(xxx) with()
--- checkstyle_check.xml --
+-- checkstyle_expected.xml --
 <?xml version="1.0" encoding="UTF-8"?>
 <checkstyle version="4.3">
   <file name="rules/0001.yml">
-    <error line="5" severity="Warning" message="Text:Alert query doesn&#39;t have any condition, it will always fire if the metric exists.&#xA; Details:Prometheus alerting rules will trigger an alert for each query that returns *any* result.&#xA;Unless you do want an alert to always fire you should write your query in a way that returns results only when some condition is met.&#xA;In most cases this can be achieved by having some condition in the query expression.&#xA;For example `up == 0` or `rate(error_total[2m]) &gt; 0`.&#xA;Be careful as some PromQL operations will cause the query to always return the results, for example using the [bool modifier](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators)." source="alerts/comparison"></error>
-    <error line="7" severity="Fatal" message="Text:Prometheus failed to parse the query with this PromQL error: unexpected identifier &#34;with&#34;.&#xA; Details:[Click here](https://prometheus.io/docs/prometheus/latest/querying/basics/) for PromQL documentation." source="promql/syntax"></error>
+    <error line="5" severity="Warning" message="Alert query doesn&#39;t have any condition, it will always fire if the metric exists.&#xA;Prometheus alerting rules will trigger an alert for each query that returns *any* result.&#xA;Unless you do want an alert to always fire you should write your query in a way that returns results only when some condition is met.&#xA;In most cases this can be achieved by having some condition in the query expression.&#xA;For example `up == 0` or `rate(error_total[2m]) &gt; 0`.&#xA;Be careful as some PromQL operations will cause the query to always return the results, for example using the [bool modifier](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators)." source="alerts/comparison"></error>
+    <error line="7" severity="Fatal" message="Prometheus failed to parse the query with this PromQL error: unexpected identifier &#34;with&#34;.&#xA;[Click here](https://prometheus.io/docs/prometheus/latest/querying/basics/) for PromQL documentation." source="promql/syntax"></error>
   </file>
-</checkstyle>
+</checkstyle>

cmd/pint/tests/0195_lint_checkstyle_no_dir.txt

@@ -0,0 +1,16 @@
+! exec pint --no-color lint --checkstyle=x/y/z/checkstyle.xml rules
+! stdout .
+cmp stderr stderr.txt
+
+-- stderr.txt --
+level=INFO msg="Finding all rules to check" paths=["rules"]
+level=ERROR msg="Fatal error" err="open x/y/z/checkstyle.xml: no such file or directory"
+-- rules/0001.yml --
+groups:
+- name: test
+  rules:
+  - alert: Example
+    expr: up
+  - alert: Example
+    expr: sum(xxx) with()
+

cmd/pint/tests/0196_checkstyle_ci.txt

@@ -0,0 +1,52 @@
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+cp ../src/v1.yml rules.yml
+cp ../src/.pint.hcl .
+env GIT_AUTHOR_NAME=pint
+env GIT_AUTHOR_EMAIL=pint@example.com
+env GIT_COMMITTER_NAME=pint
+env GIT_COMMITTER_EMAIL=pint@example.com
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b v2
+cp ../src/v2.yml rules.yml
+exec git commit -am 'v2'
+
+exec pint -l debug --offline --no-color ci --checkstyle=checkstyle.xml
+! stdout .
+cmp checkstyle.xml ../checkstyle_expected.xml
+
+-- src/v1.yml --
+- alert: rule1
+  expr: sum(foo) by(job)
+- alert: rule2
+  expr: sum(foo) by(job)
+  for: 0s
+
+-- src/v2.yml --
+- alert: rule1
+  expr: sum(foo) by(job)
+  for: 0s
+- alert: rule2
+  expr: sum(foo) by(job)
+  for: 0s
+
+-- src/.pint.hcl --
+ci {
+  baseBranch = "main"
+}
+parser {
+  relaxed = [".*"]
+}
+
+-- checkstyle_expected.xml --
+<?xml version="1.0" encoding="UTF-8"?>
+<checkstyle version="4.3">
+  <file name="rules.yml">
+    <error line="2" severity="Warning" message="Alert query doesn&#39;t have any condition, it will always fire if the metric exists.&#xA;Prometheus alerting rules will trigger an alert for each query that returns *any* result.&#xA;Unless you do want an alert to always fire you should write your query in a way that returns results only when some condition is met.&#xA;In most cases this can be achieved by having some condition in the query expression.&#xA;For example `up == 0` or `rate(error_total[2m]) &gt; 0`.&#xA;Be careful as some PromQL operations will cause the query to always return the results, for example using the [bool modifier](https://prometheus.io/docs/prometheus/latest/querying/operators/#comparison-binary-operators)." source="alerts/comparison"></error>
+    <error line="3" severity="Information" message="`0s` is the default value of `for`, consider removing this redundant line." source="alerts/for"></error>
+  </file>
+</checkstyle>

cmd/pint/tests/0197_ci_checkstyle_no_dir.txt

@@ -0,0 +1,49 @@
+mkdir testrepo
+cd testrepo
+exec git init --initial-branch=main .
+
+cp ../src/v1.yml rules.yml
+cp ../src/.pint.hcl .
+env GIT_AUTHOR_NAME=pint
+env GIT_AUTHOR_EMAIL=pint@example.com
+env GIT_COMMITTER_NAME=pint
+env GIT_COMMITTER_EMAIL=pint@example.com
+exec git add .
+exec git commit -am 'import rules and config'
+
+exec git checkout -b v2
+cp ../src/v2.yml rules.yml
+exec git commit -am 'v2'
+
+! exec pint --offline --no-color ci --checkstyle=x/y/z/checkstyle.xml
+! stdout .
+cmp stderr ../stderr.txt
+
+-- src/v1.yml --
+- alert: rule1
+  expr: sum(foo) by(job)
+- alert: rule2
+  expr: sum(foo) by(job)
+  for: 0s
+
+-- src/v2.yml --
+- alert: rule1
+  expr: sum(foo) by(job)
+  for: 0s
+- alert: rule2
+  expr: sum(foo) by(job)
+  for: 0s
+
+-- src/.pint.hcl --
+ci {
+  baseBranch = "main"
+}
+parser {
+  relaxed = [".*"]
+}
+
+-- stderr.txt --
+level=INFO msg="Loading configuration file" path=.pint.hcl
+level=INFO msg="Finding all rules to check on current git branch" base=main
+level=INFO msg="Offline mode, skipping Prometheus discovery"
+level=ERROR msg="Fatal error" err="open x/y/z/checkstyle.xml: no such file or directory"

docs/changelog.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## v0.67.0
+
+### Added
+
+- Added `--checkstyle` flag to `pint lint` & `pint ci` for writing XML report file
+  in `checkstyle` format - [#1129](https://github.com/cloudflare/pint/pull/1129).
+
 ## v0.66.1
 
 ### Fixed

internal/reporter/checkstyle.go

@@ -4,6 +4,7 @@ import (
 	"encoding/xml"
 	"fmt"
 	"io"
+	"log/slog"
 	"strconv"
 )
 
@@ -27,8 +28,8 @@ func createCheckstyleReport(summary Summary) checkstyleReport {
 	return x
 }
 
-func (d checkstyleReport) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {
-	err := e.EncodeToken(xml.StartElement{
+func (d checkstyleReport) MarshalXML(e *xml.Encoder, _ xml.StartElement) (err error) {
+	err = e.EncodeToken(xml.StartElement{
 		Name: xml.Name{Local: "checkstyle"},
 		Attr: []xml.Attr{
 			{
@@ -41,34 +42,35 @@ func (d checkstyleReport) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {
 		return err
 	}
 	for dir, reports := range d {
-		errEnc := e.EncodeToken(xml.StartElement{
-			Name: xml.Name{Local: "file"},
-			Attr: []xml.Attr{
-				{
-					Name:  xml.Name{Local: "name"},
-					Value: dir,
+		if err = e.EncodeToken(
+			xml.StartElement{
+				Name: xml.Name{Local: "file"},
+				Attr: []xml.Attr{
+					{
+						Name:  xml.Name{Local: "name"},
+						Value: dir,
+					},
 				},
-			},
-		})
-		if errEnc != nil {
-			return errEnc
+			}); err != nil {
+			return err
 		}
 		for _, report := range reports {
-			errEnc2 := e.Encode(report)
-			if errEnc2 != nil {
-				return errEnc2
+			if err = e.Encode(report); err != nil {
+				return err
 			}
 		}
-		errEnc = e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "file"}})
-		if errEnc != nil {
-			return errEnc
+		if err = e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "file"}}); err != nil {
+			return err
 		}
 	}
-	err = e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "checkstyle"}})
-	return err
+	return e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "checkstyle"}})
 }
 
-func (r Report) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {
+func (r Report) MarshalXML(e *xml.Encoder, _ xml.StartElement) (err error) {
+	msg := r.Problem.Text
+	if r.Problem.Details != "" {
+		msg += "\n" + r.Problem.Details
+	}
 	startel := xml.StartElement{
 		Name: xml.Name{Local: "error"},
 		Attr: []xml.Attr{
@@ -82,29 +84,27 @@ func (r Report) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {
 			},
 			{
 				Name:  xml.Name{Local: "message"},
-				Value: fmt.Sprintf("Text:%s\n Details:%s", r.Problem.Text, r.Problem.Details),
+				Value: msg,
 			},
 			{
 				Name:  xml.Name{Local: "source"},
 				Value: r.Problem.Reporter,
 			},
 		},
 	}
-	err := e.EncodeToken(startel)
-	if err != nil {
+	if err = e.EncodeToken(startel); err != nil {
 		return err
 	}
-	err = e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "error"}})
-
-	return err
+	return e.EncodeToken(xml.EndElement{Name: xml.Name{Local: "error"}})
 }
 
 func (cs CheckStyleReporter) Submit(summary Summary) error {
 	checkstyleReport := createCheckstyleReport(summary)
 	xmlString, err := xml.MarshalIndent(checkstyleReport, "", "  ")
 	if err != nil {
-		fmt.Printf("%v", err)
+		slog.Error("Failed to marshal checkstyle report", slog.Any("err", err))
+		return err
 	}
-	fmt.Fprint(cs.output, string(xml.Header)+string(xmlString)+"\n")
-	return nil
+	_, err = fmt.Fprint(cs.output, string(xml.Header)+string(xmlString)+"\n")
+	return err
 }

internal/reporter/checkstyle_test.go

@@ -61,7 +61,7 @@ func TestCheckstyleReporter(t *testing.T) {
 			output: `<?xml version="1.0" encoding="UTF-8"?>
 <checkstyle version="4.3">
   <file name="foo.txt">
-    <error line="5" severity="Information" message="Text:mock text&#xA; Details:mock details" source="mock"></error>
+    <error line="5" severity="Information" message="mock text&#xA;mock details" source="mock"></error>
   </file>
 </checkstyle>
 `,
@@ -90,7 +90,7 @@ func TestCheckstyleReporter(t *testing.T) {
 			output: `<?xml version="1.0" encoding="UTF-8"?>
 <checkstyle version="4.3">
   <file name="foo.txt">
-    <error line="5" severity="Bug" message="Text:mock text&#xA; Details:" source="mock"></error>
+    <error line="5" severity="Bug" message="mock text" source="mock"></error>
   </file>
 </checkstyle>
 `,
@@ -121,7 +121,7 @@ func TestCheckstyleReporter(t *testing.T) {
 			output: `<?xml version="1.0" encoding="UTF-8"?>
 <checkstyle version="4.3">
   <file name="foo.txt">
-    <error line="5" severity="Bug" message="Text:mock text&#xA;&#x9;&#x9;with [new lines] and pipe| chars that are &#39;quoted&#39;&#xA;&#x9;&#x9;&#xA; Details:" source="mock"></error>
+    <error line="5" severity="Bug" message="mock text&#xA;&#x9;&#x9;with [new lines] and pipe| chars that are &#39;quoted&#39;&#xA;&#x9;&#x9;" source="mock"></error>
   </file>
 </checkstyle>
 `,