Enable error-prone static analysis checks #961

jbduncan · 2017-07-18T14:55:51Z

Overview

Following issue #955, this is a work-in-progress 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 · 2017-07-18T16:42:13Z

Work on this PR has been postponed until google/error-prone#685 is resolved.

jbduncan · 2017-07-18T20:03:49Z

google/error-prone#685 has been resolved, but it hasn't had time to make it into an error-prone release yet, so I'll see if I can use an earlier version of error-prone.

smoyer64 · 2017-07-19T02:24:08Z

Related to #358 ... and related to the implementation it might actually close that issue.

I don't really care which library is chosen to perform this checking ... at this point FindBugs (and JSR-305 itself) seems dead. Note that JSR-305 has been implemented quite a few times. This is something I intend to ask the JCP executive committee at JavaOne this year.

ErrorProne and SpotBugs both support the JSR-305 proposal, so picking ErrorProne is probably a good decision. There was also a good discussion in JUnit Pioneer about this (junit-pioneer/junit-pioneer#33) and I think that we'll probably just follow the JUnit team's lead.

talios · 2017-07-19T03:54:56Z

@smoyer64 it seems spotbugs is coming along nicely, and should soon be poised to be a full replacement for findbugs. Findbugs/spotbugs serves a similar, but different purpose as well - so including both might be useful to consider ( along with fb-contrib, find-sec-bugs extensions).

smoyer64 · 2017-07-19T13:35:59Z

@talios - I'm a big fan of find-sec-bugs so I should have specifically mentioned that plugin package. The fb-contrib plugins also look great and I'm thinking I should add these to my parent POM.

jbduncan · 2017-07-19T21:11:22Z

@talios I am actually considering Spotbugs as well. I think it or FindBugs would work well with error-prone, since error-prone itself doesn't catch everything that FindBugs does, and vice-versa.

@smoyer64 For null checking, I'll look into the Checker Framework. Its null checking is exhaustive (which is what makes it a formal verification tool as opposed to just a static analysis tool), whereas FindBugs' null checking isn't.

Also, I'm not sure if either of you have seen ~~#961~~ #955, but there I briefly talk about which tools I want to introduce. Just thought I'd let you both know, FYI. :)

jbduncan · 2017-07-19T21:51:11Z

Oh that's a bummer; error-prone doesn't seem to work yet on JDK 9, so Travis is complaining about it.

I'll see if I can find a way in the meantime of disabling error-prone when running on JDK 9.

jbduncan · 2017-07-20T08:45:15Z

@sbrannen @marcphilipp I've made some comments in my recent commits asking about certain things related to error-prone which I'd quite like both your feedback on. Would you mind taking a look?

jbduncan · 2017-07-25T11:48:50Z

@sbrannen @marcphilipp FYI, the reason static was applied to many methods and the parameters in a number of methods were rearranged was to satisfy the checks http://errorprone.info/bugpattern/MethodCanBeStatic and http://errorprone.info/bugpattern/InconsistentOverloads.

If you'd like me to revert these changes because e.g. they're too invasive, please do let me know.

marcphilipp · 2017-07-27T12:01:42Z

I'm not a fan of MethodsCanBeStatic to be honest. The description of InconsistentOverloads is very... ehm... concise. What does it try to accomplish?

Which rule caused you to reorder parameters of a lot of methods?

jbduncan · 2017-07-29T17:06:29Z

@marcphilipp I agree that the description of InconsistentOverloads is... concise. 😉

IIRC, it reports warnings when two or more methods with the same name (overloads) don't start with the exact same sequence of parameters. For example, this passes without warning because the two overloads' common subset of parameters (a, b, c) have the same names and types, occur at the start of their respective parameter lists, and have the exact same order as each other:

public m(int a, double b, long c) {}
public m(int a, double b, long c, Object d) {}

However this example fails because for the second overload, parameter c is now located after d, breaking at least one of the requirements above:

public m(int a, double b, long c) {}
public m(int a, double b, Object d, long c) {}

So to answer your 2nd question, it was the InconsistentOverloads rule which prompted me to reorder parameters in a lot of methods.

jbduncan · 2017-07-29T17:08:47Z

I'm not a fan of MethodsCanBeStatic to be honest.

Okey dokey! Would you like me to revert the changes I made to satisfy that rule? 😃

jbduncan · 2017-07-29T18:26:43Z

Would you also like me to revert the changes I made for InconsistentOverloads?

jbduncan · 2017-07-29T18:37:05Z

Note, I've just rebased this PR on master and pushed two new commits - one a filename cleanup commit (which was caught by error-prone), the other an attempt to enable error-prone's "Immutable" check.

jbduncan · 2017-07-30T21:05:19Z

The "Immutable" check fails on JDK 9, just like my earlier attempt with the "Var" check. So I've pushed a new commit which disables all checks that depend on error-prone annotations.

jbduncan · 2017-08-07T18:21:17Z

@sbrannen I understand that @marcphilipp is on holiday, so I wondered if you could confirm for me in his stead if I should disable and revert the MethodsCanBeStatic and InconsistentOverloads rules discussed above?

sbrannen · 2017-08-10T15:25:49Z

I'm actually also on vacation and therefore haven't had a chance to look at this in detail yet.

But I'll see if I can get to it within the next week or so; otherwise, one of us will get back to you after our vacations. 😉

jbduncan · 2017-08-10T19:50:19Z

@sbrannen

I'm actually also on vacation and therefore haven't had a chance to look at this in detail yet.

Oops! Pardon me. For some reason I thought you never went on holiday or finished early, which was why I asked for your confirmation specifically. 😜

Tell you what, I'll go and disable both of those rules as soon as I have the time, since they don't seem to buy much, and I believe disabling both of them would make reviewing this PR much easier. After all, they can always be re-enabled at a later point in time if desired.

jbduncan · 2017-08-10T21:17:07Z

Okay, those two rules and their respective changes have been reverted.

sbrannen · 2017-08-19T11:21:15Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertTrue.java

+		/* no-op */
+	}
+	///CLOVER:ON
+


Would you mind moving all of theses private constructors in Assert* classes to a separate PR?

We can then merge that new PR immediately.

Certainly. Should be done and ready for merging over at #1027.

@sbrannen Ah, I remember now why I added these private constructors in this PR rather than a separate one in the first place. It's because error-prone purposely fails to compile whenever classes like Assert* don't have associated private constructors.

The error message that error-prone reports for me is as follows:

C:\Users\Jonathan\dev\Java\IntelliJ Projects\junit5\junit-jupiter-api\src\main\java\org\junit\jupiter\api\AssertFalse.java:24: warning: [PrivateConstructorForUtilityClass] Utility classes (only static members) are not designed to be instantiated and should be made noninstantiable with a default constructor. class AssertFalse { ^ (see http://errorprone.info/bugpattern/PrivateConstructorForUtilityClass) Did you mean to remove this line?

Consequently, Travis CI won't go green until #1027 gets merged into master and I can rebase this PR on top of it.

#1027 has been merged.

So feel free to rebase on master at your leisure.

sbrannen · 2017-08-19T11:23:20Z

build.gradle

@@ -9,10 +9,13 @@ buildscript {
 	}
 	dependencies {
 		classpath 'com.diffplug.spotless:spotless-plugin-gradle:3.4.1'
+		classpath 'com.github.ben-manes:gradle-versions-plugin:0.15.0'


I just moved that declaration in e6825ea.

So you can rebase on master at your leisure. 😉

sbrannen · 2017-08-19T11:24:47Z

build.gradle

+	// disable, similarly to how we specify "-Xlint" flags above.
+	// Do we want to customise exactly which checks error-prone should run? Or are we happy to just
+	// accept the default checks that each new release brings, explictly excluding the ones we dislike?
+	if (JavaVersion.current() == JavaVersion.VERSION_1_8 && subproj.name != 'documentation') {


I'd say, for now, we can "just accept the default checks that each new release brings, explictly excluding the ones we dislike."

Great, thanks for clarifying things for me @sbrannen! I'll work on reverting this comment now.

sbrannen · 2017-08-19T11:25:56Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertionUtils.java

+		}
+		else {
+			return String.valueOf(obj);
+		}


This seems to be an unrelated "fix".

Can you please move this to a separate PR outlining the rationale for the change?

Can you please move this to a separate PR outlining the rationale for the change?

See https://github.com/junit-team/junit5/pull/961/files#r134096347 for the rationale. :)

sbrannen · 2017-08-19T11:27:02Z

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AssertionUtils.java

+	static boolean floatsAreEqual(float value1, float value2) {
+		return Float.floatToIntBits(value1) == Float.floatToIntBits(value2);
+	}
+


Why did you move this method?

I moved it because, before I added "-Xep:UngroupedOverloads:OFF" to the build.gradle file, error-prone reported that methods like this one were not 'grouped' together with methods of the same name, potentially causing confusion for future maintainers.

I ultimately added "-Xep:UngroupedOverloads:OFF" to the build.gradle because the corresponding rule produced a lot of churn for little gain, but as I was reverting the changes I made initially to satisfy the rule, I thought that moving all the floatsAreEqual() methods together made a great deal of sense, so I decided to keep it.

If you want me to move it back to where it was, or to put this change in a new PR, please let me know!

You can leave it "as is" for the time being.

sbrannen · 2017-08-19T12:25:28Z

junit-jupiter-engine/src/test/java/org/junit/jupiter/api/AssertionsFailTests.java

@@ -128,7 +128,7 @@ void failWithNullStringAndThrowable() {
 	@Test
 	void failUsableAsAnExpression() {
 		// @formatter:off
-		long count = Collections.emptySet().stream()
+		long count = Stream.empty()
 				.peek(element -> fail("peek should never be called"))
 				.filter(element -> fail("filter should never be called", new Throwable("cause")))
 				.map(element -> fail(new Throwable("map should never be called")))


Addressed in db33cc1.

Ta! I'll revert this particular change then.

sbrannen · 2017-08-19T12:25:35Z

...-engine/src/test/java/org/junit/jupiter/engine/discovery/DiscoverySelectorResolverTests.java

-			"Too few test descriptors in classpath");
+		assertThat(engineDescriptor.getDescendants().size()) //
+				.as("Too few test descriptors in classpath") //
+				.isGreaterThan(150);



Addressed in db33cc1.

Thanks again! Reverting this change...

sbrannen · 2017-08-19T12:25:41Z

...er-engine/src/test/java/org/junit/jupiter/engine/discovery/predicates/IsInnerClassTests.java

@@ -49,7 +49,7 @@ void privateInnerClassEvaluatesToFalse() {

 }

-//class name must not end with 'Tests', otherwise it would be picked up by the suite
+// Class name must not end with 'Tests', otherwise it would be picked up by the suite
 class ClassWithInnerClasses {


Addressed in db33cc1.

sbrannen · 2017-08-19T12:25:45Z

...gine/src/test/java/org/junit/jupiter/engine/discovery/predicates/IsNestedTestClassTests.java

@@ -50,7 +50,7 @@ void privateNestedClassEvaluatesToFalse() {

 }

-//class name must not end with 'Tests', otherwise it would be picked up by the suite
+// Class name must not end with 'Tests', otherwise it would be picked up by the suite


Addressed in db33cc1.

sbrannen · 2017-08-19T12:25:54Z

...rationsupport/src/test/java/org/junit/jupiter/migrationsupport/rules/FailAfterAllHelper.java

@@ -13,7 +13,7 @@
 class FailAfterAllHelper {

 	static void fail() {
-		//hack: use this blacklisted exception to fail the build, all others would be swallowed...
+		// hack: use this blacklisted exception to fail the build, all others would be swallowed...
 		throw new OutOfMemoryError("a postcondition was violated");
 	}


Addressed in db33cc1.

sbrannen · 2017-08-19T12:28:48Z

@jbduncan,

Oops! Pardon me. For some reason I thought you never went on holiday or finished early, which was why I asked for your confirmation specifically. 😜

No worries. 😉

Tell you what, I'll go and disable both of those rules as soon as I have the time, since they don't seem to buy much, and I believe disabling both of them would make reviewing this PR much easier. After all, they can always be re-enabled at a later point in time if desired.

Sounds good.

FYI: I also pushed some of your recommended changes in various "Polishing" commits and requested that you move some of your additional unrelated changes to a separate. So the results of those two actions will also make this PR easier to swallow.

jbduncan · 2017-08-19T16:09:55Z

FYI: I also pushed some of your recommended changes in various "Polishing" commits and requested that you move some of your additional unrelated changes to a separate. So the results of those two actions will also make this PR easier to swallow.

Great! Sounds very reasonable to me, so I'll happily create separate PRs for the corresponding comments and do whatever else needs to be done to cleanup this PR.

I intend to process your comments above as soon as I can, but I'm currently going through a busy time IRL, so it might take me a few days to process everything.

sbrannen · 2017-08-19T16:25:03Z

I intend to process your comments above as soon as I can, but I'm currently going through a busy time IRL, so it might take me a few days to process everything.

Sure. No worries, no hurry.

But... it'd be nice to have the polishing commits in place before RC3. 😉

…o/bugpattern/ImmutableEnumChecker

future release of error-prone anyway. Also, disable use of error-prone in `./gradlew compile[Test]Java` and introduce experimental tasks for compiling java sources with error-prone.

…haviour again

…h problems that I'm struggling to resolve...)

`compileJavaWithErrorProne` works now, but problems still persist with `compileTestJavaWithErrorProne`.

jbduncan · 2017-12-11T19:46:59Z

@marcphilipp @JLLeitschuh Thank you very much for your thoughts on this! I've found that making compile[Test]JavaWithErrorProne depend on compile[Test]Kotlin has fixed the problem I was experiencing.

(FYI, I've been busy with a new job, which is why it took me so long to get back to both of you. I hope you'll accept my apologies.)

jbduncan · 2017-12-11T20:35:39Z

...but it seems I have encountered yet another stumbling block.

Travis, AppVeyor and my local setup seem to complain that they can't find any Java 9 related classes when compiling subproject junit-platform-commons-java-9. I have tried surrounding my error-prone config with the following if statement to disable error-prone for that subproject (as in fe8c123), but it doesn't seem to be working, at least locally.

if (!project.name.endsWith('-java-9')
		// error-prone doesn't work with Java 9 sources yet:
		// https://github.com/google/error-prone/issues/448
		&& subproj.name != 'documentation') {

  apply plugin: 'net.ltgt.errorprone-base'

  [...]

}

Do y'all have any further thoughts on this?

sormuras · 2017-12-11T21:01:55Z

Wait until errorprone supports Java 9? ;)

Mh, why are checking project.name ... shouldn't it be subproj.name as well?

jbduncan · 2017-12-11T23:09:09Z

Mh, why are checking project.name ... shouldn't it be subproj.name as well?

Good catch @sormuras!

Sadly, it hasn't worked. :,(

Wait until errorprone supports Java 9? ;)

;)

I'll persevere with this PR for a bit longer, methinks. This camel can carry more straws on its back. ;) (I'll just leave it for another day now, as it's getting late here in the UK and my mind is dozing off.)

If you or @marcphilipp have any further thoughts that may allow me to get unstuck again, I would be immensely grateful.

…thers

jbduncan · 2018-01-25T21:45:40Z

It's getting prohibitively difficult for me to continuously rebase this PR over master, and the size of this PR has reached a point where I can't easily debug problems, so I'm closing it with the intention of re-opening it in a new PR.

Prior to this commit, AssertionUtils.toString(Object) printed an array via the array's toString() method which resulted in non-user-friendly output. This commit addresses this issue by printing arrays using Arrays.toString(), which produces human readable output. This is a prerequisite for junit-team#961. Issue: junit-team#1030

jbduncan mentioned this pull request Jul 18, 2017

Bug report: unhandled NullPointerException google/error-prone#685

Closed

jbduncan mentioned this pull request Jul 21, 2017

Consider breaking down Gradle build into sub-scripts for maintainability #972

Closed

1 task

jbduncan force-pushed the error-prone branch 2 times, most recently from 9d5c483 to ceb3d4d Compare July 22, 2017 15:52

jbduncan force-pushed the error-prone branch from 62c2d06 to 93d89f4 Compare July 29, 2017 18:35

jbduncan mentioned this pull request Aug 3, 2017

Introduce Kotlin Assertion Helper Library #924

Closed

2 tasks

jbduncan mentioned this pull request Aug 16, 2017

Bug report: Unhandled NoSuchElementException google/error-prone#707

Closed

sbrannen requested changes Aug 19, 2017

View reviewed changes

jbduncan added 15 commits December 11, 2017 18:46

Upgrade errorprone to latest version

a9efe7e

Update EnumSource to follow the advice given in http://errorprone.inf…

34573c2

…o/bugpattern/ImmutableEnumChecker

Comment fixup

387a662

Comment fixup

0e770db

Update to latest error-prone plugin version

adbeeaa

Externalise error-prone version

ed3f5ce

Attempt to get error-prone working on JDK 9

630719b

Move 'propdeps-maven' back to its original location (as per review)

fd74fea

Update to error-prone plugin v0.0.13

c66367d

Just disable "literal name check" checker, as it will be removed in a

3e75ef9

future release of error-prone anyway. Also, disable use of error-prone in `./gradlew compile[Test]Java` and introduce experimental tasks for compiling java sources with error-prone.

Remove custom error-prone tasks and use default error-prone plugin be…

95e235a

…haviour again

Create custom error-prone tasks again (unfortunately they're rife wit…

c08c189

…h problems that I'm struggling to resolve...)

Attempt 1 to fix problems with compile[Test]JavaWithErrorProne

038e789

Push newest changes regarding compile[Test]JavaWithErrorProne.

8bca358

`compileJavaWithErrorProne` works now, but problems still persist with `compileTestJavaWithErrorProne`.

Make compileJavaWithErrorProne depend on compileKotlin & fix errors

31add43

jbduncan force-pushed the error-prone branch from 93cb69f to 31add43 Compare December 11, 2017 19:18

Apply Spotless (oops!)

2beb69b

Attempt to fix problem with junit-platform-commons-java-9

fe8c123

WIP

20bac2e

jbduncan added 2 commits January 15, 2018 03:11

WIP - update version, minor cleanup in some areas, and more mess in o…

bdec69c

…thers

Merge remote-tracking branch 'upstream/master' into error-prone

82767e8

jbduncan mentioned this pull request Jan 25, 2018

Enable error-prone static analysis checks #1266

Closed

7 tasks

jbduncan closed this Jan 26, 2018

smoyer64 mentioned this pull request May 30, 2019

Clarify when returned values might be null #358

Open

4 tasks

Enable error-prone static analysis checks #961

Enable error-prone static analysis checks #961

Conversation

jbduncan commented Jul 18, 2017 • edited Loading

Overview

Definition of Done

jbduncan commented Jul 18, 2017

jbduncan commented Jul 18, 2017

smoyer64 commented Jul 19, 2017

talios commented Jul 19, 2017

smoyer64 commented Jul 19, 2017

jbduncan commented Jul 19, 2017 • edited Loading

jbduncan commented Jul 19, 2017 • edited Loading

jbduncan commented Jul 20, 2017

jbduncan commented Jul 25, 2017

marcphilipp commented Jul 27, 2017

jbduncan commented Jul 29, 2017 • edited Loading

jbduncan commented Jul 29, 2017

jbduncan commented Jul 29, 2017

jbduncan commented Jul 29, 2017

jbduncan commented Jul 30, 2017

jbduncan commented Aug 7, 2017 • edited Loading

sbrannen commented Aug 10, 2017

jbduncan commented Aug 10, 2017

jbduncan commented Aug 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbrannen commented Aug 19, 2017

jbduncan commented Aug 19, 2017

sbrannen commented Aug 19, 2017

jbduncan commented Dec 11, 2017

jbduncan commented Dec 11, 2017

sormuras commented Dec 11, 2017

jbduncan commented Dec 11, 2017

jbduncan commented Jan 25, 2018

jbduncan commented Jul 18, 2017 •

edited

Loading

jbduncan commented Jul 19, 2017 •

edited

Loading

jbduncan commented Jul 19, 2017 •

edited

Loading

jbduncan commented Jul 29, 2017 •

edited

Loading

jbduncan commented Aug 7, 2017 •

edited

Loading