Use SBT settings to control scoverage

[ghost]

This PR is designed and tested with scoverage/scalac-scoverage-plugin#177.
One of the big issues with the sbt-scoverage plugin is turning coverage on/off as the state needs to be modified, the current settings propagated up/down projects, etc. It's certainly not trivial, even if it's actually possible. The number of open/closed issues/PR's attest to this.

So... rather than fully fix this, I have a workaround that I think will provide a solid base to move this forward, and of course it works (I think ) with 2.12 etc. The solution is basically to set things up via a jvm command line parameter: So

• sbt -Dscoverage.enabled="true" +clean +rootJVM/test will test with coverage on for 2.10, 2.11 and 2.12.0-RC1, in this example for all JVM projects only (no JS)
• sbt -Dscoverage.enabled="false" +clean +test +publishLocal will test with coverage off for 2.10, 2.11 and 2.12.0-RC1 - and will not add any depencencies to the pom
• sbt +clean +test +publishLocal will retain current behaviour

runtimeClassifier method supplied by @lustefaniak

[ghost]

N.B. Build failing as release of scoverage/scalac-scoverage-plugin#177 required

[lustefaniak]

I really don't think it is good idea to force users of the sbt-scoverage plugin to download sbt-scalajs, I see all you need from that plugin is a way to guess if the project is Scala.JS and what "binary" version it uses. I guess sbt should have such feature, but for now I think it makes much more sense to just guess that as I did: 2c1e7bb#diff-627f9fd5c400939149d75695caad1988R68

[ghost]

@lustefaniak Basically, I fully agree. In fact, I answered this comment on the SBT gitter even before you posted this comment - and even referenced your work! So yes, we are on the same page.

Note the other suggestion of having a plugin per platform ( one just extending the other) as we do with libs.

The reason I have it as it is now is "just to get it working" - note that I am also enforcing a minimum version of scala.js of 0.6.12 as that is the version that has scala 2.12.0-RC1. This will also be the one that scoverage/scalac-scoverage-plugin#177 uses. Which is the other point - what if the version of scala.js an end user uses is diff to the one that scalac-scoverage-plugin uses? Or ....

These are all points to be considered, really this PR just aims to be a minimum "that works"

[ghost]

That said, as noted in sbt gitter, I said I would do it this way until it became an issue . ie now 😄 So, I'll have a quick look to get a working version that we can refine later

[ghost]

OK: I removed the plugin as per comment ^^.

I also removed the Pgp plugin as it should be under ~/.sbt.

Added attribution to commit comment

[rorygraves]

@BennyHill Are you aware this failed CI build?

[ghost]

@rorygraves "Are you aware this failed CI build?" Yes, see comment in this PR above ^^

N.B. Build failing as release of scoverage/scalac-scoverage-plugin#177 required

or #182 (comment)

What more can I do?

[ghost]

Sorry guys, I am the only person that reviewed this PR and even that attracts negativity. This just cannot be right.

[ghost]

Also:

This PR is designed and tested with scoverage/scalac-scoverage-plugin#177.

That is in the main comment. Game over

[rorygraves]

Sorry @BennyHill, I did a quick scan through the PRs, I read your last commit as 'I've just pushed a change' without reading the entire thread.

[ghost]

I've now removed the jvm settings as I hopefully have fixed this "the sbt way" - the reviews will no doubt help in this regard

1. Changing SBT state can forget previously set values
   See:

• https://github.com/sbt/sbt/blob/7bc9b5a8751543880ad20374847a0b3c42408397/main/src/main/scala/sbt/internal/SessionSettings.scala
• https://github.com/sbt/sbt-remote-control/blob/master/server/src/main/scala/sbt/server/SettingUtil.scala#L11-L29
• https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/sbt-dev/TArFc7G3b2w/-UGTvk5IxUcJ

But, for "easy" things, a commandalias does the job

2. coverageEnabled := false vs coverageOff

These are currently not "mapped" together, despite appearing to do the same thing.

Soln: make coverage coverageOff just call coverageEnabled in ThisBuild := false, ditto for On
That way, we cut down on all the possible invocations.
Also allows per project on off - eg .settings(coverageEnabled := false)

3. Lots of issues/workarounds

There are many "this works for me, try..." suggestions. Whilst helpful, the core problems remain as these solutions tend to be specific to:

• is it a multi project
• is coverage turned on/off interactively or at the command line of sbt invocation
• is covergage completely turned of for a project. I don't just mean highlighting, but the whole scoverage package is just not included
• is it a scala crossbuild
• is it an sjs crossbuild
• is it a scala and scals.js crossbuild
• are other plugins used that change global state (eg Teamcity)
• does the project care if deps remain in the pom (don't add to libraryDependencies unless enabled #153)
• is the artifact "used" outside of sbt

The main way that I have tried to address these issues is by trying to just "standard" sbt methods so hopefully the behave is more familiar

More thoughts:

• I kept coverage for backward compatibility but added coverageOn as I believe it more accurately reflects the intention, and mirrors coverageOff. Reading through the issues, it looks like some/many folks think coverage is a task that actually does the coverage, but it doesn't.
• The README needs modifying to make all this clear
• I have tested on some local projects but tests definitely need to be add

[ghost]

Self review 1

[ghost]

No longer required

[gslowikowski]

Please rebase, remove conflicts, remove cmdlineScoverageEnabled from ScoverageSbtPlugin.scala, revert 479b4c6 (plugin version upgrade should not be a part of this PR), squash commits and I will merge it.

[rorygraves]

@gslowikowski This looks good, lets get it merged then #188 should be good too.

[gslowikowski]

I found one more problem. You removed custom hidden scoveragePlugin configuration added in 4510f09 (#168).

scalac-scoverage-plugin dependency was added to this configuration because it shouldn't be on the classpath and in published pom/ivy project's descriptor.
Previously (before #168) it was added with provided scope - it was on the classpath, but wasn't added to published pom.xml file. That solution wasn't correct.

The solution with custom configuration for plugin dependencies is described in SBT Faq in "How should I express a dependency on an outside tool such as proguard?" section and used e.g. in SBT Sxr plugin

Restore previous logic using custom configuration, please.

[ghost]

This implementation does not add anything when scoverageOff has been called, so works as is

[ghost]

See:
Before and after

[gslowikowski]

Some people publish instrumented artifacts. They shouldn't, but they do. This is very bad, but we cannot do anything to stop them. Compare generated poms with coverage turned on.

scalac-scoverage-runtime dependency is now added with compile scope (previously provided) and is present in pom.xml file. This prevents from problems like in #115.

scalac-scoverage-plugin should not be in dependencies at all.

[ghost]

scalac-scoverage-plugin should not be in dependencies at all.

That's my point it isn't. The after ^^ is from a local publish of this PR, that's how I know that it isn't added.

See also this bigger example

The example you give from SBT is different as it is not a plugin, they don't turn things on/off. But the idea here is that as now the "turning on/off" is done very correctly (as far as I hope) then we do not need to worry.

[ghost]

Perhaps I could make a suggestion? Could we merge this as-is and then I can rebase #188 and that should pass.

I can then change my project https://github.com/typelevel/catalysts to stop using the local publish and we can see what is exactly working?

The project should give a good indication of where we are. We can then always modify/update later and check the differences. I've got nothing against adding the new scope, but so far I do not see that is necessary (but was with the old method)

[ghost]

Some people publish instrumented artifacts. They shouldn't, but they do

And I would say in that case, with covergeOn then they do need the dependency 😄

[ghost]

And the reason for my suggestion to use https://github.com/typelevel/catalysts:

• I'm the maintainer so we have some leeway of doing a few extra commits to try things out
• it compiles for 2.10/2.11/2.12
• is a multi project build
• Cross compiles jvm/js

I concede that ideally we have all tests in scoverage repository, but for now this is, IMHO, a rapid way to checks things out

And on that note, it would be quicker still if sbt-scoverage directly used the latest RC release od the compiler plugin, no local updates required

[ghost]

So, using catalysts publishLocal:

• sbt clean publishLocal and sbt clean test coverageOn test coverageOff publishLocal both yield a pom with no reference to anything to do with scoverage. This is, I believe, ideal behaviour:
• sbt clean test coverageOn publishLocal adds the following to the POM:

        <dependency>
            <groupId>org.scoverage</groupId>
            <artifactId>scalac-scoverage-runtime_2.11</artifactId>
            <version>1.2.0</version>
        </dependency>
        <dependency>
            <groupId>org.scoverage</groupId>
            <artifactId>scalac-scoverage-plugin_2.11</artifactId>
            <version>1.2.0</version>
        </dependency>

Again, this is correct. It was compiled with the coverage library against the runtime. Any lib getting this dependency will know for sure that has been instrumented. Sure, we could remove it using the mechanism you describe ^^ but we are lying. So hey, I can do this - I'm just explaining what is there.

[ghost]

So I've pushed the change

[gslowikowski]

Thank you.