Skip to content
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.

Sign up for GitHub

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

Run recent JDK tests on JDK 21 #834

Merged
merged 7 commits into from
Sep 28, 2023
Merged

Run recent JDK tests on JDK 21 #834

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cacadbe
Prepare for JDK 21
armughan11 Sep 28, 2023
4ecb46e
Fix typo
armughan11 Sep 28, 2023
3f64acf
Fixed code coverage report
armughan11 Sep 28, 2023
2ee89d6
Update NullAwaySwitchTests.java
armughan11 Sep 28, 2023
a9ca068
shift comment
msridhar Sep 28, 2023
4003737
tweaks to test
msridhar Sep 28, 2023
f65a1c0
disable testJdk21 task
msridhar Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions code-coverage-report/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ plugins {
id 'jacoco'
}

// Use JDK 17 for this module, via a toolchain. We need JDK 17 since this module
// depends on jdk17-unit-tests.
// Use JDK 21 for this module, via a toolchain. We need JDK 21 since this module
// depends on jdk-recent-unit-tests.
// We must null out sourceCompatibility and targetCompatibility to use toolchains.
java.sourceCompatibility = null
java.targetCompatibility = null
java.toolchain.languageVersion.set JavaLanguageVersion.of(17)
java.toolchain.languageVersion.set JavaLanguageVersion.of(21)

// A resolvable configuration to collect source code
def sourcesPath = configurations.create("sourcesPath") {
Expand Down Expand Up @@ -79,5 +79,5 @@ dependencies {
implementation project(':jar-infer:jar-infer-lib')
implementation project(':jar-infer:nullaway-integration-test')
implementation project(':guava-recent-unit-tests')
implementation project(':jdk17-unit-tests')
implementation project(':jdk-recent-unit-tests')
}
4 changes: 2 additions & 2 deletions jdk17-unit-tests/build.gradle → jdk-recent-unit-tests/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ plugins {
id 'nullaway.java-test-conventions'
}

// Use JDK 17 for this module, via a toolchain
// Use JDK 21 for this module, via a toolchain
// We must null out sourceCompatibility and targetCompatibility to use toolchains.
java.sourceCompatibility = null
java.targetCompatibility = null
java.toolchain.languageVersion.set JavaLanguageVersion.of(17)
java.toolchain.languageVersion.set JavaLanguageVersion.of(21)

configurations {
// We use this configuration to expose a module path that can be
Expand Down
0 ...jdk17/NullAwayInstanceOfBindingTests.java → ...jdk17/NullAwayInstanceOfBindingTests.java
View file
Open in desktop
File renamed without changes.
0 ...llaway/jdk17/NullAwayModuleInfoTests.java → ...llaway/jdk17/NullAwayModuleInfoTests.java
View file
Open in desktop
File renamed without changes.
0 ...way/jdk17/NullAwayOptionalEmptyTests.java → ...way/jdk17/NullAwayOptionalEmptyTests.java
View file
Open in desktop
File renamed without changes.
0 ...r/nullaway/jdk17/NullAwayRecordTests.java → ...r/nullaway/jdk17/NullAwayRecordTests.java
View file
Open in desktop
File renamed without changes.
27 changes: 27 additions & 0 deletions ...r/nullaway/jdk17/NullAwaySwitchTests.java → ...r/nullaway/jdk17/NullAwaySwitchTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.uber.nullaway.NullAway;
import java.util.Arrays;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
Expand Down Expand Up @@ -174,4 +175,30 @@ public void testSwitchExprLambda() {
"}")
.doTest();
}

@Ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment indicating we are ignoring because of a crash in the dataflow library

@Test
public void testSwitchExprNullCase() {
defaultCompilationHelper
.addSourceLines(
"SwitchExpr.java",
"package com.uber;",
"import javax.annotation.Nullable;",
"class SwitchExpr {",
" public enum NullableEnum {",
" a,",
" b,",
" }",
" // NOTE: we should report a bug here for nullableEnum but cannot do so until",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually thinking more this is not true; if there is case null then there is no NPE. Can you confirm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case null is crashing the code right now, so I can't really check. But without case null it should report a NPE, which it doesn't. I could probably have two switch statements one with null and one without and then get this documented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying that NullAway should not report an NPE here, because there will be no NPE if case null is present. See here. You only get an NPE if there is no case null case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, I misunderstood your comment. I added the comment for NullAway not reporting without null, as we discussed previously, but in this context i can either omit it entirely or mention that it only applies when null case isn't defined.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I misunderstood before. I went ahead and deleted the comment.

" // Error Prone supports matching switch expressions",
" static Object handleNullableEnum(@Nullable NullableEnum nullableEnum) {",
" return switch (nullableEnum) {",
" case a -> new Object();",
" case b -> new Object();",
" case null -> throw new IllegalArgumentException(\"NullableEnum parameter is required\");",
" };",
" }",
"}")
.doTest();
}
}
2 changes: 1 addition & 1 deletion settings.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ include ':jar-infer:test-java-lib-jarinfer'
include ':jar-infer:nullaway-integration-test'
include ':jmh'
include ':guava-recent-unit-tests'
include ':jdk17-unit-tests'
include ':jdk-recent-unit-tests'
include ':code-coverage-report'
include ':sample-app'
include ':jar-infer:test-android-lib-jarinfer'