coverageEnabled should respect configurations

[hseeberger]

In particular one should be able to define coverageEnabled.in(Test, test) := true which should result in compile to be not affected (no instrumentation), instrumentation would only happen for test.

[RichardBradley]

You have to rebuild anyway after a coverage run, so this would be misleading.

A previous design of the plugin (v < 1.0) used to put the instrumented classes in a different location (target/scoverage-classes), but I think that caused more trouble than it solved.

I think the current design is that you need to "clean" between covered and non-covered tasks.

[arpanchaudhury]

+1 on that. It should also not affect your run task. Instrumentation is causing some error while running the server with coverageEnable set to true.

[noamBarkai]

a typical CI build of ours is:
sbt clean coverage test it:test publish
now it seems we can't run this way any more, but rather must split to two separate builds:
sbt clean coverage test it:test
sbt clean test it:test publish
meaning we need to have our tests run twice in order to verify we're not publishing untested code
this is a serious drawback, meaning we will need to disable scoverage for the time being, until this issue is resolved. bummer 😢

[RichardBradley]

meaning we need to have our tests run twice in order to verify we're not publishing untested code

It is widely considered best practice to run all tests twice anyway if you are using code coverage (for all code coverage systems, not just scoverage): once covered and once uncovered. This is because the coverage instrumentation can change the code behaviour in significant ways, especially regarding concurrency and synchronisation.

See http://stackoverflow.com/a/15799871/8261 for more info.

sbt clean coverage test it:test
sbt clean test it:test publish
...
in order to verify we're not publishing untested code

Why would your code change between the first command and the second?
Can you not just run:

sbt clean coverage test it:test
sbt clean publish

?

[noamBarkai]

Richard,
Re best practice with two test runs - good point! I hadn't thought of that.
Re your suggestion of two consecutive runs: if I understand the issue at
hand correctly the second command needs to be "sbt clean package", not just
"sbt package", no?
In which case the coverage reports are swept away and the build has nothing
to show in the report summary, no?

On Thu, Jan 21, 2016 at 7:13 PM, Richard Bradley notifications@github.com
wrote:

meaning we need to have our tests run twice in order to verify we're not
publishing untested code

It is widely considered best practice to run all tests twice anyway if you
are using code coverage (for all code coverage systems, not just scoverage):
once covered and once uncovered. This is because the coverage
instrumentation can change the code behaviour in significant ways,
especially regarding concurrency and synchronisation.

See http://stackoverflow.com/a/15799871/8261 for more info.

sbt clean coverage test it:test
sbt clean test it:test publish
...
in order to verify we're not publishing untested code

Why would your code change between the first command and the second?
Can you not just run:

sbt clean coverage test it:test
sbt publish

?

—
Reply to this email directly or view it on GitHub
#128 (comment)
.

[noamBarkai]

• publish, not package of course

On Thu, Jan 21, 2016 at 7:36 PM, Noam Barkai noam.barkai@gmail.com wrote:

Richard,
Re best practice with two test runs - good point! I hadn't thought of that.
Re your suggestion of two consecutive runs: if I understand the issue at
hand correctly the second command needs to be "sbt clean package", not just
"sbt package", no?
In which case the coverage reports are swept away and the build has
nothing to show in the report summary, no?

On Thu, Jan 21, 2016 at 7:13 PM, Richard Bradley <notifications@github.com

wrote:

meaning we need to have our tests run twice in order to verify we're not
publishing untested code

It is widely considered best practice to run all tests twice anyway if
you are using code coverage (for all code coverage systems, not just
scoverage): once covered and once uncovered. This is because the
coverage instrumentation can change the code behaviour in significant ways,
especially regarding concurrency and synchronisation.

See http://stackoverflow.com/a/15799871/8261 for more info.

sbt clean coverage test it:test
sbt clean test it:test publish
...
in order to verify we're not publishing untested code

Why would your code change between the first command and the second?
Can you not just run:

sbt clean coverage test it:test
sbt publish

?

—
Reply to this email directly or view it on GitHub
#128 (comment)
.

[RichardBradley]

the second command needs to be "sbt clean package", not just "sbt package", no?

Yes, sorry, typo. Now fixed

In which case the coverage reports are swept away and the build has nothing to show in the report summary, no?

Yes, you'll have to take a copy of them before running clean.

[slavaschmidt]

independent on the topic of running tests twice, the possibility to enable code coverage only for tests, for example like suggested by @hseeberger coverageEnabled.in(Test, test) := true would be very useful in multi-project scenarios if only a subset of all subprojects must have a code coverage information collected

[jeffrey-aguilera]

The bigger problem, as I see it, is that coverageEnabled := True in the build.sbt file causes ALL byte code to be instrumented, even with a clean; compile.

To wit:

> clean
[success] Total time: 0 s, completed May 29, 2017 6:09:20 PM
> compile
[info] Updating {file:/Users/jaguilera/KariusDx/karius-flow/}karius-flow...
[info] Resolving org.scoverage#scalac-scoverage-plugin_2.12;1.3.0 ...
[info] Done updating.
[info] Compiling 4 Scala sources to /Users/jaguilera/KariusDx/karius-flow/target/scala-2.12/classes...
[info] [info] Cleaning datadir [/Users/jaguilera/KariusDx/karius-flow/target/scala-2.12/scoverage-data]
[info] [info] Beginning coverage instrumentation
[info] [warn] Could not instrument [EmptyTree$/null]. No pos.
[info] [info] Instrumentation completed [199 statements]
[info] [info] Wrote instrumentation file [/Users/jaguilera/KariusDx/karius-flow/target/scala-2.12/scoverage-data/scoverage.coverage.xml]
[info] [info] Will write measurement data to [/Users/jaguilera/KariusDx/karius-flow/target/scala-2.12/scoverage-data]
[warn] there was one deprecation warning (since 2.11.0); re-run with -deprecation for details
[warn] one warning found
[success] Total time: 2 s, completed May 29, 2017 6:09:25 PM

Removing coverageEnabled := true:

> reload
[info] Loading project definition from /Users/jaguilera/KariusDx/karius-flow/project
[info] Set current project to Karius Flow (in build file:/Users/jaguilera/KariusDx/karius-flow/)
[warn] libraryDependencies is updated after lock.sbt was created.
[warn] Run `;unlock ;reload ;lock` to re-create lock.sbt.
[warn] Run just `lock` instead if you want to keep existing library versions.
> clean
[success] Total time: 0 s, completed May 29, 2017 6:09:40 PM
> compile
[info] Updating {file:/Users/jaguilera/KariusDx/karius-flow/}karius-flow...
[info] Resolving jline#jline;2.14.3 ...
[info] Done updating.
[info] Compiling 4 Scala sources to /Users/jaguilera/KariusDx/karius-flow/target/scala-2.12/classes...
[warn] there was one deprecation warning (since 2.11.0); re-run with -deprecation for details
[warn] one warning found
[success] Total time: 2 s, completed May 29, 2017 6:09:43 PM

Basically, the coverageEnabled setting in build.sbt is borderline worthless, as it guarantees production bits are shipped with scoverage instrumentation. Methinks the scoverage-classes solution was much better than the current situation.