-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix nullaway convention plugin #14811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| // For test compilation tasks, disable errorprone entirely to avoid nullaway issues | ||
| options.errorprone { | ||
| isEnabled.set(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd only disable errorprone for test source if nullaway is enabled for the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we are only disabling error prone for test sources if the module has the plugin installed viaid("otel.nullaway-conventions")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, so errorprone will still run on those test source, just via a different mechanism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this will turn error prone off entirely for those particular test sources. I assumed it would be ok to not run it on test code, but maybe that was an incorrect assumption on my part. I couldn't figure out a way to just disable nullaway without disabling errorprone entirely, but I can keep digging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work
tasks {
withType<JavaCompile>().configureEach {
options.errorprone.nullaway {
if (name.contains("test", ignoreCase = true)) {
disable()
} else {
error()
}
customInitializerAnnotations.add("org.openjdk.jmh.annotations.Setup")
excludedFieldAnnotations.add("org.mockito.Mock")
excludedFieldAnnotations.add("org.mockito.InjectMocks")
}
}
}
Fixes #14771
While doing some development locally I realized that changes I was making to test code was not being reflected, and I noticed that we had an actual test failure merged in
mainthat was not failing in CI.I think previously we were disabling the entire module instead of just nullaway/errorprone. This change fixes that.
I think every module that uses nullaway might not be actually executing tests as of right now. This should fix it.