move wildcard-related tests to a separate test class#1361
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1361 +/- ##
=========================================
Coverage 88.47% 88.47%
Complexity 2602 2602
=========================================
Files 98 98
Lines 8727 8727
Branches 1736 1736
=========================================
Hits 7721 7721
Misses 503 503
Partials 503 503 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughRemoved three public test methods from GenericMethodTests.java (two occurrences of nullableWildcardFromCaffeine and genericMethodLambdaArgWildCard). Added a new test class WildcardTests.java (extends NullAwayTestsBase) containing three test methods: simpleWildcard, genericMethodLambdaArgWildCard, and nullableWildcardFromCaffeine; two of the new tests are annotated with Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java(0 hunks)nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java(1 hunks)
💤 Files with no reviewable changes (1)
- nullaway/src/test/java/com/uber/nullaway/jspecify/GenericMethodTests.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 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.
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
🧬 Code graph analysis (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java (2)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(9-51)nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java (1)
JSpecifyJavacConfig(16-59)
⏰ 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 caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on ubuntu-latest
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java (2)
1-10: Class setup and imports look consistent with existing JSpecify testsPackage, imports, and extending
NullAwayTestsBaseare appropriate and align with the existing testing patterns in this module.
58-105: Test structure, helpers, and gradle invocation are all correctThe
nullableWildcardFromCaffeine()test is well-structured and properly usesmakeHelperWithInferenceFailureWarning()to verify substitution behavior with generic wildcards. The helper methods correctly configure JSpecify mode withJSpecifyJavacConfig.withJSpecifyModeArgs()and the necessary compiler flags (OnlyNullMarked=trueandWarnOnGenericInferenceFailure=true).To verify this test in isolation, run:
./gradlew :nullaway:test --tests "com.uber.nullaway.jspecify.WildcardTests"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java (1)
12-37: simpleWildcard test covers both positive/negative paths; refine diagnostic expectation laterThe structure and use of
nullableWildcardvsnonnullWildcardwithFoo<@Nullable String>look good and should exercise the intended JSpecify wildcard semantics once #1360 is fixed. The// BUG: Diagnostic contains: something about how f cannot be passed heremarker is clearly a placeholder; when you un-ignore the test, it would be good to tighten that substring to reflect the actual NullAway message so the assertion is meaningful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Learnt from: msridhar
Repo: uber/NullAway PR: 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.
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
📚 Learning: 2025-08-14T18:50:06.159Z
Learnt from: msridhar
Repo: uber/NullAway PR: 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:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-08-28T04:54:20.953Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1248
File: nullaway/src/main/java/com/uber/nullaway/generics/GenericsChecks.java:847-857
Timestamp: 2025-08-28T04:54:20.953Z
Learning: In NullAway's GenericsChecks.java, NewClassTree support for explicit type argument substitution requires more extensive changes beyond just modifying the conditional in compareGenericTypeParameterNullabilityForCall. The maintainers prefer to handle NewClassTree support in a separate follow-up rather than expanding the scope of PRs focused on specific issues like super constructor calls.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Use the `--tests` flag with gradle to run specific test classes or methods, e.g., `./gradlew :nullaway:test --tests "com.uber.nullaway.NullAwayTest"`
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-11-25T22:43:06.446Z
Learnt from: CR
Repo: uber/NullAway PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T22:43:06.446Z
Learning: Run only the tests for the main NullAway module using `./gradlew :nullaway:test` unless specifically asked to run tests in a different module
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
📚 Learning: 2025-10-29T23:56:18.236Z
Learnt from: msridhar
Repo: uber/NullAway PR: 1316
File: jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/NullnessAnnotationSerializer.java:261-293
Timestamp: 2025-10-29T23:56:18.236Z
Learning: In NullAway's jdk-javac-plugin NullnessAnnotationSerializer, type variable bounds with annotations (e.g., `T extends Nullable Object`) are checked at their declaration sites by the typeParamHasAnnotation method for both class-level and method-level type parameters. The hasJSpecifyAnnotationDeep method is designed to check type uses (return types, parameters, etc.) and does not need a TYPEVAR case because type variable declaration bounds are already handled separately.
Applied to files:
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java
🧬 Code graph analysis (1)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java (2)
nullaway/src/test/java/com/uber/nullaway/NullAwayTestsBase.java (1)
NullAwayTestsBase(9-51)nullaway/src/main/java/com/uber/nullaway/generics/JSpecifyJavacConfig.java (1)
JSpecifyJavacConfig(16-59)
⏰ 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 caffeine with snapshot
- GitHub Check: Build and test on windows-latest
- GitHub Check: Build and test on macos-latest
🔇 Additional comments (2)
nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java (2)
39-59: Lambda wildcard inference regression test looks well-scopedThis is a clear, minimal reproduction of the wildcard/lambda inference behavior you want (no diagnostics, correct inference to
Function<Object, @Nullable Object>). UsingmakeHelperWithInferenceFailureWarning()is appropriate so any inference failure becomes visible once #1350 is addressed and the@Ignoreis removed. No changes needed.
61-94: Caffeine-derived nullable wildcard test is a solid guard for subtle substitution bugsThe Caffeine-style setup correctly stresses
<K1 extends K, V1 extends @Nullable V>with both explicit and inferred type arguments onbuild(...), underOnlyNullMarked+WarnOnGenericInferenceFailure. This should reliably catch regressions in how NullAway/JSpecify handle annotated wildcards andTypeidentity without relying on external testdata files. Looks good as-is.
| private CompilationTestHelper makeHelper() { | ||
| return makeTestHelperWithArgs( | ||
| JSpecifyJavacConfig.withJSpecifyModeArgs( | ||
| Arrays.asList("-XepOpt:NullAway:OnlyNullMarked=true"))); | ||
| } | ||
|
|
||
| private CompilationTestHelper makeHelperWithInferenceFailureWarning() { | ||
| return makeTestHelperWithArgs( | ||
| JSpecifyJavacConfig.withJSpecifyModeArgs( | ||
| Arrays.asList( | ||
| "-XepOpt:NullAway:OnlyNullMarked=true", | ||
| "-XepOpt:NullAway:WarnOnGenericInferenceFailure=true"))); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Helper methods correctly compose JSpecify and NullAway flags
Both helpers cleanly wrap JSpecifyJavacConfig.withJSpecifyModeArgs(...) with the needed NullAway options (OnlyNullMarked, optionally WarnOnGenericInferenceFailure), ensuring JSpecify mode and -XDaddTypeAnnotationsToSymbol=true are always set. If you find yourself adding more such helpers later, you might consider a small shared utility to reduce the duplicated Arrays.asList(...) literals, but it’s not necessary for this PR.
🤖 Prompt for AI Agents
In nullaway/src/test/java/com/uber/nullaway/jspecify/WildcardTests.java around
lines 96-108, the two helper methods duplicate the Arrays.asList(...) literals
for the NullAway/JSpecify args; refactor by extracting a small shared utility
(e.g., a private method or constant) that returns the base List<String> of
common flags (including the JSpecify mode and
"-XepOpt:NullAway:OnlyNullMarked=true"), then have makeHelper() call with that
base list and makeHelperWithInferenceFailureWarning() call the base list plus
the additional "-XepOpt:NullAway:WarnOnGenericInferenceFailure=true" element
(use new ArrayList<>(base) to append safely), and keep passing the resulting
list into JSpecifyJavacConfig.withJSpecifyModeArgs(...) so behavior is
unchanged.
The
GenericMethodTestsclass is getting huge so this helps a little. Also, add a new simple wildcard test to aid when we start working on #1360.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.