tester: Report missing lines in test coverage #2622
Conversation
With the --threshold flag set, opa did not report the missing lines, making it difficult to understand where the problem is. This commit modifies opa test so that the pretty format reports text instead of JSON, and shows the lines not covered if the threshold is not met. Fixes open-policy-agent#2562 Signed-off-by: Aditya <aditya10699@gmail.com>
There was a problem hiding this comment.
Thanks for the PR! 👍 Its looking pretty good.
Looking at the output I think we will want to maybe make a few adjustments.
Some example output I was playing around with:
{16:41} ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -v -c
/tmp/opa-test/authz_test.rego 100%
--------------------------------------------------------------------------------
Covered:
/tmp/opa-test/authz_test.rego: 3-4
/tmp/opa-test/authz_test.rego: 7-8
/tmp/opa-test/authz_test.rego: 11-12
/tmp/opa-test/authz_test.rego: 15-16
/tmp/opa-test/authz.rego 70%
--------------------------------------------------------------------------------
Covered:
/tmp/opa-test/authz.rego: 3-5
/tmp/opa-test/authz.rego: 14-17
Not covered:
/tmp/opa-test/authz.rego: 8
/tmp/opa-test/authz.rego: 10-11
exit status 1
{16:41} ✘ ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -c
/tmp/opa-test/authz_test.rego 100%
--------------------------------------------------------------------------------
Covered:
/tmp/opa-test/authz_test.rego: 3-4
/tmp/opa-test/authz_test.rego: 7-8
/tmp/opa-test/authz_test.rego: 11-12
/tmp/opa-test/authz_test.rego: 15-16
/tmp/opa-test/authz.rego 70%
--------------------------------------------------------------------------------
Covered:
/tmp/opa-test/authz.rego: 3-5
/tmp/opa-test/authz.rego: 14-17
Not covered:
/tmp/opa-test/authz.rego: 8
/tmp/opa-test/authz.rego: 10-11
exit status 1
{16:45} ✘ ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -c --threshold 99
Missing coverage:
/tmp/opa-test/authz.rego: 8
/tmp/opa-test/authz.rego: 10-11
Code coverage threshold not met: got 83.35 instead of 99.00
exit status 2
{16:53} ✘ ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -c -v --threshold 99
Missing coverage:
/tmp/opa-test/authz.rego: 8
/tmp/opa-test/authz.rego: 10-11
Code coverage threshold not met: got 83.35 instead of 99.00
exit status 2
I think one nice improvement we could make is to support the --verbose flag. For the invocation with only coverage (no threshold set) I think the non-verbose (default) should only show the summary and maybe missing coverage. Then when --verbose/-v is supplied it could show all the covered and missing like it currently does.
For the invocation with a threshold we should default to what we do now, and when in verbose mode also include the covered lines (to match the behavior between coverage with and without threshold).
Please include unit tests for the code changes as well 😄
cmd/test.go
Outdated
| @@ -236,7 +236,7 @@ func opaTest(args []string) int { | |||
| } | |||
| } | |||
| } else { | |||
| reporter = tester.JSONCoverageReporter{ | |||
| reporter = tester.PrettyCoverageReporter{ | |||
There was a problem hiding this comment.
We will need to continue supporting the JSON output format when specified (eg: --format json). This one should be shown when the format is configured to be pretty.
This would introduce an (arguably) breaking change in the CLI behavior... @tsandall thoughts on keeping it default to json if maybe the --format flag was not set?
There was a problem hiding this comment.
Could you check the change I have made? I've kept pretty as the default because the help says so and I'm not really sure if this is the only instance where the default is set to pretty.
There was a problem hiding this comment.
I think its probably OK. We should however add a note to the CHANGELOG noting the new behavior.
Add support for --verbose flag in reporting test coverage in pretty mode Signed-off-by: Aditya <aditya10699@gmail.com>
| Modules: modules, | ||
| Output: os.Stdout, | ||
| Threshold: testParams.threshold, | ||
| if testParams.outputFormat.String() == "json" { |
There was a problem hiding this comment.
nit: Use a switch similar to the one above for the non-coverage case.
| fmt.Fprintln(r.Output, "Not covered:") | ||
| r.printCoverage(name, coverageReport.NotCovered) | ||
| } | ||
| fmt.Println() |
There was a problem hiding this comment.
Should this print into r.Output?
| } | ||
| } | ||
| } | ||
| return errors.New("") |
There was a problem hiding this comment.
Should return nil for a "success" case
| Threshold: r.Threshold, | ||
| } | ||
| } | ||
| fmt.Fprintf(r.Output, "Expected coverage %v%%, got %v%%\n", r.Threshold, report.Coverage) |
There was a problem hiding this comment.
We shouldn't show "Expected coverage .." when there was no threshold, for example as-is we get:
{10:55} ✘ ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -c
Expected coverage 0%, got 83.35%
Missing coverage:
/tmp/opa-test/authz.rego: 8
/tmp/opa-test/authz.rego: 10-11
We are also showing an empty "Missing Coverage:" section when the coverage is 100%:
{11:49} ✘ ~/p/g/s/g/o/opa:Syn3rman-coverage-pretty-reporter ✓ ❯ go run . test /tmp/opa-test -c
Expected coverage 0%, got 100%
Missing coverage:
Im also seeing a non-zero exit code when the tests pass and there is no threshold.. which seems odd. I'm assuming thats from the error("") being returned?
Most of these are things that should be caught by unit tests, please add some tests to cover this new code.
|
Any update @Syn3rman ? |
|
I'm going to close this for now, if/when work continues please reopen |
With the --threshold flag set, opa did not report the missing lines,
making it difficult to understand where the problem is. This commit
modifies opa test so that the pretty format reports text instead of
JSON, and shows the lines not covered if the threshold is not met.
Fixes #2562
Signed-off-by: Aditya aditya10699@gmail.com