Enable error-prone static analysis checks #1266

jbduncan · 2018-01-25T21:52:37Z

Overview

Following issue #955 and closed PR #961, this is a PR to introduce error-prone checks to JUnit 5.

Feedback is welcome!

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes
All continuous integration builds pass

jbduncan · 2018-01-25T22:20:40Z

Hmm. My understanding of Vintage Test Engine is very limited, to say the least, so I'm stumped as to why one of its corresponding tests is failing.

Any thoughts would be appreciated. :)

sormuras · 2018-01-25T22:33:29Z

You only changed a single file in "vintage": TestRun.java https://github.com/junit-team/junit5/pull/1266/files#diff-fbff14085969788bb222d28b4e767dee

Does reverting the comparison help?

jbduncan · 2018-01-25T22:37:51Z

@sormuras It does, actually! I'll push a new commit with that section reverted.

sormuras · 2018-01-25T22:38:30Z

See the javadoc of lookupTestDescriptor(Description description):

<p>There are edge cases where multiple {@link Description Descriptions}
with the same {@code uniqueId} exist, e.g. when using overloaded methods
to define {@linkplain org.junit.experimental.theories.Theory theories}.
In this case, we try to find the correct {@link TestDescriptor} by
checking for object identity on the {@link Description} it represents.

jbduncan · 2018-01-25T22:39:56Z

Oh, doh! Thanks @sormuras. :P

I must've made that change thinking that error-prone was going to complain about the usage of ==. Reverting soon. :)

codecov · 2018-01-25T22:55:42Z

Codecov Report

Merging #1266 into master will decrease coverage by 0.02%.
The diff coverage is 73.68%.

@@             Coverage Diff              @@
##             master    #1266      +/-   ##
============================================
- Coverage     92.02%   91.99%   -0.03%     
- Complexity     3160     3163       +3     
============================================
  Files           296      296              
  Lines          7686     7695       +9     
  Branches        630      631       +1     
============================================
+ Hits           7073     7079       +6     
- Misses          447      451       +4     
+ Partials        166      165       -1

Impacted Files	Coverage Δ	Complexity Δ
...g/junit/platform/console/options/UriConverter.java	`100% <ø> (ø)`	`4 <0> (?)`
...latform/launcher/tagexpression/TagExpressions.java	`100% <ø> (+7.14%)`	`4 <0> (ø)`	⬇️
...ter/engine/descriptor/DynamicDescendantFilter.java	`100% <ø> (ø)`	`8 <0> (ø)`	⬇️
...rc/main/java/org/junit/jupiter/api/Assertions.java	`96.5% <ø> (ø)`	`115 <0> (ø)`	⬇️
...java/org/junit/platform/console/options/Theme.java	`92% <0%> (ø)`	`17 <0> (ø)`	⬇️
.../platform/launcher/tagexpression/ShuntingYard.java	`98.63% <0%> (ø)`	`29 <0> (ø)`	⬇️
...n/java/org/junit/platform/console/tasks/Color.java	`92.85% <0%> (ø)`	`8 <0> (ø)`	⬇️
...orm/console/tasks/VerboseTreePrintingListener.java	`92.3% <100%> (ø)`	`22 <0> (ø)`	⬇️
...va/org/junit/vintage/engine/execution/TestRun.java	`98.21% <100%> (+0.06%)`	`29 <2> (ø)`	⬇️
...java/org/junit/platform/engine/UniqueIdFormat.java	`88.88% <100%> (ø)`	`16 <1> (ø)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2128a4...cf18c7d. Read the comment docs.

jbduncan · 2018-02-20T23:31:38Z

Welp, I'm thoroughly stumped once again. For some reason, task compileTestJavaWithErrorProne is apparently not able to find classes written in Kotlin, and I don't know where I can find help to resolve the error.

@sormuras Do you have any ideas as to where I could seek help, or any other thoughts?

JLLeitschuh · 2018-02-21T15:31:37Z

@jbduncan There are two things that I've discovered when working with Kotlin that might help you.

The Kotlin compiler depends upon the Java AST to resolve the symbols. The Java compiler depends upon the compiled Kotlin bytecode to compile the Java code. So, the Java compiler needs to have the Kotlin bytecode added as an input so it can compile everything. The Kotlin Compiler modifies the Java compile task to add the compiled Kotlin Bytecode. You probably have to do this manually for the compileTestJavaWithErrorProne task. You can probably look at the Gradle Kotlin Plugin for suggestions on how to do this.

If this project is using the java-library plugin, then there was a time (this might not be a problem anymore) where Kotlin Code wasn't always included as an output of a project.
You had to add this hack to make it actually work (this uses Gradle Kotlin DSL but you can do the same in Groovy).

val compileKotlin: KotlinCompile by tasks
afterEvaluate {
    /*
     * Needed to configure kotlin to work correctly with the "java-library" plugin.
     * See:
     * https://docs.gradle.org/current/userguide/java_library_plugin.html#sec:java_library_known_issues
     */
    pluginManager.withPlugin("java-library") {
        configurations {
            "apiElements" {
                outgoing
                    .variants
                    .getByName("classes")
                    .artifact(mapOf(
                        "file" to compileKotlin.destinationDir,
                        "type" to "java-classes-directory",
                        "builtBy" to compileKotlin
                    ))
            }
        }
    }
}

jbduncan · 2018-02-22T12:12:03Z

@JLLeitschuh Wow, thanks a lot for the pointers! 👍 Both of the approaches you listed look promising to me, so I'll look into them when I'm next free.

jbduncan · 2018-02-22T16:31:26Z

@JLLeitschuh Many thanks for your help! As you suspected, the error-prone Java compiler needed the .class files produced by the Kotlin compiler, so after doing some digging around in the sources of the Gradle Kotlin Plugin, I discovered that adding the .class files to the classpath of the error-prone Java compiler fixes things.

Cheers! 🎉

jbduncan · 2018-02-22T18:06:14Z

@sormuras The JDK 10 build on Travis is complaining about being unable to find annotation method 'value()' in type 'Profile+Annotation': class file for jdk.Profile+Annotation not found when building :junit-platform-commons-java-9:compileJava.

I've done a Google search on jdk.Profile+Annotation, but I could only find this GitHub link: ceylon/ceylon-compiler#1347, which doesn't seem to help.

It's not clear to me if this is happening because of the changes I made to build.gradle or if it's a very obscure bug in Gradle or JDK 10. Do you have any insight or other thoughts on this?

sormuras · 2018-02-22T18:28:45Z

I have no idea.

jbduncan · 2018-02-22T23:46:51Z

Unless if anyone else in the JUnit 5 team has encountered this sort of error before (https://travis-ci.org/junit-team/junit5/jobs/345033831), I wonder if the best course of action in the meantime would be to make it optional for the JDK 10 build to pass.

WDYT @sormuras?

jbduncan · 2018-02-25T01:01:14Z

@sormuras I believe that the error is caused by error-prone issue google/error-prone#860.

error-prone uses a pretty recent Java compiler, but not the absolute bleeding-edge one, so it doesn't yet recognise Java 10 classfiles. I think that's the reason why Travis's JDK 10 build is producing strange messages. For that reason, I'm opting to only enable error-prone when running on JDK 9, and I'll work on making that change next.

jbduncan · 2018-02-25T02:46:50Z

I'm ready for another review whenever the JUnit team's ready. :)

…m usages to Array.stream(String.split(..., (0|-1)) so that we're explicit about trailing-empty-string-trimming behaviour)

This reverts commit 0eb0f65.

Currently, it doesn't run on Java 10. See google/error-prone#860

…ed errors"" This reverts commit 3ff428c.

…y reported errors""" This reverts commit 5c508ff.

…oduce too much churn. Also fix errors that were reintroduced in recent commit reverts.

…minor cleanup)

marcphilipp · 2018-03-13T16:52:22Z

@jbduncan Thanks for keeping this PR up-to-date and sorry if we've been stringing you along. My gut feeling is that using error-prone for this project might be more trouble than it's worth. We have to make sure that JUnit always works with the latest JDK, even EA releases. Naturally, error-prone lags behind in that regard. Thus, I think we should abandon this effort, at least for now.

@junit-team/junit-lambda Thoughts?

jbduncan · 2018-03-13T19:50:44Z

@marcphilipp No worries! I was aware that JUnit 5 is tested against the latest JDK releases, so I knew that there was risk that this PR wouldn't be accepted; I was keeping it up-to-date as I find this sort of thing enjoyable.

(And I was also secretly hoping that the fact that error-prone cannot keep up with the latest stable and EA releases would not be that big a problem. 😉)

If it is decided that this effort should be abandoned, then I'll accept that decision with grace. :)

JLLeitschuh · 2018-03-14T00:25:56Z

Isn't error prone a fork of the eclipse java compiler?

@jbduncan I'm sorry to hear this might get canned, I know you put a significant amount of effort into it.

jbduncan · 2018-03-14T00:33:10Z

@JLLeitschuh IIRC error-prone is actually a fork of OpenJDK's javac.

jbduncan · 2018-03-14T00:35:32Z

@JLLeitschuh I'd be interested to understand why you ask, though. :)

mmerdes · 2018-03-14T12:06:18Z

@marcphilipp
I agree with you.

sbrannen · 2018-03-14T13:16:39Z

Unfortunately ("unfortunate" because of the amount of effort invested in this by @jbduncan), I have to agree with @marcphilipp as well: it appears that staying on top of issues with error prone will be... well... error prone for the maintainers of the build.

In any case, thanks for all of the hard work, @jbduncan!

And... I'm glad you at least found the work "enjoyable". 😉

marcphilipp · 2018-03-14T13:24:46Z

I'm closing this PR since the team seems to be in consensus on this topic.

@jbduncan Sorry again and thanks for trying to convince us! I'm sure we can find another issue for you to work on if you want. 😉

jbduncan · 2018-03-14T15:10:38Z

And... I'm glad you at least found the work "enjoyable". 😉

Well, there were times where I got maddeningly stuck, as I think you already knew... 😉

...but overall I found the process very enjoyable, even though it was just a dead end all along. 😁

jbduncan · 2018-03-14T15:14:44Z

@marcphilipp Thanks! I'll likely decide to go back to #955 in the foreseeable future, either to choose another static analysis tool if any exist that follow the latest stable releases and/or EA releases, or to close the issue altogether if no such tools apparently exist. :)

JLLeitschuh · 2018-03-15T01:57:16Z

PMD as an alternative?

I'd be interested to understand why you ask, though.

Because I've found that the eclipse compiler is a PITA as well. It can't handle generic inference correctly in many cases. Wondering if I was going to deal with these same issues moving some of my projects forward to JDK 9 support.

jbduncan force-pushed the error-prone-investigation-4 branch from 3fe7ff1 to de2c216 Compare January 25, 2018 22:10

jbduncan force-pushed the error-prone-investigation-4 branch from 6ef3d12 to f134504 Compare February 11, 2018 20:33

jbduncan mentioned this pull request Feb 22, 2018

Prevent ConstructorLeaksThis false positive on constructor reference google/error-prone#789

Closed

jbduncan force-pushed the error-prone-investigation-4 branch from f5a2732 to 0c0da26 Compare February 22, 2018 16:33

jbduncan mentioned this pull request Feb 25, 2018

Upgrade javac to JDK10 to support classfile version 54 google/error-prone#860

Closed

jbduncan added 10 commits March 1, 2018 21:47

Enable error-prone static analysis checks

baa271b

Apply review feedback

5324f13

Does removing custom classpaths from error-prone tasks affect the CI?

03cf2fd

Classpath and source options are needed, after all

5c5783b

Turn on Var and Immutable checks, and fix many reported errors

200f4a5

Fix tasks so that they depend of .class results of Kotlin compiler

e8037a4

Fix all remaining reported problems (and change Pattern::splitToStrea…

ba35c71

…m usages to Array.stream(String.split(..., (0|-1)) so that we're explicit about trailing-empty-string-trimming behaviour)

Attempt to get the Kotlin side of things working with Java 10

cb04d24

Revert "Turn on Var and Immutable checks, and fix many reported errors"

64e9291

This reverts commit 0eb0f65.

Fix variable name

d48a9ac

jbduncan added 6 commits March 1, 2018 21:50

Require Java 9 to run error-prone

f4ee1d5

Currently, it doesn't run on Java 10. See google/error-prone#860

Revert "Revert "Turn on Var and Immutable checks, and fix many report…

1dcc518

…ed errors"" This reverts commit 3ff428c.

Revert "Revert "Revert "Turn on Var and Immutable checks, and fix man…

6bf91fe

…y reported errors""" This reverts commit 5c508ff.

Turn off @var check and remove error-prone annotations, since they pr…

bea83b8

…oduce too much churn. Also fix errors that were reintroduced in recent commit reverts.

Cleanup

300c617

Now that Kotlin runs on JDK 10 now, remove corresponding checks (and …

cf18c7d

…minor cleanup)

jbduncan force-pushed the error-prone-investigation-4 branch from de7579f to cf18c7d Compare March 1, 2018 23:13

marcphilipp added the status: team discussion label Mar 13, 2018

marcphilipp removed the status: team discussion label Mar 14, 2018

marcphilipp closed this Mar 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable error-prone static analysis checks #1266

Enable error-prone static analysis checks #1266

jbduncan commented Jan 25, 2018 •

edited

Loading

jbduncan commented Jan 25, 2018

sormuras commented Jan 25, 2018 •

edited

Loading

jbduncan commented Jan 25, 2018

sormuras commented Jan 25, 2018

jbduncan commented Jan 25, 2018

codecov bot commented Jan 25, 2018 •

edited

Loading

jbduncan commented Feb 20, 2018

JLLeitschuh commented Feb 21, 2018 •

edited

Loading

jbduncan commented Feb 22, 2018

jbduncan commented Feb 22, 2018

jbduncan commented Feb 22, 2018 •

edited

Loading

sormuras commented Feb 22, 2018

jbduncan commented Feb 22, 2018

jbduncan commented Feb 25, 2018

jbduncan commented Feb 25, 2018

marcphilipp commented Mar 13, 2018 •

edited by sbrannen

Loading

jbduncan commented Mar 13, 2018 •

edited

Loading

JLLeitschuh commented Mar 14, 2018

jbduncan commented Mar 14, 2018

jbduncan commented Mar 14, 2018

mmerdes commented Mar 14, 2018

sbrannen commented Mar 14, 2018

marcphilipp commented Mar 14, 2018

jbduncan commented Mar 14, 2018

jbduncan commented Mar 14, 2018

JLLeitschuh commented Mar 15, 2018

Enable error-prone static analysis checks #1266

Enable error-prone static analysis checks #1266

Conversation

jbduncan commented Jan 25, 2018 • edited Loading

Overview

Definition of Done

jbduncan commented Jan 25, 2018

sormuras commented Jan 25, 2018 • edited Loading

jbduncan commented Jan 25, 2018

sormuras commented Jan 25, 2018

jbduncan commented Jan 25, 2018

codecov bot commented Jan 25, 2018 • edited Loading

Codecov Report

jbduncan commented Feb 20, 2018

JLLeitschuh commented Feb 21, 2018 • edited Loading

jbduncan commented Feb 22, 2018

jbduncan commented Feb 22, 2018

jbduncan commented Feb 22, 2018 • edited Loading

sormuras commented Feb 22, 2018

jbduncan commented Feb 22, 2018

jbduncan commented Feb 25, 2018

jbduncan commented Feb 25, 2018

marcphilipp commented Mar 13, 2018 • edited by sbrannen Loading

jbduncan commented Mar 13, 2018 • edited Loading

JLLeitschuh commented Mar 14, 2018

jbduncan commented Mar 14, 2018

jbduncan commented Mar 14, 2018

mmerdes commented Mar 14, 2018

sbrannen commented Mar 14, 2018

marcphilipp commented Mar 14, 2018

jbduncan commented Mar 14, 2018

jbduncan commented Mar 14, 2018

JLLeitschuh commented Mar 15, 2018

jbduncan commented Jan 25, 2018 •

edited

Loading

sormuras commented Jan 25, 2018 •

edited

Loading

codecov bot commented Jan 25, 2018 •

edited

Loading

JLLeitschuh commented Feb 21, 2018 •

edited

Loading

jbduncan commented Feb 22, 2018 •

edited

Loading

marcphilipp commented Mar 13, 2018 •

edited by sbrannen

Loading

jbduncan commented Mar 13, 2018 •

edited

Loading