Coberture and JDK9

[Pavel Bludov]

The Cobertura project uses com.sun:tools:jar and, therefore is incompatible with JDK9.

https://github.com/cobertura/cobertura/issues/271

The cobertura-maven-plugin simply does not load:

[ERROR] Failed to execute goal on project cobertura-maven-plugin: Could not resolve dependencies for project org.codehaus.mojo:cobertura-maven-plugin:maven-plugin:2.7: Could not find artifact com.sun:tools:jar:0 at specified path /usr/lib/jvm/java-9-jdk/../lib/tools.jar

The bad news is that the Cobertura hasn't been updated since 2015. There is not much options to choose:

• forget about JDK9 (not a real solution)
  
• wait till someone fixes cobertura (small chance)
  
• fix cobertura (much work)
  
• migrate to other plugin for the code coverage (unknown problems)
  

They are all bad. Does anyone know the better option?

[Roman Ivanov]

Hi Pavel,

sorry for delay in reply.

> The bad news is that the Cobertura hasn't been updated since 2015. There is not much options to choose

There are a lot of problems with Cobertura, it is kind of dead.

https://github.com/jacoco/jacoco

Recent release (very long awaited) https://github.com/jacoco/jacoco/releases/tag/v0.8.0 have all required features for us to give it one more chance.

There are already some work for this 

https://github.com/checkstyle/checkstyle/wiki/How-to-generate-UT-coverage-report

"Jacoco way: we never used it in build system , config was removed at commit . It never worked better than cobertura for us."

thanks

Roman Ivanov

​

[Pavel Bludov]

There are a lot of problems with Cobertura, it is kind of dead.

https://github.com/jacoco/jacoco

Recent release (very long awaited) https://github.com/jacoco/jacoco/releases/tag/v0.8.0 have all required features for us to give it one more chance.

Thanks, Roman! I'll give it a try. For now, the Java9 roadmap draft is:

• Remove dependency of tools.jar in the code
• Remove dependency of tools.jar in tests
• Replace or fix Cobertura
• Upgrade PowerMock to version 2.0 (java9 support is already here, but it is still a beta version)
• Update the java grammar (modules, some changes to try-with-resource, lambda parameter '_', etc)

[Roman Ivanov]

good plan .

> Update the java grammar (modules, some changes to try-with-resource, lambda parameter '_', etc)​

this is unrelated issues. Do you know exact problems? 

We have to process jdk9 compiled sources, and with jdk9 in runtime, right now.

In scope of this issue to try to be compiled by jdk9, build time, not a runtime.

[Pavel Bludov]

this is unrelated issues. Do you know exact problems? 

We have to process jdk9 compiled sources, and with jdk9 in runtime, right now.

In scope of this issue to try to be compiled by jdk9, build time, not a runtime.

Yes. The last one is different. And dangerous. May be it should be done as a side effect of this:

https://groups.google.com/forum/#!topic/checkstyle-devel/VpN-W4jv5vc

The ANTLR repo already has the java9 parser:

https://github.com/antlr/grammars-v4/blob/master/java9/Java9.g4

There should be a bit of coding and a lot of regression testing in this task.

[Roman Ivanov]

Let's focus on building checkstyle on jdk9 for now.

Changing grammar is very complicated task and I do not see good reason to do this.

Our CI parse openjdk9 sources without problems, so we are fine for now.

--
You received this message because you are subscribed to the Google Groups "checkstyle-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to checkstyle-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Pavel Bludov]

Here is the Jacoco report.

Most of them are grammar and gui. What is more:

ClassResolver::resolve

This is a known Jacoco "feature" for methods that always throw an exception.
Not a big problem. By the way, this method can be rewritten from overnested if mess to
something like this:

public Class<?> resolve(String name, String currentClass)
 throws ClassNotFoundException {

 Stream<Function<String, Class<?>>> resolvers = Stream.of(
   // see if the class is full qualified
   this::resolveQualifiedName,
   // try matching explicit imports
   this::resolveMatchingExplicitImport,
   // see if in the package
   this::resolveInPackage,
   // see if inner class of this class
   n -> resolveInnerClass(n, currentClass),
   // try matching star imports
   this::resolveByStarImports
 );
 return resolvers.map(r -> r.apply(name)).filter(Objects::nonNull).findFirst()
   .orElseThrow(() -> new ClassNotFoundException(name));
}

PackageNamesLoader::getPackageNames same bug. Can be fixed with adding a test case that will NOT cause PackageNamesLoader::processFile to throw an exception.

System.exit in Main::main also lacks coverage, as it should be. Can be disabled.

JavadocUtils::static init I have no idea, why Cobertura misses this. Seems to be a bug in Cobertura.
Can be fixed with extracting the part of the code to another method.
Actually, the code in JavadocUtils duplicates the code in TokenUtils.

Thats all. If you are OK with these changes, I'll send a PR soon.

[Roman Ivanov]

Hi Pavel,

thanks a lot for investigation.

we need to start return of jacoco to our code and CI, step by step.

> Most of them are grammar and gui. 

please send PR with update to allow to run validation from CI.

please update TravisCI config to do jacoco coverage check.

All classes that are not 100% covered should be placed to custom level that is stricktly defined.

All future updates to fix coverage should update such custom numbers, eventually all should become 100.

> System.exit in Main::main also lacks coverage, as it should be. Can be disabled.

please propose PR, to let me see how it will looks like.

>JavadocUtils::static init I have no idea, why Cobertura misses this. Seems to be a bug in Cobertura.

Are you sure ? to prove this , lets merge current coverage now and by special PR fix coverage to 100%.

thanks,

Roman Ivanov

​

[Pavel Bludov]

вторник, 6 февраля 2018 г., 22:57:51 UTC+8 пользователь Roman Ivanov написал:

please send PR with update to allow to run validation from CI.

please update TravisCI config to do jacoco coverage check.

I've started a new issue: https://github.com/checkstyle/checkstyle/issues/5516
please approve and comment it.

[Pavel Bludov]

> System.exit in Main::main also lacks coverage, as it should be. Can be disabled.

please propose PR, to let me see how it will looks like.

It looks like

/**
 * This test verifies nothing. It is a workaround for the Jacoco limitations.
 * @see <a href="https://github.com/jacoco/jacoco/issues/117">Jacoco issue 117</a>
 * @throws Exception if there is a problem
 */
@Test
public void testJacocoWorkaround() throws Exception {
  mockStatic(System.class);
  Main.main();
  verifyStatic(System.class);
}

And one note about the Jacoco. just for you information:
https://github.com/jacoco/jacoco/issues/394
I'm not sure, will it affect the checkstyle  surefire tests or not. Jacoco works fine with openjdk version "1.8.0_151" for me.

 

>JavadocUtils::static init I have no idea, why Cobertura misses this. Seems to be a bug in Cobertura.

Are you sure ? to prove this , lets merge current coverage now and by special PR fix coverage to 100%.

 
The report http://checkstyle.sourceforge.net/cobertura/com.puppycrawl.tools.checkstyle.utils.JavadocUtils.html
shows that JavadocUtils has two fields that are not "public int."
I'm sure they are the internal fields of cobertura, added to the class at the stage of instrumentalization.

[Pavel Bludov]

Here is the coverage report by the Openclover:

https://pbludov.github.io/cobertura-jacoco/clover/

I expected that it can do coverage for lambdas. Unfortunately, it can not. But this can still be interesting. For example:
https://pbludov.github.io/cobertura-jacoco/clover/com/puppycrawl/tools/checkstyle/checks/metrics/JavaNCSSCheck.html?line=285#src-285

Openclover does a source-based instrumentation, so it can detect uncovered switch cases.
In other hand, it is incompatible with any test using reflection.

If you are interested, I can add a profile for the Openclover. But there will be a lot of suppression in it.

[Roman Ivanov]

Hi Pavel,

>In other hand, it is incompatible with any test using reflection.

We use reflection a lot to cover conner cases in private hard to reach cases. Big suppression set is hard to maintain.

I think we need to keep focus on finishing jacoco, and think about smth else in few years later.

We use coverage only as automatic detection of uncovered code to ease my code review process. 100% covered code is not a guarantee that module works fine. We use pitest to boost this analysis, but regressions still happening. Significantly less likely, but happening.

Looks like clover become free recently http://openclover.org .

But still this will be small enhancement to jacoco. With usage of pitest , clover become less attractive. We still have several unresolved cases in pitest, so it is better to finish it, we will benefit more from it.

>JavaNCSSCheck.html?line=285#src-285

This supposed to be caught by pitest execution. Is this case is not used, we can remove it and non of UT notice it (pure pitest scenario) ... It is a bug in pitest. 

Please investigate this case.

Thanks,

Roman Ivanov

--

[Pavel Bludov]

Hi Roman!

This supposed to be caught by pitest execution. If this case is not used, we can remove it and none of UT notice it (pure pitest scenario) ... It is a bug in pitest. 

Please investigate this case.

I'm on it.

Also, the next big stopper on the way to JDK9 is PowerMock. I've started an issue: https://github.com/checkstyle/checkstyle/issues/5558
Please approve and comment it.

[Roman Ivanov]

approved.

--

[Pavel Bludov]

Only two minor issues are left:
https://github.com/checkstyle/checkstyle/issues/5561

and
https://github.com/powermock/powermock/issues/864
The last one is on hold for now, but there is a workaround.

[Pavel Bludov]

Hi Roman!
 

This supposed to be caught by pitest execution. Is this case is not used, we can remove it and non of UT notice it (pure pitest scenario) ... It is a bug in pitest. 

Please investigate this case.

It turns out that Pitest does not mutate switch cases by default. There is a REMOVE_SWITCH mutator that removes switch cases and, therefore, finds all uncovered:
https://github.com/hcoles/pitest/blob/master/pitest/src/main/java/org/pitest/mutationtest/engine/gregor/config/Mutator.java#L174
For some unknown reason, this mutator is not activated by default. Moreover, it is not documented. Perhaps there are some problems with this mutator.
To see the mutator in action, simply add this to the config:

<profile>
 <id>pitest-checks-metrics</id>
 <properties>
 <skipTests>true</skipTests>
 </properties>
 <build>
 <plugins>
 <plugin>
 <groupId>org.pitest</groupId>
 <artifactId>pitest-maven</artifactId>
 <version>${pitest.plugin.version}</version>
 <configuration>
 <mutators>
   <mutator>DEFAULTS</mutator>
   <mutator>REMOVE_SWITCH</mutator>
 </mutators>
 <targetClasses>
 <param>com.puppycrawl.tools.checkstyle.checks.metrics.*</param>
 </targetClasses>
 <targetTests>
 <param>com.puppycrawl.tools.checkstyle.checks.metrics.*</param>
 </targetTests>
 <coverageThreshold>100</coverageThreshold>
 <mutationThreshold>100</mutationThreshold>
 <timeoutFactor>${pitest.plugin.timeout.factor}</timeoutFactor>
 <timeoutConstant>${pitest.plugin.timeout.constant}</timeoutConstant>
 <threads>${pitest.plugin.threads}</threads>
 </configuration>
 </plugin>
 </plugins>
 </build>
</profile>

[Pavel Bludov]

Good news! Only four small commits need to build CS on JDK9:

1. Switch to powermock-api-mockito2 (Issue #5558)

2. Drop cobertura from pom.xml (there are still many Cobertura related issues besides pom.xml, but it's a different story)
   
3. One-line workaround for https://github.com/powermock/powermock/issues/864
4. Drop javax.xml.bind.annotation.XmlElement dependency from InputIndentationFromGuava.java

You can see them in my repo:

https://github.com/Softus/checkstyle/tree/jdk9

[Roman Ivanov]

good , just send PRs please.

clear code diff and CI status is better than multiple of words.