-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix console-related test failures on JDK 22 #3415
Conversation
Recent pull requests have been failing on OpenJDK 22 in builds of GitHub Actions Workflow "Cross-Version". Latest successful builds used openjdk-22-ea+7_linux-x64_bin.tar.gz, while builds with openjdk-22-ea+8_linux-x64_bin.tar.gz and openjdk-22-ea+9_linux-x64_bin.tar.gz have been failing. The failing tests are all related to interaction with the console. For example, the banner of ConsoleLauncher was changed in commit [1] to include italics and underline formatting. The corresponding test ConsoleLauncherTests#displayBanner fails under Java 22, because it expects only the plain text, and not the formatting. Similarly, test StandaloneTests#listAllObservableEngines fails due to bold formatting around names of the libraries added in [2]. This failure seems to happen on openjdk-22-ea+9_linux-x64_bin due to recent change in JDK: "8308591: JLine as the default Console provider".[3], which was released as part of jdk-22+8.[4] Override this default behavior of JDK in tests using VM option `-Djdk.console=java.base`, as mentioned in release notes.[4] An alternative solution would be to rewrite the failing tests to check the strings regardless of console formatting. [1] f916512 (Fix some tests, 2023-04-26) [2] b2ab677 (Polishing, 2023-04-26) [3] https://bugs.openjdk.org/browse/JDK-8308591 https://bugs.java.com/bugdatabase/view_bug?bug_id=8308591 openjdk/jdk@bae2247 [4] https://jdk.java.net/22/release-notes openjdk/jdk@jdk-22+7...jdk-22+8
Fix for builds that don't have explicit javaToolchainVersion Note that in GitHub workflows, only cross-version.yml uses -PjavaToolchainVersion explicitly. An alternative would be to do something like what plugin junitbuild.java-toolchain-conventions.gradle.kts does, quote: val defaultLanguageVersion = JavaLanguageVersion.of(17) val javaLanguageVersion = buildParameters.javaToolchainVersion.map { JavaLanguageVersion.of(it) }.getOrElse(defaultLanguageVersion) Or to refactor the build parameter javaToolchainVersion to have a default value: integer("javaToolchainVersion") { description = "Defines the Java toolchain version to use for compiling code" defaultValue = 17 }
Test `execute()` in StandaloneTests asserts matching of lines between expected standard output and actual output lines. It also passes the same actual lines (but in a `String` form) as a message to method assertLinesMatch, which takes three arguments. Same situation for standard error. Drop this confusing argument from the two calls to assertLinesMatch and let class `AssertLinesMatch` construct the error message for us.
Some tests in StandaloneTests provide concatenation of standard output and standard error as a message for the assertion about exit code, while other tests provide only standard output. Some use method de.sormuras.bartholdy.Result#getOutput while others use method de.sormuras.bartholdy.Result#getOutputLines. Unify these approaches by replacing the messages with a supplier that gives a) a human-readable message about exit codes, and b) both standard output and standard error.
Problem ------- Tests that claim by their name to run on Java 8 don't actually run on Java 8. This can be clear from the output for tests that add option `--show-version` to the arguments and _don't_ fail -- they all print the version for the current JDK. The tool `java` of JDK 8 does _not_ have the option `--show-version`. The actual option that exists in JDK 8 has fewer hyphens, as per documentation of Java 8 [1]: -showversion Displays version information and continues execution of the application. This option is equivalent to the `-version` option except that the latter instructs the JVM to exit after displaying version information. And when I actually run Java 8 binary with the incorrect option `--show-version` used by the affected tests, I get: $ /usr/lib/jvm/java-8-openjdk-amd64/bin/java --show-version Unrecognized option: --show-version Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. The option `--show-version` was only added in Java 9 [2]. Explanation ----------- In actuality, the tests are being run on whatever the current JDK is. These tests create an instance of class `de.sormuras.bartholdy.tool.Java`, which importantly has the following method: @OverRide public Path getHome() { return Bartholdy.currentJdkHome(); } When the `bartholdy` library creates the process, class `AbstractTool` does the following: protected Path createPathToProgram() { return getHome().resolve("bin").resolve(getProgram()); } The string `"java"` returned from method `getProgram` of class `Java` gets resolved against `Bartholdy.currentJdkHome()`. As far as I can tell, the library doesn't promise to look up the `java` binary in the `JAVA_HOME` supplied in the environment. In fact, just before consuming library user's environment, method `run()` of class `de.sormuras.bartholdy.tool.AbstractTool` puts the current JDK as `JAVA_HOME` into the environment to correspond to the behavior of class `de.sormuras.bartholdy.tool.Java` described above: builder.environment().put("JAVA_HOME", Bartholdy.currentJdkHome().toString()); The issue has been present since commit [3] where these tests were introduced. Fix --- Fix affected tests to run them under actual Java 8 by overriding method `de.sormuras.bartholdy.tool.Java#getHome`. Replace erroneous option `--show-version` with `-showversion`. Footnotes --------- [1] "Java Platform, Standard Edition Tools Reference", "java" https://docs.oracle.com/javase/8/docs/technotes/tools/windows/java.html [2] https://docs.oracle.com/javase/9/tools/java.htm > `--show-version` or `-showversion` > > Displays version information and continues execution of the application. > This option is equivalent to the `-version` option except that the latter > instructs the JVM to exit after displaying version information. [3] c62cd6a (Fix package path computation in `ClasspathScanner`, 2021-05-12)
To make tests executeOnJava8() and executeOnJava8SelectPackage() see the class files, update test compile() to use option `--release 8`. Because compiling to release 8 is deprecated, add a linter option to disable the warning to make compile() pass.
Because option `-showversion` of Java 8 behaves slightly differently to option `--show-version` of later versions of Java, prepare two new files for expected stdout and stderr -- expected-out-java8.txt and expected-err-java8.txt -- which are similar to existing files expected-out.txt and expected-err.txt, but have different layout of fastforward lines "JAVA VERSION" and "TREE".
…t-failures Temporary merge to see if new tip of pull request junit-team#3416 fixes issue in the Java 22 pull request.
spotlessApply
…t-failures spotlessApply
TODO: write commit message about additions to StandaloneTests.
Thanks for working on this, Andrei. I created #3419 to track the underlying issue. My hope is that we'll find a solution that does not need the |
@sormuras thank you for review and creating the issue. I don't remember for sure, but I think I saw the tty problem locally too, so it might be related to JLine. Please also see related #3416 which is ready for review (unlike this PR). It is fixing an issue introduced in #2613, so you are probably the best reviewer for #3416. |
I was not able to reproduce directly, meaning on a standard shell - I can reproduce it on GitHub Actions though. See https://github.com/sormuras/JDK-8313893 for details. The issue originates from Picocli: remkop/picocli#1104 |
Superseded by #3423 |
Overview
Details are in the commit message.
I'm not sure if this is a good approach, or how JUnit project likes to handle compatibility issues like this. I tried to follow the example of surrounding code in
junitbuild.testing-conventions.gradle.kts
as best as I could.Originally, I tried fixing just
ConsoleLauncherTests
, but after reading the build logs more carefully, I realized that a lot of other console-related tests are also affected (see examples in the commit message).I hereby agree to the terms of the JUnit Contributor License Agreement.
TODO