Simplified set of CI jobs #1271
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughCI matrix reduced to macOS and Windows; build step unified to run ./gradlew build and jacoco gating broadened; Maven-local publish gating loosened. Gradle test conventions centralize JVM --add-exports/opens flags, change tested JDKs (21→17), update disabled test tasks, and remove a version-specific test exclusion in NullAway. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant GH as GitHub Actions
participant R as Runner (macOS / Windows / Linux)
participant G as Gradle (./gradlew)
Dev->>GH: push / open PR
GH->>R: start job (matrix: macOS, Windows)
R->>G: ./gradlew build
Note right of G: Uses centralized jvmArgs (commonJavacOpens)\nRuns test tasks (testJdk17, testJdk24)
alt repo == uber/NullAway and runner == Linux
R->>G: ./gradlew jacocoAggregate
Note right of G: Coverage aggregation runs
end
R->>G: publishToMavenLocal (no matrix gating)
R->>R: git clean / format check
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1271 +/- ##
=========================================
Coverage 88.55% 88.55%
Complexity 2431 2431
=========================================
Files 91 91
Lines 8013 8013
Branches 1598 1598
=========================================
Hits 7096 7096
Misses 458 458
Partials 459 459 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle (1)
81-101: Deduplicate repeated --add-exports/--add-opens JVM args.The same block appears in both test and testJdkX tasks; pull into a shared list to avoid drift.
Example:
+def commonJavacOpens = [ + "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", + "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", + "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", +]Then:
- jvmArgs += [ ...same list... ] + jvmArgs += commonJavacOpensjdk-recent-unit-tests/build.gradle (1)
57-59: Disable testJdk17 here — good catch to match the new extra-test set.Minor nit: the header comment still says “Use JDK 21…”, but the toolchain is 24. Update it for clarity.
Outside selected lines, apply:
-// Use JDK 21 for this module, via a toolchain +// Use JDK 24 for this module, via a toolchain
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.github/workflows/continuous-integration.yml(2 hunks)buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle(1 hunks)jdk-recent-unit-tests/build.gradle(1 hunks)nullaway/build.gradle(0 hunks)
💤 Files with no reviewable changes (1)
- nullaway/build.gradle
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradlejdk-recent-unit-tests/build.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on ubuntu-latest
🔇 Additional comments (5)
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle (1)
68-106: JDK test sweep updated to [17,24] — aligns with stated CI policy.This correctly shifts extra test tasks from 21→17 while keeping 24. The onlyIf gate to latest EP is preserved. LGTM.
jdk-recent-unit-tests/build.gradle (1)
64-71: Gate main tests to latest EP on JDK 24+ — consistent with plan.This prevents running unsupported combos on this module. LGTM.
.github/workflows/continuous-integration.yml (3)
12-20: OS matrix includes Linux, macOS, Windows — matches PR objectives.Job naming is simplified; no loss of clarity. LGTM.
32-33: Single “./gradlew build” must still trigger all intended JDK/EP variants.Confirm that:
- testJdk17/testJdk24 are wired into check via the conventions plugin across modules,
- the “core NullAway + EP 2.14.0 on JDK 11” suite is invoked during build via toolchains (no longer via CI matrix).
41-41: Coverage upload gated to Linux and main repo — sensible.Reduces duplicate uploads and avoids secrets on forks. LGTM.
Collaborator
Author
|
@yuxincs we will have to update required CI job names before landing this one |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle (1)
64-65: Prefer jvmArgs.addAll(...) to avoid Groovy DSL +='gotchas'.Using += relies on Groovy list semantics; addAll is clearer and avoids null/overload edge cases.
- jvmArgs += commonJavacOpens + jvmArgs.addAll(commonJavacOpens)And similarly for the other test tasks:
- jvmArgs += commonJavacOpens + jvmArgs.addAll(commonJavacOpens)Also applies to: 86-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle(2 hunks)jdk-recent-unit-tests/build.gradle(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- jdk-recent-unit-tests/build.gradle
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
Applied to files:
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build spring-framework with snapshot
- GitHub Check: Build caffeine with snapshot
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (1)
buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle (1)
68-92: JDK matrix update to [17, 24] aligns with the new CI policy.Good shift: JDK 17 covers older LTS supported by latest Error Prone; JDK 24 covers latest GA. The gating on latest EP via onlyIf looks correct.
Please confirm CI toolchains provision 17 and 24 on all OS runners and that the “oldest EP on JDK 11” job lives elsewhere in the workflow, to fully match the PR objectives.
Comment on lines
+47
to
+60
| // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer | ||
| def commonJavacOpens = [ | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", | ||
| "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", | ||
| "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", | ||
| "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", | ||
| "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", | ||
| ] |
Contributor
There was a problem hiding this comment.
Add missing --add-exports for javac code/comp (otherwise EP breaks on JDK 17/24).
Error Prone references types in com.sun.tools.javac.code and .comp at compile time; opening is not enough. Without corresponding --add-exports for these packages, you’ll hit “package is not visible”/IllegalAccess errors on newer JDKs.
Apply this diff to complete the set:
def commonJavacOpens = [
"--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED",
+ "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
+ "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED",
"--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED",
"--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED",
]Nit: consider renaming variable to commonJavacArgs since it contains both exports and opens.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer | |
| def commonJavacOpens = [ | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", | |
| ] | |
| // to expose necessary JDK types on JDK 16+; see https://errorprone.info/docs/installation#java-9-and-newer | |
| def commonJavacOpens = [ | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.model=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.processing=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED", | |
| "--add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED", | |
| "--add-opens=jdk.compiler/com.sun.tools.javac.jvm=ALL-UNNAMED", | |
| ] |
🤖 Prompt for AI Agents
In buildSrc/src/main/groovy/nullaway.java-test-conventions.gradle around lines
47 to 60, add missing
"--add-exports=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED" and
"--add-exports=jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED" to the
commonJavacOpens list so Error Prone can access those packages at compile time;
optionally rename the variable to commonJavacArgs to reflect that it contains
both --add-exports and --add-opens entries.
Collaborator
Author
There was a problem hiding this comment.
these opens are just used for running tests
Contributor
There was a problem hiding this comment.
, the current configuration with just --add-opens might be sufficient if Error Prone only accesses these packages through reflection during test runs.
If the current test configuration is working properly on JDK 17 and 24, then the existing --add-opens entries are adequate for the test execution context. My previous suggestion may have been overly cautious for this specific use case.
🐰💭
🧠 Learnings used
Learnt from: msridhar
PR: uber/NullAway#1245
File: guava-recent-unit-tests/src/test/java/com/uber/nullaway/guava/NullAwayGuavaParametricNullnessTests.java:101-102
Timestamp: 2025-08-14T18:50:06.159Z
Learning: In NullAway JSpecify tests, when JDK version requirements exist due to bytecode annotation reading capabilities, prefer failing tests over skipping them on unsupported versions to ensure CI catches regressions and enforces proper JDK version usage for developers.
yuxincs
approved these changes
Sep 2, 2025
This was referenced Sep 2, 2025
Merged
Merged
msridhar
added a commit
that referenced
this pull request
Sep 24, 2025
We update to the latest WALA release, which supports running on JDK 25, so we can re-enable JarInfer tests. Note that running JarInfer now does not work on Temurin, since they exclude `.jmod` files from their distribution (see [the WALA docs](https://github.com/wala/WALA/wiki/Analysis-Scope)). So we switch our CI JDKs to be from the Zulu distribution. According to the CI job proposal from #1271, we should also be switching to building on JDK 25, with a separate test job for JDK 21. I will hold off on that for a bit (to check stability) and then do it in a separate PR, along with an upgrade to Gradle 9.1.0 which supports running on JDK 25. Fixes #1189 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - Added compatibility with JDK 25 by expanding the supported Java version matrix. - Tests - Updated toolchain and test tasks to target JDK 25. - Removed a previous workaround that disabled a specific JDK test task. - Chores - Upgraded dependencies: WALA to 1.6.12 and Mockito to 5.19.0. - Build/CI - CI and test generation now include JDK 25; CI JDK distribution switched to Zulu. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Dec 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the following new policy for jobs to run on CI:
testtask)This is in preparation for updating to Gradle 9 (#1270), which can only run on JDK 17+.
Summary by CodeRabbit
Chores
Tests