guava
guava copied to clipboard
google
Run tests under Java 17
We would want to detect anything that breaks with more recent versions than what we currently test with (Java 11).
To do so, I'm told that we'll have to omit the --no-module-directories option that we're likely adding in https://github.com/google/guava/pull/5800, just as we'll have to have found a way to omit it under Java 8.
I think that's fine, as I think we're using it only to work around a JDK11(?) bug that was fixed in JDK13(?): https://bugs.openjdk.java.net/browse/JDK-8215582
After that, I see issues because we pass the JDK sources to our Javadoc generation (so that @inheritDoc works for JDK classes). We've seen this before, though we just omitted files to work around it. (Maybe we'd want to rewrite the files, or maybe we can really pass -source 17 or whatever nowadays??) For purposes of filing this bug, I'm just skipping that by passing -Dmaven.javadoc.skip=true.
There's also a TreeMap bug that I've been meaning to report for a long time. (As of Java 15, that was the only test failure.)
My notes from Java 15 (internal bug 182929722) also describe some compilation errors, which I still see (as expected) under Java 17:
[ERROR] /usr/local/google/home/cpovirk/clients/guava-black/guava/guava-tests/benchmark/com/google/common/util/concurrent/FuturesGetCheckedBenchmark.java:[37,25] package java.security.acl does not existJust pick another exception type to test with.
[ERROR] /usr/local/google/home/cpovirk/clients/guava-black/guava/guava-tests/test/com/google/common/reflect/TypeTokenTest.java:[421,11] types java.lang.CharSequence and java.util.List<E> are incompatible; [ERROR] class java.lang.Object&java.util.List<java.lang.String>&java.lang.CharSequence inherits abstract and default for isEmpty() from types java.lang.CharSequence and java.util.List [ERROR] /usr/local/google/home/cpovirk/clients/guava-black/guava/guava-tests/test/com/google/common/reflect/TypeTokenTest.java:[435,11] types java.lang.CharSequence and java.util.List<E> are incompatible; [ERROR] class java.lang.Object&java.util.List<java.lang.String>&java.lang.CharSequence inherits abstract and default for isEmpty() from types java.lang.CharSequence and java.util.List...because both define
isEmpty(). Just pick another type.
we'll have to omit the --no-module-directories option
Do I understand correctly that this issue is only to run tests on 17, but not to migrate our release scripts to 17?
If we move all release scripts to 17, then we can just keep the present configuration (i.e., without 9–12 specific no-module-directories option). That will mean that Javadoc will be generated fine on 8 and 13+ (with working search on 13+), whilst on 9–12 it will just have silently broken search functionality (but, if we release Javadoc JARs to Maven Central and upload Javadocs to guava.dev with 17, that mustn't affect anyone).
We would want to detect anything that breaks with more recent versions
I've noticed that some projects run their tests with early-access OpenJDK builds (in non-blocking mode); and some report the failures to OpenJDK.
Do you think it'd be valuable for Guava?
Do I understand correctly that this issue is only to run tests on 17, but not to migrate our release scripts to 17?
That's my initial thinking. But you raise an interesting option that I don't think we'd considered.
We currently run the release script on our own machines and with whatever the Google default JDK is. That, particularly the second half, is not necessarily a great idea (see also internal bug 165936854), so it could be nice to hardcode a JDK that we change only deliberately.
The place to do that would be (Google-internal) devtools/javalibraries/release/release.sh. It would need to pull a Java 17 from somewhere (I can point you at a Java Platform Team copy) and point JAVA_HOME at that. (Or there's probably some better way that we should be doing all this.)
That would also mean fixing the tests -- for this project and for the other project that use that script, or else we could make the choice of JDK conditional on the project.
I think I've seen the early-access approach in Error Prone. It sounds like a good idea, too, though I want to say we're all pulling from a common Google quota, so there is some cost.
(I have been meaning to report the TreeMap thing forever :( One of these years....)
One more bit of work to support 17: fix Javadoc generation there 🙃
Turns out it won’t work on 17 just yet, because, apparently, javadoc tool switched a warning on using incompatible language features in source files to an error. As we pass source 8, but also take JDK sources from the JDK at JAVA_HOME (which naturally uses newer language features), it fails:
Error on 17
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.0:javadoc (default-cli) on project guava: An error has occurred in Javadoc report generation:
[ERROR] Exit code: 1 - /usr/local/google/home/dmitrytimofeev/third_party_projects/guava/guava/target/jdk-sources/java.base/java/lang/reflect/Proxy.java:710: warning: as of release 10, 'var' is a restricted type name and cannot be used for type declarations or as the element type of an array
[ERROR] var types = new HashSet<Class<?>>();
[ERROR] ^ ^
Warning on the same thing in the currently used 11
[WARNING] /usr/local/google/home/dmitrytimofeev/third_party_projects/guava/android/guava/target/jdk-sources/java.base/java/lang/reflect/Proxy.java:710: warning: as of release 10, 'var' is a restricted local variable type and cannot be used for type declarations or as the element type of an array
[WARNING] var types = new HashSet<Class<?>>();
[WARNING] ^
Oh, right, this is the same general problem as we're working around with https://github.com/google/guava/blob/a197d99b96d69ea75a2c4a15ecd8a730dd3270d6/guava/pom.xml#L110. There were already some problems that were errors, but now there are even more.
The comment there says that "we're using -source 8 to avoid other modules problems." I don't remember what the problems are, though. My guess would be that we need to keep doing that, at least under JDK11, though who knows if it would work under JDK17?
There must be some kind of conditional setup that would make it all work, but it's fine with me to fall back to ignoring JDK17.
(progress in https://github.com/google/guava/commit/7e04a00131a3a88888752e3548f3c5140b996c85)
Possible other trouble:
testNewHashMapWithExpectedSize_wontGrow(com.google.common.collect.MapsTest): Unable to make field transient java.util.HashMap$Node[] java.util.HashMap.table accessible: module java.base does not "opens java.util" to unnamed module @4297a8a1
testNewLinkedHashMapWithExpectedSize_wontGrow(com.google.common.collect.MapsTest): Unable to make field transient java.util.HashMap$Node[] java.util.HashMap.table accessible: module java.base does not "opens java.util" to unnamed module @4297a8a1
testNoProviders(com.google.common.hash.MacHashFunctionTest): class com.google.common.hash.MacHashFunctionTest (in unnamed module @0x4297a8a1) cannot access class sun.security.jca.Providers (in module java.base) because module java.base does not export sun.security.jca to unnamed module @0x4297a8a1
testInterruptIsSlow(com.google.common.util.concurrent.InterruptibleTaskTest): Unable to make protected final java.lang.Thread java.util.concurrent.locks.AbstractOwnableSynchronizer.getExclusiveOwnerThread() accessible: module java.base does not "opens java.util.concurrent.locks" to unnamed module @4297a8a1
testMockingEasyMock(com.google.common.util.concurrent.RateLimiterTest)
Some of that might be stuff we're willing to use --add-opens for (or whatever the flag is; I haven't gotten the hang of them all yet).
Another thing for us to look at (possibly not for JDK 17 but for a later version?) is https://openjdk.org/jeps/411. I saw failures when testing with jdk-20-ea+16. I was able to get around them by adding -Djava.security.manager=allow to...
https://github.com/google/guava/blob/b337be608971b02a51f1a31a50a49c4755c6fd75/pom.xml#L241
...but that broke JDK8 builds with an error whose message I couldn't see until I ran Maven with -X -e and then directly ran the shell script that it printed out:
Error occurred during initialization of VM
java.lang.InternalError: Could not create SecurityManager: allow
JDK17 appears to... pass all tests!? (It does still fail to build Javadoc.) I knew that @eamonnmcmanus had done a bunch of work, but I hadn't realized we were so close.
Probably what put us over the top was the EasyMock upgrade in https://github.com/google/guava/pull/6229, which pulled in an EasyMock fix for new JDKs.
The errors I see with JDK20 are almost all Security-Manager-related.
testUnloadableInStaticFieldIfClosed(com.google.common.base.FinalizableReferenceQueueClassLoaderUnloadingTest): The Security Manager is deprecated and will be removed in a future release
testUnloadableWithSecurityManager(com.google.common.base.FinalizableReferenceQueueClassLoaderUnloadingTest): The Security Manager is deprecated and will be removed in a future release
testExistsThrowsSecurityException(com.google.common.reflect.ClassPathTest): The Security Manager is deprecated and will be removed in a future release
testAbstractFutureInitializationWithInnocuousThread_doesNotThrow(com.google.common.util.concurrent.AbstractFutureInnocuousThreadTest): The Security Manager is deprecated and will be removed in a future release
testRenaming_noPermissions(com.google.common.util.concurrent.CallablesTest): The Security Manager is deprecated and will be removed in a future release
testOnSuccessThrowsRuntimeException(com.google.common.util.concurrent.FutureCallbackTest):
testOnSuccessThrowsError(com.google.common.util.concurrent.FutureCallbackTest):
It's interesting that JDK20 shows a problem in some tests that use Mockito. Though I know the JDK is locking down some internals that Mockito depends on to do certain things, I wouldn't have expected the problems to affect mocking of FutureCallback from within its package. But I haven't looked at the error closely enough to even say if Mockito is the cause, let alone to say specifically where the problem lies.
Somewhat related: Another thing we may look at is limiting our Error Prone usage to LTS JDKs, since some Error Prone checks use a repackaged Checker Framework, which doesn't support versions like JDK15 (as eamonnmcmanus recently reported).
It looks like JDK17 Javadoc can work fine with <source>17</source>. The trick is how to set that for JDK17 without also setting it for other JDK versions (especially older ones): Worst case, we could use profiles. But maybe we can do something with java.specification.version or another property.
(maven-javadoc-plugin says that it defaults to maven.compiler.source, which seems to mean "whatever <source> is set for maven-compiler-plugin." I had thought that it might mean that maven.compiler.source was a property that was read by both plugins, but it sounds like it's "written" by maven-compiler-plugin and read only by other plugins, like maven-javadoc-plugin.)
RE: Mockito: It looks specific to the InnocuousThread tests:
testOnSuccessThrowsRuntimeException(com.google.common.util.concurrent.FutureCallbackTest) Time elapsed: 0.005 sec <<< ERROR!
org.mockito.exceptions.base.MockitoException:
ClassCastException occurred while creating the mockito mock :
class to mock : 'com.google.common.util.concurrent.FutureCallback', loaded by classloader : 'jdk.internal.loader.ClassLoaders$AppClassLoader@5ffd2b27'
created class : 'org.mockito.codegen.FutureCallback$MockitoMock$WO17i1nt', loaded by classloader : 'com.google.common.util.concurrent.AbstractFutureInnocuousThreadTest$1@2b6c724a'
proxy instance class : 'org.mockito.codegen.FutureCallback$MockitoMock$WO17i1nt', loaded by classloader : 'com.google.common.util.concurrent.AbstractFutureInnocuousThreadTest$1@2b6c724a'
instance creation by : ObjenesisInstantiator
You might experience classloading issues, please ask the mockito mailing-list.
at com.google.common.util.concurrent.FutureCallbackTest.testOnSuccessThrowsRuntimeException(FutureCallbackTest.java:105)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at junit.framework.TestCase.runTest(TestCase.java:177)
at junit.framework.TestCase.runBare(TestCase.java:142)
at junit.framework.TestResult$1.protect(TestResult.java:122)
at junit.framework.TestResult.runProtected(TestResult.java:142)
at junit.framework.TestResult.run(TestResult.java:125)
at junit.framework.TestCase.run(TestCase.java:130)
at junit.framework.TestSuite.runTest(TestSuite.java:241)
at junit.framework.TestSuite.run(TestSuite.java:236)
at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:90)
at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:35)
at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:115)
at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:97)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
at java.base/java.lang.reflect.Method.invoke(Method.java:578)
at org.apache.maven.surefire.booter.ProviderFactory$ClassLoaderProxy.invoke(ProviderFactory.java:103)
at jdk.proxy1/jdk.proxy1.$Proxy0.invoke(Unknown Source)
at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:150)
at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcess(SurefireStarter.java:91)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:69)
Caused by: java.lang.ClassCastException: Cannot cast org.mockito.codegen.FutureCallback$MockitoMock$WO17i1nt to com.google.common.util.concurrent.FutureCallback
at java.base/java.lang.Class.cast(Class.java:3983)
at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.ensureMockIsAssignableToMockedType(SubclassByteBuddyMockMaker.java:95)
at org.mockito.internal.creation.bytebuddy.SubclassByteBuddyMockMaker.createMock(SubclassByteBuddyMockMaker.java:52)
at org.mockito.internal.creation.bytebuddy.ByteBuddyMockMaker.createMock(ByteBuddyMockMaker.java:42)
at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:99)
at org.mockito.internal.MockitoCore.mock(MockitoCore.java:88)
at org.mockito.Mockito.mock(Mockito.java:1998)
at org.mockito.Mockito.mock(Mockito.java:1913)
... 22 more
The simplest thing is probably just to not use Mockito there, since we don't really need it. That will also make our tests runnable under GWT, which is nice. I'll take a stab at it.