Understanding coverage calculation on differently formatted rego

[kcparker]

I have been working with opa test -c with the example from https://www.openpolicyagent.org/docs/latest/policy-testing/. When I condense the rego from example.rego to

package authz

allow { input.path == ["users"]; input.method == "POST"; }

allow { some profile_id; input.path = ["users", profile_id]; input.method == "GET"; profile_id == input.user_id; }

And remove all test in example_test.rego except for test_post_allowed

package authz

test_post_allowed {
    allow with input as {"path": ["users"], "method": "POST"}
}

Actual Behavior

The coverage result shows the example.rego was covered only 20 percent.

{
  "files": {
    "example.rego": {
      "covered": [
        {
          "start": {
            "row": 3
          },
          "end": {
            "row": 3
          }
        }
      ],
      "not_covered": [
        {
          "start": {
            "row": 4
          },
          "end": {
            "row": 4
          }
        },
        {
          "start": {
            "row": 4
          },
          "end": {
            "row": 4
          }
        },
        {
          "start": {
            "row": 4
          },
          "end": {
            "row": 4
          }
        },
        {
          "start": {
            "row": 4
          },
          "end": {
            "row": 4
          }
        }
      ],
      "coverage": 20
    },
    "tests/test.rego": {
      "covered": [
        {
          "start": {
            "row": 3
          },
          "end": {
            "row": 4
          }
        }
      ],
      "coverage": 100
    }
  },
  "coverage": 42.85
}

Expected Behavior

I think coverage is counted from statements but the first allow definition has 2 statements and the second has 4, so coverage should be 33%. Overall I would think that if statements were counted then it shouldn't matter how the rego is formatted, but if I leave example.rego in the original format and only run test_post_allowed then I get a coverage on example.rego equal to "coverage": 42.85.

Steps to Reproduce the Problem

Copy the code blocks above for example.rego and example_test.rego and run opa test . -c. You should get the same result I have posed above.

opa version
Version: 0.28.0
Build Commit: 3fbcd71
Build Timestamp: 2021-04-27T13:51:34Z
Go Version: go1.15.8
WebAssembly: unavailable

[kcparker]

In cover/cover.go, the Cover.Report() function has some inconsistency on how it reports covered and not covered statements. If a line has multiple statements and is covered, the code only puts that line in the FileReport.Covered once; however, if the line is not covered the code puts that line in the FileReport.NotCovered for each statement in that line. This produces the weird coverage percentage 20% in my above example.

The report function should go by LOC covered, and the percent should be 50% in my example above.

[tsandall]

I think the problem is that the coverage tracer is only keeping track of what lines are hit--it doesn't keep track distinguish between multiple statements on the same line. As a result, in your example, the coverage tracer only sees one line being hit. When it calculates the coverage percentage for the first allow rule in your example, it only counts a single line as covered (3). On the other hand, the coverage tracer doesn't explicitly track lines that are not covered because it's simply watching what expressions have executed--to compute what lines/statements have not executed, the coverage reporter walks the AST and checks if each statement is in the "hits" set.

Modifying the coverage reporter to ignore multiple statements on the same line that are not covered would be a simple change however it wouldn't solve the underlying issue which is that it would be nice if formatting your policy did not change the coverage results. For the latter, I'd think we'd have to identify all of the statements which we expect events for (i.e., EXIT events for all rule heads, and EVAL events for all expressions) and then keep track of which [EXIT, Rule.Head] and [EVAL, Expr] tuples are seen. The coverage percentage could then be independent of formatting.

@srenatus do you have any ideas around this?

[kcparker]

You're right. I think the better solution is to have the coverage tracer when walking the AST of what is hit create a new list for the covered statements. I have rewritten the coverage tracer myself so that no matter how the rego statements are formatted the coverage percentage result is the same. With this discussion I am also trying to understand if I am seeing is intentional or not.

[srenatus]

Having a coverage metric that is independent of code formatting would be much preferable, I agree.

I have rewritten the coverage tracer myself so that no matter how the rego statements are formatted the coverage percentage result is the same.

This would make a great contribution, I think (wink wink). That said, I cannot tell if the current metric, our "line coverage" approximation here, is flawed enough to be replaced, or if we'd better introduce the new one, "statement coverage", independently... 🤔

[kcparker]

The problem is in the Cover.Report() function, if you don't think it is flawed enough to be replaced then I would suggest a new Cover.StatementReport() function. If this sounds good I'll open an issue and I'd like to contribute back to OPA.

[marcaurele]

Why is the coverage also reported on the test file itself? It should report the coverage of the executions on the policy files only, no?

[anderseknert]

OPA doesn't really have a concept of "test files" — any rule prefixed test_ will be considered a test by the test runner, and tests are of course just regular rules evaluated as any other. Keeping tests separately from policy is recommended, but not enforced. I suppose coverage could exclude rules with the test_ prefix, but I don't think it matters that much in Rego, where there's pretty much only two schools to follow wrt test coverage — either don't care about it, or it should be at 100% at all times or else fail the build. The latter sounds dramatic coming from other languages, but is really quite simple to accomplish in Rego, and I'd say a good practice for anything meant for production.