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.

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

Cannot create mock due to module java.base does not "opens java.lang.invoke" #1406

Closed
akefirad opened this issue Dec 27, 2021 · 29 comments · Fixed by #1407
Closed

Cannot create mock due to module java.base does not "opens java.lang.invoke" #1406

akefirad opened this issue Dec 27, 2021 · 29 comments · Fixed by #1407
Labels

Comments

@akefirad
Copy link

Describe the bug

org.spockframework.mock.runtime.DefaultMethodInvoker is accessing java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP which fails with:

org.spockframework.mock.CannotCreateMockException: Cannot create mock for class jdk.proxy2.$Proxy25Failed to invoke default method 'someDefaultMethod'

	at org.spockframework.mock.runtime.DefaultMethodInvoker.respond(DefaultMethodInvoker.java:64)
	at org.spockframework.mock.runtime.MockInvocation.callRealMethod(MockInvocation.java:59)
	at org.spockframework.mock.CallRealMethodResponse.respond(CallRealMethodResponse.java:30)
	at org.spockframework.mock.runtime.MockController.handle(MockController.java:50)
	at org.spockframework.mock.runtime.JavaMockInterceptor.intercept(JavaMockInterceptor.java:84)
	at org.spockframework.mock.runtime.DynamicProxyMockInterceptorAdapter.invoke(DynamicProxyMockInterceptorAdapter.java:34)
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field static final java.lang.invoke.MethodHandles$Lookup java.lang.invoke.MethodHandles$Lookup.IMPL_LOOKUP accessible: module java.base does not "opens java.lang.invoke" to unnamed module @3d99d22e
	at org.spockframework.mock.runtime.DefaultMethodInvoker.respond(DefaultMethodInvoker.java:50)
	... 7 more

To Reproduce

This is a minimal reproducible example minimal-reproducible-example.zip

Expected behavior

The code should not fail.

Actual behavior

The code fails with the given exception.

Java version

openjdk version "17.0.1" 2021-10-19 LTS
OpenJDK Runtime Environment Corretto-17.0.1.12.1 (build 17.0.1+12-LTS)
OpenJDK 64-Bit Server VM Corretto-17.0.1.12.1 (build 17.0.1+12-LTS, mixed mode, sharing)
(and probably Java 16)

Buildtool version

Apache Maven 3.8.1 (05c21c65bdfed0f71a2f2ada8b84da59348c4c5d)
Maven home: /Users/user/.sdkman/candidates/maven/current
Java version: 17.0.1, vendor: Amazon.com Inc., runtime: /Users/user/.sdkman/candidates/java/17.0.1.12.1-amzn
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "12.0.1", arch: "x86_64", family: "mac"

What operating system are you using

Mac

Dependencies

com.example:demo:jar:0.0.1-SNAPSHOT
+- org.springframework.boot:spring-boot-starter:jar:2.6.2:compile
|  +- org.springframework.boot:spring-boot:jar:2.6.2:compile
|  |  \- org.springframework:spring-context:jar:5.3.14:compile
|  |     +- org.springframework:spring-aop:jar:5.3.14:compile
|  |     +- org.springframework:spring-beans:jar:5.3.14:compile
|  |     \- org.springframework:spring-expression:jar:5.3.14:compile
|  +- org.springframework.boot:spring-boot-autoconfigure:jar:2.6.2:compile
|  +- org.springframework.boot:spring-boot-starter-logging:jar:2.6.2:compile
|  |  +- ch.qos.logback:logback-classic:jar:1.2.9:compile
|  |  |  \- ch.qos.logback:logback-core:jar:1.2.9:compile
|  |  +- org.apache.logging.log4j:log4j-to-slf4j:jar:2.17.0:compile
|  |  |  \- org.apache.logging.log4j:log4j-api:jar:2.17.0:compile
|  |  \- org.slf4j:jul-to-slf4j:jar:1.7.32:compile
|  +- jakarta.annotation:jakarta.annotation-api:jar:1.3.5:compile
|  +- org.springframework:spring-core:jar:5.3.14:compile
|  |  \- org.springframework:spring-jcl:jar:5.3.14:compile
|  \- org.yaml:snakeyaml:jar:1.29:compile
+- org.springframework.boot:spring-boot-starter-test:jar:2.6.2:test
|  +- org.springframework.boot:spring-boot-test:jar:2.6.2:test
|  +- org.springframework.boot:spring-boot-test-autoconfigure:jar:2.6.2:test
|  +- com.jayway.jsonpath:json-path:jar:2.6.0:test
|  |  +- net.minidev:json-smart:jar:2.4.7:test
|  |  |  \- net.minidev:accessors-smart:jar:2.4.7:test
|  |  |     \- org.ow2.asm:asm:jar:9.1:test
|  |  \- org.slf4j:slf4j-api:jar:1.7.32:compile
|  +- jakarta.xml.bind:jakarta.xml.bind-api:jar:2.3.3:test
|  |  \- jakarta.activation:jakarta.activation-api:jar:1.2.2:test
|  +- org.assertj:assertj-core:jar:3.21.0:test
|  +- org.hamcrest:hamcrest:jar:2.2:test
|  +- org.junit.jupiter:junit-jupiter:jar:5.8.2:test
|  |  +- org.junit.jupiter:junit-jupiter-api:jar:5.8.2:test
|  |  |  +- org.opentest4j:opentest4j:jar:1.2.0:test
|  |  |  +- org.junit.platform:junit-platform-commons:jar:1.8.2:test
|  |  |  \- org.apiguardian:apiguardian-api:jar:1.1.2:test
|  |  \- org.junit.jupiter:junit-jupiter-params:jar:5.8.2:test
|  +- org.mockito:mockito-core:jar:4.0.0:test
|  |  +- net.bytebuddy:byte-buddy:jar:1.11.22:test
|  |  +- net.bytebuddy:byte-buddy-agent:jar:1.11.22:test
|  |  \- org.objenesis:objenesis:jar:3.2:test
|  +- org.mockito:mockito-junit-jupiter:jar:4.0.0:test
|  +- org.skyscreamer:jsonassert:jar:1.5.0:test
|  |  \- com.vaadin.external.google:android-json:jar:0.0.20131108.vaadin1:test
|  +- org.springframework:spring-test:jar:5.3.14:test
|  \- org.xmlunit:xmlunit-core:jar:2.8.4:test
+- org.spockframework:spock-spring:jar:2.1-M2-groovy-3.0:test
|  +- org.spockframework:spock-core:jar:2.1-M2-groovy-3.0:test
|  |  \- org.junit.platform:junit-platform-engine:jar:1.8.2:test
|  \- org.codehaus.groovy:groovy:jar:3.0.9:test
+- org.codehaus.groovy:groovy-json:jar:3.0.9:test
\- org.codehaus.groovy:groovy-xml:jar:3.0.9:test

Additional context

Spock users can solve the issue by adding the following to the pom.xml:

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-surefire-plugin</artifactId>
    <configuration>
        <argLine>
            --add-opens=java.base/java.lang.invoke=ALL-UNNAMED
        </argLine>
    </configuration>
</plugin>

But I guess a permanent fix would be nice in the library.

@akefirad akefirad added the bug label Dec 27, 2021
@kriegaex
Copy link
Contributor

kriegaex commented Dec 27, 2021

You are trying to spy on an interface. You can only spy on an instance of a concrete class, but you did not instantiate any Foo object. Simply change def foo = Spy(Foo) to def foo = Stub(Foo) or def foo = Mock(Foo), then your test passes.

Alternatively, add a class like

package com.example.demo;

public class FooImpl implements Foo {
  @Override
  public String foo() {
    return "FOO";
  }
}

and use Foo foo = Spy(FooImpl) or so in your test.

@akefirad
Copy link
Author

That is right. Will do that. But does that mean the lib doesn’t need any fix? I mean the exception will be thrown if the specific method of Sock is called regardless of the use case.

@kriegaex
Copy link
Contributor

I am leaving it up to a committer to answer the last question.

@kriegaex
Copy link
Contributor

kriegaex commented Dec 27, 2021

Just one more bit of background information for your understanding, if you are interested in such details:

  • When creating an interface mock, Spock uses JDK proxies, as you can see class name jdk.proxy2.$Proxy25 from the error message.
  • JDK proxies are always final, hence you cannot subclass them, which would be necessary to mock or spy them.
  • JDK 16 introduced JEP 396 (Strongly Encapsulate JDK Internals by Default), therefore some internal APIs cannot be used without special Java CLI options anymore.

It looks as if in this case Spock is first creating an interface mock and then trying to spy on it. This feels wrong to me, if it proves to be true. What Spock could do instead is to either throw a comprehensive error message or force the creation of a Byte Buddy (or CGLIB) proxy in this case in order to avoid the error message. But actually, I think the use case is simply invalid and should have been forbidden all along.

@leonard84
Copy link
Member

You can Spy on interfaces, this is only useful if it has default methods that you don't want to mock, but use as-is.

As for the other matter, I'm not aware of a way to fix this in a library. If you are using the module system, then you'll have to open the base module accordingly.

@akefirad
Copy link
Author

Hm, I'm not expert in the topic, but I had a feeling that it might be possible to open the module in the library. (Feel free to close the ticket if nothing can be done here. Thanks for the great work BTW.)

@leonard84
Copy link
Member

AFAIK, you can only open your own module to other modules, not open external modules up to your module.

The only thing I find slightly surprising, is that is in an unnamed module instead of org.spockframework.core (Automatic module name), but I have only worked with the module system tangentially.

@kriegaex
Copy link
Contributor

You can Spy on interfaces, this is only useful if it has default methods that you don't want to mock, but use as-is.

Yes, that is a valid use case which I forgot. Thanks for the explanation. In this case, with the current interface mock implementation, the problem cannot be solved, neither by using Spy(MyInterface), nor by using a mock with a CallRealMethodResponse default response nor by stubbing with callRealMethod(), because they all end up doing the same thing.

Would it make sense to ask @raphw how to do this in Byte Buddy or to check how Mockito does it? For example, this works in Spock under JDK 17:

def "use Mockito spy"() {
  given:
  def foo = Mockito.spy(Foo)
  Mockito.doReturn("bar").when(foo).foo()
  
  expect:
  foo.foo() == "bar"
  foo.bar() == "bar"
}

Mockito uses BB, we use it too. So there should be a way to solve this, which does not involve using the trusted method handle thingy via (now forbidden) reflective access to private JDK internals. If there would be an equivalent way to solve this with CGLIB, is another open question though.

@raphw
Copy link
Contributor

raphw commented Dec 28, 2021

Java 17 offers a new method for default invocation.

This method can theoretically also be used from Byte Buddy (or cglib) proxies. Byte Buddy does however gracefully handle this when using a @SuperCall proxy or similar as long as the default method is not ambiguous. I'd suggest to only use the reflection hack if the above official approach is not available as a fallback.

@leonard84
Copy link
Member

Thanks for the information @raphw I added the code in leonard84@4c15c24

@kriegaex
Copy link
Contributor

kriegaex commented Dec 29, 2021

I re-tested a locally built 2.1-groovy-3.0-SNAPSHOT from #1407 with the example from the minimal reproducible example presented here and can confirm that it fixes the problem on JDK 17. I think you are going to add one or multiple tests to the PR later anyway, but might like some feedback.

@leonard84
Copy link
Member

@kriegaex I actually wasn't planning on adding additional tests, as we already have some for default method handling, so it is confirmed not to break on Java 17 by the CI jobs.

The only way I came up with testing if the new invokeDefault is called would be checking the output for the warning message, but this seems unreliable, as that only happens the very first time an illegal access happens. If you have a suggestion for how to test this properly, please tell

@kriegaex
Copy link
Contributor

kriegaex commented Dec 29, 2021

I actually wasn't planning on adding additional tests, as we already have some for default method handling, so it is confirmed not to break on Java 17 by the CI jobs.

Does that mean that currently the tests are failing? Or are they being skipped on Java 16+? If there are no failing tests, then I would argue that there are not enough tests.

If you have a suggestion for how to test this properly, please tell.

Well, I would simply add the test from this MCVE here (or something very similar) as a regression test. It reproducibly fails on current master on JDK 16+ and passes on the PR branch. I think, it is important to document improvements by regression tests.

@leonard84
Copy link
Member

We don't use the module-path, only classpath and there you only get a warning message about reflective access. Using module-path with gradle wasn't really supported last I checked (gradle 6.x), but it might be possible now with gradle 7.x, would you like to investigate @kriegaex?

@kriegaex
Copy link
Contributor

kriegaex commented Jan 3, 2022

Actually, I do not understand your comment, @leonard84. The example project in the ZIP file is a Maven project and easily reproduces the problem. Nothing there explicitly uses the module path.

@leonard84
Copy link
Member

I am talking about the Spock Gradle build, not the MCVE. Surefire uses the module-path by default sometimes even when it shouldn't (see https://issues.apache.org/jira/browse/SUREFIRE-1831).

@kriegaex
Copy link
Contributor

kriegaex commented Jan 3, 2022

Is it not normal that the JRE classes are on the module path? Is that any different when running tests from Gradle? Sorry if this seems to be a stupid question. I am also wondering why, when running on JDK 16+, in a Spock test running from Gradle, suddenly an inaccessible class due to JEP 396 should be accessible by default - unless Spock's Gradle build somehow adds --add-opens to the Java command line by default, which would rather surprise me.

Even if there were differences when running the exact same test from Gradle vs. Maven, would the difficulty of replicating a problem mean that there should be no regression test? How about adding some Maven (integration) regression tests? I think we are not considering to only care about Gradle users, are we?

Sorry, maybe I should try running that test from within the Spock build, which I did not do before. I simply ran the Maven reproducer against 2.1-M2 and the snapshot containing the fix, because from a user perspective I care about whether my build works correctly, not how the same thing is adapted to the Spock build itself.

@leonard84
Copy link
Member

A bit of background information https://sormuras.github.io/blog/2018-09-11-testing-in-the-modular-world.html

If you run your code with --classpath instead of --module-path, then java will use the classpath behavior, i.e., no module based encapsulation. I would assume that JEP 396 uses changes in the module descriptors to prevent access to internals, which doesn't affect classpath usage at all.

Take it with a grain of salt, as I haven't really worked with the module-path yet, as it seems good suited for the JDK, but not so great for application development just more complexity and problems from my limited experience.

It is not really Gradle vs. Maven, but classpath vs. module-path users. And I don't really want to add Maven to the Spock build as this will be quite complex.

If you want to contribute a test-subproject for modules that runs on Java versions >= 9 then that would be helpful. It seems that there is some Gradle documentation for that now.

kriegaex added a commit to kriegaex/spock that referenced this issue Jan 4, 2022
The smoke test lives in JavaSpies and reproduces the reported issue on
Java 16+ for Spock 2.1-M2. It should however pass with the bugfix in
this branch.
@kriegaex
Copy link
Contributor

kriegaex commented Jan 4, 2022

OK, I simply added a regression test and created a PR for your bugfix branch, which you can just merge. First I thought that the test would not reproduce the error, but I accidentally ran it on JDK 8. Hence my previous comment, which I have just deleted because it was irrelevant.

On JDK 16+, even with Gradle the issue is easily reproducible. Just run the test on master vs. your bugfix branch. We do not need any new modules, just use the test from this issue. I only renamed some classes, methods and fields in order to more clearly commmunicate the intent. There were too many foo's and bar's in the original here.


Update: Argh, no! It is not reproducible and I should not have deleted my previous comment. Sorry for the chaos, but I accidentally ran the test from IntelliJ IDEA, not delegating to Gradle. As soon as I do delegate, the test passes on the main branch, i.e. does not reproduce the problem, even though it should. I am stunned that Gradle obviously defaults to simply ignoring Java modules completely (even for the JDK itself) when running tests. That running the same test from IDEA or Surefire reproduces the problem, indicates that it is a real issue, though.

You work at Gradle, don't you? Can you please consult with a colleague who can explain to you first hand how to run a test with the JRE classes on the module classpath, as should be the default because every normal JRE runs that way?

@kriegaex
Copy link
Contributor

kriegaex commented Jan 4, 2022

Here is my previously deleted comment. I copied it over from Slack in order to revive it:

Surefire uses the module-path by default sometimes even when it shouldn't (see SUREFIRE-1831).

This was closed as a duplicate of SUREFIRE-1809, which in turn was closed as fixed. I built a Surefire 3.0.0-M6 snapshot and ran the Maven project against it. As expected, that change has no influence on the outcome of the test. Still, it fails with Spock 2.1-M2 and passes on the feature branch with the Spock fix.

So this is not a Surefire problem, rather a problem with Gradle putting the JRE classes on the classpath, hiding a problem which would always occur in real-world scenarios where the Java 9+ JRE is on the module path, as is to be expected. I do not speak Gradle and cannot help you there, creating a new module for Spock tests.

P.S.: I do not like JMS either. The only thing I ever do as a user or OSS contributor is to somehow manage to keep everything running or make it run again. Maybe the two of us are just not interested in all this enhanced security and modularity stuff in the JRE and should completely change our way of thinking, but I simply do not see much value in JMS, because as a (hobby) programmer I mostly deal with either cross-cutting concerns (AspectJ) or test automation (Spock). In both cases, JMS more often gets in the way than it actually is helpful. Having said that, JMS is real since Java 9, so I guess we have to account for cases in which it introduces Spock testing problems.

@kriegaex
Copy link
Contributor

kriegaex commented Jan 4, 2022

So after we found out that my PR was redundant, the open question is: How do we get PartialMockingInterfacesWithDefaultMethods to fail on the main branch in Gradle?

@kriegaex
Copy link
Contributor

kriegaex commented Jan 14, 2022

@leonard84, I think I know why PartialMockingInterfacesWithDefaultMethods passes in Gradle, but fails in IDEA with JDK 16+. I found the reason when running the test in Gradle with debug output, searching the log for "java.exe" on my local Windows system, because I wanted to know how exactly Gradle starts the JVM. I saw the path and realised that the tests are being run in a JRE 8. Uh-oh!

Renaming my local JDK and JRE 8 directories to something Gradle could not find, then running the build again, it failed, being unable to find any JRE/JDK 8. I realised: Spock uses a Gradle toolchain configuration in order to ensure compilation level Java 8 in order to avoid a failing build for

public static final Object _ = Wildcard.INSTANCE;
in the Specification class. On Java 9+ it would be enough to use the --release 8 compiler switch instead for _ to be accepted as an identifier. Even a simple source and target of 8 suffices, if it is not important to actually build against the JDK 8 API. Then you would not require toolchains at all and e.g. a local developer build on Java 11 or 17 would no longer require also a JDK 8 to be installed when building Spock.

Either way, even if you do use toolchains, you need to configure them correctly, i.e. please ensure that they are only used for compilation, not for running tests.

Simply add this to PartialMockingInterfacesWithDefaultMethods:

  def setupSpec() {
    println "### Java version: " + System.properties["java.version"]
  }

Then build e.g. with Java 17 using Gradle, e.g. via :spock-specs:test --tests "org.spockframework.smoke.mock.PartialMockingInterfacesWithDefaultMethods". Now look at the console and search for the test output. You are going to see something like:

### Java version: 1.8.0_281

This IMO is a major bug in the Gradle build. I have no idea how to fix it, being a Maven guy.

So the problem for tests passing on JDK 16+ which should actually fail is not that Gradle somehow magically deactivates the JMS on Java 9+ for running tests. It is much simpler: The Spock build runs those tests in a JRE 8, being completely blind for any kind of JMS problems on more recent JDKs. I experimentally switched to a Java 17 toolchain, renaming the _ field to something else temporarily. As expected, now the test fails just like in IDEA or in Maven.

@leonard84
Copy link
Member

leonard84 commented Jan 14, 2022

If you want to run tests with a different Java version, then you need to use the -DjavaVersion=11 parameter. See the github actions for an example.

./gradlew :spSpecs:test --tests PartialMockingInterfacesWithDefaultMethods -DjavaVersion=11

> Configure project :spock-core
[versioning] WARNING - the working copy has unstaged or uncommitted changes.

> Task :spock-specs:test

PartialMockingInterfacesWithDefaultMethods STANDARD_OUT
    ### Java version: 11.0.9
WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.codehaus.groovy.reflection.CachedClass (file:/Users/leonard/.gradle/caches/modules-2/files-2.1/org.codehaus.groovy/groovy/2.5.15/85c14d7031ae2bfd1d0bbed513ebddcc3a530633/groovy-2.5.15.jar) to method java.lang.reflect.Proxy.newProxyInstance(java.lang.Class,java.lang.reflect.Constructor,java.lang.reflect.InvocationHandler)

But the actual reason are these lines added in b0e5f62 😅

spock/build.gradle

Lines 175 to 178 in c2bcc86

if (javaVersionNoShadow >= 17) {
// Workaround for Java 17 to enable mocking of default methods
jvmArgs("--add-opens", "java.base/java.lang.invoke=ALL-UNNAMED")
}

Which will now be removed again by the PR.

@kriegaex
Copy link
Contributor

kriegaex commented Jan 15, 2022

Sorry if this sounds judgemental, but while I think that it is nice to have an option to easily run tests with another JVM using toolchains instead of simply reconfiguring IDE or CLI to start another local Java version, I am kind of shocked at this utterly counter-intuitive build configuration. All other projects I ever worked on before used the same JVM for building and testing by default. Anyone who wanted to enforce usage of a different JVM had to reconfigure something, either in the local build environment or in the build settings.

Please consider to change this. Keeping it like this would mean that in both CI builds and local developer builds you would not need to set extra parameters, unless you want to build for a different target or run tests for same target JRE. You cannot expect everyone who builds Spock in order to re-test a bug or bisect a problem to study the CI build configuration and derive the right way to run her local build from that. Imagine someone wants to build the product on JDK 9-17 and the build mysteriously fails because no JRE 8 is installed, just because the user does not know she needs an extra CLI build option. Or like in my case - even worse - she never realises that the tests run on JRE 8 while she thinks she builds and tests on JDK x. Also, a developer building Spock has to remember to set the CLI parameter each time she runs the build from CLI. When a Gradle test run is saved as a run configuration in an IDE like IntelliJ IDEA, she has to remember to update it in parallel with changing the JDK settings for the project, if by default she wants to always run the tests on the same JDK as the build. What is worse, IDEA is always going to run tests with the specified build JDK when using the IDE build/run capabilities, because it does not recognise the complicated magic in the Spock Gradle build configuration, i.e. the IDE will behave differently from the command line.

Of course, in some of my Maven projects I also have JDK-dependent build options like --add-opens, if they are necessary. But those detect the runtime environment automatically and do not depend on users telling them which version is to be used.

BTW, even though it is now obsolete, if (javaVersionNoShadow >= 17) should have been ... >= 16). I can easily make the test fail on JDK 16 on the pre-merge master I am still working on.

One more thing: When the issue was opened and saying that --add-opens=java.base/java.lang.invoke=ALL-UNNAMED was needed to build the example Maven project successfully, why did you not mention that this is a known fact and using the then master or milestone required that option to run successfully in order to use some Spock features? I see that since 2021-10-05 that option was part of the build configuration. Instead you argued that the error is not reproducible because of some supposed difference in how Gradle sets up the JVM with regard to Java modules, while actually it was a known problem which Spock worked around in the exact same way as the author of this issue. I think we could have saved a lot of time that way and focused finding and implementing the fix suggested by Rafael.

@kriegaex
Copy link
Contributor

kriegaex commented Jan 15, 2022

One more thing: I think it makes sense to document for both Spock 1.x and previously released 2.x (2.0, 2.1-M1, 2.1-M2) versions that mocking interfaces with default methods is going to fail on JDK 16+, unless --add-opens=java.base/java.lang.invoke=ALL-UNNAMED is used when starting the JVM. Like you said about 3 months ago on Slack, even proving your point with a download statistics graph: "There is still quite a lot of usage of old Spock versions." So I think, we should document that. Neither the main GitHub read-me nor the Spock manual currently seem to contain sections mentioning system requirements for running Spock, so I am not sure where a PR would be best. I leave that up to you to decide, but then I can certainly contribute, if you would e.g. agree to push out new GitHub pages not just for the current version, but also for 1.1, 1.2, 1.3. Those manuals are still online.

@leonard84
Copy link
Member

why did you not mention that this is a known fact and using the then master or milestone required that option to run successfully in order to use some Spock features? I see that since 2021-10-05 that option was part of the build configuration. Instead you argued that the error is not reproducible because of some supposed difference in how Gradle sets up the JVM with regard to Java modules, ...

Because I forgot that it was part of the build script, and I couldn't reproduce the problem due to the workaround being present. I only stumbled upon that snippet while getting the PR ready for merge.

One more thing: I think it makes sense to document for both Spock 1.x and previously released 2.x (2.0, 2.1-M1, 2.1-M2) versions that mocking interfaces with default methods is going to fail on JDK 16+, unless --add-opens=java.base/java.lang.invoke=ALL-UNNAMED is used when starting the JVM.

1.x won't run on 17 anyway as that requires Groovy 3 or 4. As for updating the docs for older versions, that would entail manually editing the HTML in the gh-pages branch as the system isn't really designed to be updated. I think a stackoverflow post with a self-answer might actually be the best way in this case WDYT @kriegaex? Most users google the problem instead of reading documentation?

@kriegaex
Copy link
Contributor

kriegaex commented Feb 19, 2022

1.x won't run on 17 anyway as that requires Groovy 3 or 4.

That statement is way too general. My playground project is still on Spock 1.3 with Groovy 2.5.14, Geb 3.0.1 and Selenium 3.14.0. Of course, I cannot compile to bytecode targets 16 or 17, but it builds and runs just fine on JRE 17, see e.g. this build log. Latest Chrome browser, JDKs 8, 11, 17, OS Windows, Linux and MacOS.

I am using Groovy-Eclipse for compilation, not GMavenPlus, that is a bit different from what you probably use. Otherwise, it is a pretty standard project. There are even modules using PowerMock, Mockito and Sarek under Spock, in order to test the combinations.

Slightly off-topic: What does not work on Java 16+ is using the unofficial EmbeddedSpecRunner, because Spock instantiates the Groovy parser without a CompilerConfiguration which sets the correct byte code target, and Groovy itself stupidly defaults to using the highest one supported by the JVM, even though Groovy's own embedded ASM cannot understand the resulting class files later in the process. See my comments in this test. I think that Groovy should be improved in this regard, but also Spock itself could do a better job, not using the default compiler config but creating its own one and/or offering a constructor to set the Java byte code target level.

As for updating the docs for older versions, that would entail manually editing the HTML in the gh-pages branch as the system isn't really designed to be updated.

Hm, would you accept PRs for those branches? I do not want to create any shadow documentation on Stack Overflow. I also think that it would be nice for the project to be more open for backporting some essential fixes and documentation updates to the 1.x line.

Sorry for the meta discussion. We can just leave it at that and let this issue rest in peace. But like I said, running Spock 1.3 projects under Java 17 is a real-life scenario when compiling to targets <= 15, as supported by Groovy 2.5.14. My little example project has ~350 tests, some of them quite special, because the playground project is what I am using in order to answer Stack Overflow questions or try things.

@leonard84
Copy link
Member

Hm, would you accept PRs for those branches? I do not want to create any shadow documentation on Stack Overflow. I also think that it would be nice for the project to be more open for backporting some essential fixes and documentation updates to the 1.x line.

I would be open to accept PRs updating the docs if necessary, but only if that is a small change and not modifying the whole document.

I just don't have the time to also maintain the 1.x branch, even 2.x releases often take longer than I want. As there hasn't been a Sarek release (that I know of), I'm sure you can relate 😉 .

@kriegaex
Copy link
Contributor

kriegaex commented Feb 22, 2022

Slightly off-topic: What does not work on Java 16+ is using the unofficial EmbeddedSpecRunner, because Spock instantiates the Groovy parser without a CompilerConfiguration which sets the correct byte code target, and Groovy itself stupidly defaults to using the highest one supported by the JVM, even though Groovy's own embedded ASM cannot understand the resulting class files later in the process. See my comments in this test. I think that Groovy should be improved in this regard, but also Spock itself could do a better job, not using the default compiler config but creating its own one and/or offering a constructor to set the Java byte code target level.

Correction: I tried extending Spock 1.3 in order to create the embedded spec runner with a compiler configuration, but it did not work. I realised that it actually defaults to producing Java 1.7 byte code, which can be overridden even without changing Spock via e.g. -Dgroovy.target.bytecode=15. The reason that it does not work is another one: Groovy 2.5.15 embeds ASM 8.0.1, which cannot understand Java 16+ byte code, but when compiling a script, the parser needs to analyse JRE class files, some of which have Java 16 or 17 format (class file versions 60 or 61). Simply upgrading the embedded ASM to 9.2 enables Groovy 2.5 to analyse and compile up to JDK 18 class files, making the problem go away. If my PR apache/groovy#1690 gets accepted, the next Groovy version 2.5.16 should be able to run without any problems on Java 16/17, i.e. the same would be true for Spock 1.3-groovy-2.5. I tested this in Spock 1.3 with a Groovy snapshot, and it works beautifully, even with EmbeddedSpecRunner. I.e., in the future it should even be possible to build 1.3 on Java 17. Maybe we need a few --add-opens, I did not test it. But that was not my goal anyway. The goal was to be able to use Spock 1.3 on JDK 16+, which now I can even when compiling specs on the fly.

mbland added a commit to mbland/tomcat-servlet-testing-example that referenced this issue Dec 23, 2023
Removing gradle-node-plugin cuts the project's dependency footprint
slightly, and enables Windows builds to succeed.

Executing `pnpm` from a Gradle task proved straightforward. Also added a
frontendInstall task, removed the separate "Install Node packages" step
from run-tests.yaml, and refined `inputs` and `outputs` for more precise
dependency tracking.

Specifically learned about using `outputs.upToDateWhen { true }` as a
means to prevent `frontendInstall` from always running from:

- https://discuss.gradle.org/t/why-does-my-task-run-even-though-non-of-its-inputs-has-changed/5350/2

---

I entertained the notion of fixing Windows builds by removing
gradle-node-plugin after a couple of days of trying to fix that plugin.

The moral of the following story:

- Sometimes you really _shouldn't_ add a dependency if you can do what
  you need to do without it.

When starting the project, I thought running Node.js from Gradle might
be tricky, so I searched for a plugin. As it turned out,
node-gradle/gradle-node-plugin was available, and seemed to work well on
macOS and Linux. However, it broke the project on Windows, because it
failed trying to execute `uname` on Windows.

I got the plugin successfully working on Windows by introducing the
following PowerShell command to replace uname (in
com.github.gradle.node.NodePlugin.addPlatform()):

```kotlin
if (name.toLowerCase().contains("windows")) {
    executable = "powershell.exe"
    args = listOf(
        "-Command",
        "& {Get-CimInstance Win32_OperatingSystem | " +
            "Select-Object -ExpandProperty OSArchitecture}"
    )
}
```

I also updated com.github.gradle.node.util.parseOsArch to handle the
"ARM 64-bit Processor" value returned by the command.

However, many tests continued to fail, which I realized was because no
`win-arm64` build of Node.js was available for the default Node.js
version 18.17.1:

- https://nodejs.org/dist/v18.17.1/

So I updated the default to 20.10.0 (and the npm version to 10.2.3,
which ships with 20.10.0). All the (very slow) tests began to pass
except for NpmProxy_integTest and YarnProxy_integTest.

After some creative debugging whereby I emitted the test-supplied values
for HTTP_PROXY and HTTPS_PROXY, I could see that Node couldn't see the
test-supplied values. Some digging revealed that past versions of Gradle
and its TestKit didn't have native support for arm64 that would allow
for setting environment variables:

- gradle/gradle: TestKit does not pass environment variables for older
  Gradle versions when running on M1 mac #22012:
  gradle/gradle#22012

It seemed that Gradle 7.5.1, the default used by gradle-node-plugin,
should be OK. However, it didn't seem to be.

And this is where the despair really started kicking in.

I tried various combinations of updating the JDK from 11 to 17.0.9,
Gradle from 7.5.1 to 8.5, and updating the build.gradle.kts file.

I thought updating to 17.0.9 might solve some problems, and it seemed as
high as I could go to still suport 7.5.1:

- https://docs.gradle.org/current/userguide/compatibility.html

One minor change to the build file involved moving merging entries from
the deprecated pluginBundle block to the existing gradlePlugin block:

- https://docs.gradle.org/current/userguide/upgrading_version_7.html#pluginbundle_dropped_in_plugin_publish_plugin

Another involved disabling the com.cinnober.gradle.semver-git plugin,
because launching subprocesses during the configuration phase is now an
error when using the configuration cache:

- https://docs.gradle.org/current/userguide/upgrading_version_7.html#use_providers_to_run_external_processes
- https://docs.gradle.org/current/userguide/configuration_cache.html#config_cache:requirements:external_processes

I also deleted the ill advised and useless org.gradle.test-retry plugin
and its configuration. The tests didn't appear to be unstable, so the
tests would run multiple times to keep failing in the same way. If they
didn't fail the same way, or sometimes passed, that'd've been even
worse. See:

- https://mike-bland.com/2023/09/13/the-inverted-test-pyramid.html

Then the build would fail while compiling Kotlin:

```text
  API version 1.3 is no longer supported; please, use version 1.4 or
  greater.
```

So I set in the build file:

```kotlin
tasks.compileKotlin {
    kotlinOptions {
        apiVersion = "1.9"
        languageVersion = "1.9"
```

I also boosted IntelliJ IDEA "Settings > Build, Execution, Deployment >
Compiler > Kotlin Compiler > Target JVM Version" setting to 17. Then I
updated the build file variable:

```kotlin
val compatibilityVersion = JavaVersion.VERSION_17
```

And then, finally, tests still failed because something funny happened
inside the Spock test framework, with a bunch of errors like:

```text
SystemVersion_integTest > use system node version and exec node, npm, npx and yarn program (#gv.version) > use system node version and exec node, npm, npx and yarn program (8.5) FAILED
    java.lang.reflect.InaccessibleObjectException: Unable to make field private final java.util.Map java.util.Collections$UnmodifiableMap.m accessible: module java.base does not "opens java.util" to unnamed module @609db546
        at org.junit.contrib.java.lang.system.EnvironmentVariables.getFieldValue(EnvironmentVariables.java:188)
        at org.junit.contrib.java.lang.system.EnvironmentVariables.getEditableMapOfVariables(EnvironmentVariables.java:150)
        at org.junit.contrib.java.lang.system.EnvironmentVariables.access$200(EnvironmentVariables.java:49)
        at org.junit.contrib.java.lang.system.EnvironmentVariables$EnvironmentVariablesStatement.restoreOriginalVariables(EnvironmentVariables.java:134)
        at org.junit.contrib.java.lang.system.EnvironmentVariables$EnvironmentVariablesStatement.evaluate(EnvironmentVariables.java:125)
        at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
        at org.spockframework.junit4.AbstractRuleInterceptor.evaluateStatement(AbstractRuleInterceptor.java:66)
    [ ...snip... ]
```

I already decided at this point to get rid of gradle-node-plugin, and
developed this change. But then I decided to go a little further.

This appears to be related to:

- spock/spockframework: Cannot create mock due to module java.base does
  not "opens java.lang.invoke" #1406
  spockframework/spock#1406

This appears to be a consequence of:

- JEP 396: Strongly Encapsulate JDK Internals by Default
  https://openjdk.org/jeps/396

This apparently shipped in Java 16, and people noticed in in Spock after
upgrading to Java 17.

Based on the information in the spock issue, I tried adding the
following `jvmArgs` entry in the build file:

```kotlin
tasks.withType(Test::class) {
    useJUnitPlatform()
    systemProperty(
        // ...snip...
    )
    jvmArgs("--add-opens", "java.base/java.util=ALL-UNNAMED")
```

Reference for `--add-opens`:

- https://docs.oracle.com/en/java/javase/17/migrate/migrating-jdk-8-later-jdk-releases.html#GUID-12F945EB-71D6-46AF-8C3D-D354FD0B1781

This actually fixed the error, but on macOS, three tests failed with:

```text
> Task :npmInstall FAILED
npm WARN tarball tarball data for npm@https://registry.npmjs.org/npm/-/npm-10.2.3.tgz (sha512-lXdZ7titEN8CH5YJk9C/aYRU9JeDxQ4d8rwIIDsvH3SMjLjHTukB2CFstMiB30zXs4vCrPN2WH6cDq1yHBeJAw==) seems to be corrupted. Trying again.
npm ERR! code EINTEGRITY
npm ERR! sha512-lXdZ7titEN8CH5YJk9C/aYRU9JeDxQ4d8rwIIDsvH3SMjLjHTukB2CFstMiB30zXs4vCrPN2WH6cDq1yHBeJAw== integrity checksum failed when using sha512: wanted sha512-lXdZ7titEN8CH5YJk9C/aYRU9JeDxQ4d8rwIIDsvH3SMjLjHTukB2CFstMiB30zXs4vCrPN2WH6cDq1yHBeJAw== but got sha512-GbUui/rHTl0mW8HhJSn4A0Xg89yCR3I9otgJT1i0z1QBPOVlgbh6rlcUTpHT8Gut9O1SJjWRUU0nEcAymhG2tQ==. (2583937 bytes)
```

The tests being:

- NpmRule_integTest. succeeds to run npm module using npm_run_ when the
  package.json file contains local npm (8.5)
- NpmTask_integTest. execute npm command using the npm version specified
  in the package.json file (8.5)
- NpxTask_integTest. execute npx command using the npm version specified
  in the package.json file (8.5)

And on Windows, after all of that, the NpmProxy and YarnProxy tests
still failed. At least the changes in this commit will allow Windows to
build now, right...?

```text
* What went wrong:
Failed to calculate the value of task ':strcalc:compileJava' property
'javaCompiler'.
WindowsRegistry is not supported on this operating system.
```

Great:

- gradle/native-platform: WindowsRegistry is not supported on this
  operating system. #274
  gradle/native-platform#274 (comment)
- gradle/gradle: Support ARM64 Windows #21703
  gradle/gradle#21703

So much for Windows on arm64 for now.

This was obviously quite the learning journey, but it's only reminded me
again of how much I hate the Java ecosystem. Yet more days I wish I had
back in my life.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants