Coverage Broken: why does nobody care?

[smarter]

@deusaquilus

Code-coverage is a company’s first and only reliable line-of-defense against incidental failure that can be easily understood and measured.

These companies might also be interested in helping improve the Scala 2 coverage implementation then, consider:

import java.nio.file._

object Util {
  def createFile(fileName: String): Path = {
    Files.createFile(
      return Paths.get(fileName)
    )
  }
}

createFile is never called because of the stray return, but in Scala 2 we get:

Whereas in Scala 3 we get:

So neither is right, but Scala 2 is over-counting the coverage percentage whereas Scala 3 is under-counting. The latter is more noticeable, but the former seems more problematic to me since it gives you a false sense of confidence. I don’t think either issue is due to carelessness, this stuff is just hard to get right.

[deusaquilus]

@smarter

So neither is right, but Scala 2 is over-counting the coverage percentage whereas Scala 3 is under-counting. The latter is more noticeable, but the former seems more problematic to me since it gives you a false sense of confidence. I don’t think either issue is due to carelessness, this stuff is just hard to get right.

I would daresay that it’s better to over-count this. It is not the job of code-coverage to find incorrect runtime code-paths (or at least incorrect ones according to what our human definitions are) because this requires understanding what a user’s intentions are, which is the job of the actual unit-test’s assertion clauses. They’re the ones that should catch this problem, not the coverage tool.

The conceptual model that most users have about code-coverage is based on an abstract notion of what represents a “line of code” and what “lines of code” get “hit.” That means that if the actual line representing the argument passed to Paths.get is invoked in some kind of way, it should be counted as “hit,” even if it technically represents a behavior that does not cause the execution of the surrounding function.

When code-coverage attempts to be more clever than this “line being hit” model, it inherently introduces unexpected behavior that confuses and irritates users. Coverage checkers should be as simple and naive as possible.

[smarter]

Thanks for the input! It’d be great to hear more feedback from people on edge cases like this since it affects the design and complexity of the implementation. Here’s another one:

  def one(cond: Boolean, fallback: Boolean): Boolean =
    cond || fallback

  def two(cond: Boolean, fallback: => Boolean): Boolean =
    cond || fallback

  def three(cond: Boolean, fallback: () => Boolean): Boolean =
    cond || fallback()

If cond is true and the boolean is short-circuited, should the second argument be marked as covered or not? The current behavior of scoverage marks the first two as covered but the last one as uncovered.

[deusaquilus]

I think that in an ideal world the 2nd argument would be marked not-covered in the last two cases but having only the last case not-covered is fine too. Since most users typically start with a simplistic “was the whole line hit” model, they generally need to do some digging in order to be able to understand that short-circuiting is going on at all. Conceptually, it is easier to notice that short-circuting would affect the last-case than the second case so it would occur to a user much faster than in the 2nd case.

[morgen-peschke]

I’d honestly prefer having the second marked uncovered in all 3 cases. Aside from simplifying the mental model of when something gets marked and when it doesn’t, knowing if the fallback was never reached could help debug why a particular branch isn’t getting hit downstream.

[matthew.rooney]

I take the view that in all 3 cases the arguments to the function should be marked as covered, but the shortcircuited statement should be marked as not covered.

in my view the method declaration should be marked as covered if the method is executed at all. I don’t think the coverage tool should try and calculate whether or not a particular lazy parameter has been accessed.

however instrumentation at the call site should indicate whether or not the supplied byName parameter was executed or not.

one possible naive solution to alot of these issues would be to introduce an invokeAround method to the Invoker class and wrap any statements in one of those calls (with some filtering).

method calls are problematic, but one solution to that could be nArity variations of invokeAround

for example

val a = ""
lazy val b = 1
def c = false
foo(x => x + x, a, b, c)

could be transformed to

val a = invokeAround("", ...)
lazy val b = {
  invoked(...)
  invokeAround(1, ...)
}
def c =  {
   invoked(...)
   invokeAround(false)
}

invokeAround4(
  (_1,_2,_3,_4) => foo(_1,_2,_3,_4), 
  invokeAround(x => x + x),
  invokeAround(a),
  invokeAround(b),
  invokeAround(c), ...)

with the possiblity to drop some invoke arounds when a val is passed to a strict method parameter etc

sudo implementation of invokeAround

def invokeAround[A](a :=> A, id: number, file: String): A = {
  invoked(number, file)
  a
}

def invokedAround1[A, Res](f: A => Res, param1: A, id: number, file: String): Res = {
   // enforces strict evaluation of the parameters
   // if a throw happens during evaluation of param1 then this code won't be reached
   invoked(id, file)
   f(param1)
}

[smarter]

@matthew.rooney

I take the view that in all 3 cases the arguments to the function should be marked as covered, but the shortcircuited statement should be marked as not covered.

Sorry I should’ve been clearer, by “arguments” I meant, “arguments to the || operator”, not “method parameters”, but I get what you mean.

[matthew.rooney]

Looking at the code it seems that this idea is not necessary. Since it already uses a lifting based implementation: (was fun to think about though)

dotty.tools.dotc.typer.LiftCoverage$#liftForCoverage

[DamianReeves]

Just want to say its appreciated how folks have focused on this item, which slipped through the cracks.

I think assumptions of positive intent can sometimes leave you burnt, I choose to assume positive intent from the community here.

The Scala compiler team, VirtusLab, and Scala Center have shown a real commitment to improving the tooling many have complained about.

I just wanted to say thanks.

[Paul-Folbrecht]

Ditto!

[matthew.rooney]

@smarter I’ve had a go at an implementation for shortcircuited boolean expressions (and a similar thing for numerical expressions). This numeric expressions are a bit overzealous, since it instruments everything that was selected for the expression.

WIP create overzealous instrumentation for numeric operations and branching approach for short circuited boolean operations, perhaps all boolean operations can be instrumented in the same way?

Perhaps it would make sense for that to be the default and add special cases for:

• Simple numeric expressions with only literals and val/var coefficients. (These can be instrumented as one thing)
• Simple numeric expressions that make up an assignment. (The left and right handside can be instrumented as one thing)

The result of something like a < b && b > 0 || c is 3 instrumented expressions a < b, b > 0, c

[alexandru]

I have been a long-time user of scoverage for Scala 2. It does the job, however it’s not perfect. Small annoyances aside, my grievance has been that the process becomes really slow when the test coverage is turned on. At $work I’m thinking of dropping the coverage metrics entirely due to the build with tests taking close to an hour, which is bad if you’re waiting on it to finish in order to test the deployed artefacts. And I’m against running the test coverage for PRs for the same reason.

Not sure if there’s anything that anyone can do about it, maybe this is intrinsic to the JVM, but some performance improvements would be appreciated too.

[smarter]

The initial project (http://guillaume.martres.me/code_coverage.pdf) measured some significant performance improvements in the Scala 3 implementation compared to the Scala 2 one, but I don’t think anyone has benchmarked it recently. I think it should be possible to improve it further using invokedynamic to reduce the bytecode overhead (I believe jacoco does that too).

[smarter]

@matthew.rooney

I’ve had a go at an implementation for shortcircuited boolean expressions (and a similar thing for numerical expressions).

Before we go too far in that direction, we might want to setup some benchmarks with the coverage phase enabled. @mbovel can correct me but I believe it’s only a matter of adding a file in dotty/bench/profiles at main · lampepfl/dotty · GitHub which passes the correct flags to the compiler to enable coverage.

[dragos]

@alexandru

Not sure if there’s anything that anyone can do about it, maybe this is intrinsic to the JVM, but some performance improvements would be appreciated too.

The main issue is writing to disk on every invocation. At my previous job we forked scoverage and changed the invoker to maintain an in-memory map, and dumped it to disk in a shutdown hook. One further improvement had to do with multi-threaded code and contention in the said map, but overall it had an overhead in the 20-30% ballpark, which should be acceptable for most projects.

[swoogles]

Is it possible for someone involved with the project to put up a bounty for fixing the flatmap bug? I would love for there to be a way to bridge the divide between:

• The people that have money, and need the fix
• The people that have the knowledge/ability, and need motivation

There’s no guarantee that it would resolve this, but it provides one more option.

I personally have loved the recent uptick in ways to micro-finance projects that I rely on.

[nafg]

I don’t know why my email replies never show up, but perhaps they could use Algora (https://console.algora.io/)

[TheElectronWill]

Yep, I took care to use some lifting when finishing the first version of the coverage implementation for Dotty. A fun problem with a nice solution Sorry for the bugs btw… the tests I wrote were incomplete and I was pretty much the only one interested in implementing this in the compiler :S.

@DamianReeves I see that some fixes have already been merged to master (e.g. Coverage: mark case bodies as branches; don't ignore branches with synthetic spans by KacperFKorban · Pull Request #18437 · lampepfl/dotty · GitHub), but if more fixes are required and you’re interested in contributing, I would be happy to work with you (or anyone else) in a future Scala Spree!

[bishabosha]

Hello Scala Community,

At the Scala Center, we hear you loud and clear regarding coverage support in the Scala 3 compiler. We understand its importance and want to keep you informed about our actions.

We would first like to remind everyone that we are a small team, which is why this likely slipped through our attention. There are many open issues in the compiler, and companies have not approached us directly about the importance of coverage support. As a reminder, the usual channel for escalation of very important issues is a proposal to the Scala Center’s Advisory Board. A good way to get your voice heard directly is to join the Advisory Board, which would also help fund contributions, or you can work with our community representative (currently Eugene Yokota) to create a proposal.

As noted earlier by Paweł Marks, the release officer of Scala 3, one engineer at VirtusLab is now working on addressing immediate issues with coverage support. In the past week these PR’s are already opened: lampepfl/dotty#18424 and lampepfl/dotty#18437.

Going forward, the Scala Organisation (the Scala Center, LAMP, VirtusLab, and Lightbend) will assign an engineer to be responsible in the long term for coverage support in the Scala 3 compiler, but we are still deciding whom.

[hepin1989]

Donation to scala center and then they can hire people.and thanks lightbend scala center and viruslab , everyone make scala very easier to use day by day.

I think another way to support this is ask the company’s open-source committees to support.

I will right a post in our internal forum for that,hope this will help.