Enable errorprone globally #5716
Conversation
build_rules.gradle
Outdated
| @@ -560,6 +560,10 @@ ext.applyJavaNature = { | |||
| tasks.withType(JavaCompile) { | |||
| options.compilerArgs += "-XepDisableWarningsInGeneratedCode" | |||
| } | |||
| dependencies { | |||
There was a problem hiding this comment.
Should this be outside the enableErrorProne block? My understanding is that these annotations are for Findbugs.
There was a problem hiding this comment.
Well these imports were mostly added to remove warnings on module-info.java files reported by error-prone. I can relocate it if you prefer but I have the impression that this scope is tighter.
There was a problem hiding this comment.
I don't have a strong preference. Maybe add a comment explaining why it's here since it's not totally obvious why ErrorProne usage should require a FindBugs dependency.
| @@ -255,6 +255,7 @@ public Integer answer(InvocationOnMock invocation) { | |||
| * is invoked. | |||
| */ | |||
| @Test | |||
| @SuppressWarnings("AssertionFailureIgnored") | |||
There was a problem hiding this comment.
Can this test be improved such that we don't need this suppression? The ErrorProne AssertionFailureIgnored page suggests using JUnit's assertThrows().
There was a problem hiding this comment.
I did not find an easy way to do this and also the proposed solution by errorprone is invalid. See google/error-prone#1059
Any suggestions are welcome.
There was a problem hiding this comment.
You could re-create the idea behind assertThrows with:
Throwable failure = null;
try {
getRequest.execute();
} catch (Throwable e) {
failure = e;
}
assertNotNull("Expected execute to throw an exception", failure);
assertThat(failure, Matchers.instanceOf(SocketTimeoutException.class));
assertEquals(1 + defaultNumberOfRetries, executeCount.get());
expectedLogs.verifyWarn("performed 10 retries due to IOExceptions");There was a problem hiding this comment.
I don't know why I was thinking in a way more complex idea. Thanks, added this.
build_rules.gradle
Outdated
| @@ -344,7 +344,7 @@ class JavaNatureConfiguration { | |||
| /** Controls whether the errorprone plugin is enabled and configured */ | |||
| boolean enableErrorProne = true | |||
| /** Controls whether compiler warnings are treated as errors */ | |||
| boolean failOnWarning = false | |||
There was a problem hiding this comment.
My preference is to remove this configuration altogether and make the failOnWarning behavior default.
There was a problem hiding this comment.
We can't remove this for the moment we have modules still skipping using this e.g. 'sdks/java/core'.
There was a problem hiding this comment.
Got it. I'll work on finishing up sdks/java/core as I have time and then remove this. /cc @pabloem
| @@ -255,6 +255,7 @@ public Integer answer(InvocationOnMock invocation) { | |||
| * is invoked. | |||
| */ | |||
| @Test | |||
| @SuppressWarnings("AssertionFailureIgnored") | |||
There was a problem hiding this comment.
You could re-create the idea behind assertThrows with:
Throwable failure = null;
try {
getRequest.execute();
} catch (Throwable e) {
failure = e;
}
assertNotNull("Expected execute to throw an exception", failure);
assertThat(failure, Matchers.instanceOf(SocketTimeoutException.class));
assertEquals(1 + defaultNumberOfRetries, executeCount.get());
expectedLogs.verifyWarn("performed 10 retries due to IOExceptions");
build_rules.gradle
Outdated
| @@ -344,7 +344,7 @@ class JavaNatureConfiguration { | |||
| /** Controls whether the errorprone plugin is enabled and configured */ | |||
| boolean enableErrorProne = true | |||
| /** Controls whether compiler warnings are treated as errors */ | |||
| boolean failOnWarning = false | |||
There was a problem hiding this comment.
Got it. I'll work on finishing up sdks/java/core as I have time and then remove this. /cc @pabloem
iemejia
force-pushed
the
enable-errorprone-globally
branch
2 times, most recently
from
9edfa0a
to
1b84c6b
Compare
iemejia
force-pushed
the
enable-errorprone-globally
branch
from
1b84c6b
to
580767d
Compare
iemejia
force-pushed
the
enable-errorprone-globally
branch
from
580767d
to
359110e
Compare
|
retest this please |
|
Addressed all comments, rebased after the plugin changes, and merged, only missing thing now is 'sdks/java/core'. Probably it would be a good idea to submit an email when finished @swegner to notify and thank the contributors to this. |
|
Makes sense to me-- I can send a notification once we're fully-migrated. I probably won't get to the remaining |
R: @swegner