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

Jump to bottom

Fix StandaloneTests specific for Java 8 #3416

Merged
merged 4 commits into from
Sep 10, 2023
Merged

Fix StandaloneTests specific for Java 8 #3416

merged 4 commits into from
Sep 10, 2023

Conversation

rybak
Copy link
Contributor

@rybak rybak commented Aug 6, 2023 •
edited
Loading

Overview

While working on JDK 22 compatibility in #3415, I've discovered a problem with tests specific for Java 8 – details in the last commit.

I've also reproduced this issue on commit c62cd6a, where the affected tests were introduced. The slight build script change needed to run Gradle on this commit are in a separate branch in my fork, along with some debug output.

I hereby agree to the terms of the JUnit Contributor License Agreement.

@rybak
Drop misleading messages in StandaloneTests
4d51cd6 
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.
@rybak
Copy link
Contributor Author

rybak commented Aug 6, 2023 •
edited
Loading

From 3fad8c6:

Tests that claim by their name to run on Java 8 don't actually run on Java 8.

Re-ran the tests on main to double-check:

Screenshot_20230806 java 8 tests

@rybak
Copy link
Contributor Author

rybak commented Aug 6, 2023 •
edited
Loading

I have a suspicion that this might be caused by a discrepancy in bytecode version in class files. In JDK 17, javac produces bytecode of version 61.0, while in JDK 8, javac produces bytecode of version 52.0.

In class StandaloneTests, test compile() launches javac with the current JDK, thus creating class files, which cannot be picked up – see new commit 795e01d post-force-push update: squashed into 3fad8c6.

With commit 795e01d, tests executeOnJava8() and executeOnJava8SelectPackage() now fail (post-force-push update: no longer fail in 3fad8c6) for more obvious reasons: output of -showversion goes to stderr instead of stdout, as expected by expected-out.txt.

rybak added a commit to rybak/junit5 that referenced this pull request Aug 6, 2023
@rybak
Merge branch 'fix-standalone-test-java8' into fix-java-22-console-tes…
6c20951 
…t-failures

Temporary merge to see if new tip of pull request junit-team#3416 fixes issue in
the Java 22 pull request.
@rybak rybak mentioned this pull request Aug 6, 2023
Closed
1 task
rybak added 2 commits August 6, 2023 23:44
@rybak
Unify messages about exit codes in StandaloneTests
3f2590d 
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.
@rybak
Run StandaloneTests for Java 8 under Java 8
3fad8c6 
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`.

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".

Footnotes
---------
[1] "Java Platform, Standard Edition Tools Reference", "java"
    https://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html
    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) from junit-team#2613
@rybak rybak force-pushed the fix-standalone-test-java8 branch from e96edf4 to 3fad8c6 Compare August 6, 2023 21:47
@rybak
Copy link
Contributor Author

rybak commented Aug 6, 2023

Force pushed to squash fixup commits and to reword commit messages.

@rybak rybak marked this pull request as ready for review August 6, 2023 21:49
@rybak
Copy link
Contributor Author

rybak commented Aug 6, 2023

I hope my explanation of the issue isn't too long and makes sense :-)

@marcphilipp marcphilipp added this to the 5.10.1 milestone Sep 1, 2023
@marcphilipp marcphilipp self-assigned this Sep 10, 2023
marcphilipp
marcphilipp approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

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

Thanks for spotting and fixing this! 👍

@rybak rybak force-pushed the fix-standalone-test-java8 branch from e53f934 to 060f2c4 Compare September 10, 2023 10:56
@rybak
Copy link
Contributor Author

rybak commented Sep 10, 2023

Removed two commits from the top:

$ git range-diff e53f93482a...060f2c4bec
1:  0b1a0029b3 < -:  ---------- Remove -Xlint:-options
2:  e53f93482a < -:  ---------- Revert "Remove -Xlint:-options"

@marcphilipp marcphilipp merged commit e802db4 into junit-team:main Sep 10, 2023
26 checks passed
@rybak rybak deleted the fix-standalone-test-java8 branch April 25, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcphilipp marcphilipp marcphilipp approved these changes

Assignees

@marcphilipp marcphilipp

Labels
None yet
Projects
None yet
Milestone
5.10.1
Development

Successfully merging this pull request may close these issues.
2 participants
@rybak @marcphilipp