Skip to content

Conversation

@jaydeluca
Copy link
Member

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 main that 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.

Comment on lines 22 to 24
// For test compilation tasks, disable errorprone entirely to avoid nullaway issues
options.errorprone {
isEnabled.set(false)
Copy link
Member

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?

Copy link
Member Author

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")

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

@zeitlinger zeitlinger Sep 26, 2025

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")
    }
  }
}

@jaydeluca jaydeluca marked this pull request as ready for review September 26, 2025 17:38
@jaydeluca jaydeluca requested a review from a team as a code owner September 26, 2025 17:38
@trask trask merged commit 4bc7282 into open-telemetry:main Sep 26, 2025
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix Apache HttpClient telemetry collection

3 participants