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

JDK-8293877: Rewrite MineField test #10366

Closed

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Sep 21, 2022

Please review an update to convert the remaining langtools shell tests, in test/langtools/tools/javac/Paths/* from shell to Java code.

This will be an awkward/difficult/tedious (choose your adjective) review. The old files have been deleted, and the new code is different enough to defeat any diff program. That being said, I have done what I can to provide a line-for-line translation as best I can, in the parts of the code that matter. It may help to use tools to display the old code and new code side-by-size, in an ad-hoc fashion, and manually scrolling the versions together.

The old shell tests were written in a stylized and clever manner to reduce the verbiage of writing in shell script. The new tests are written in a differently stylized manner, using API methods to provide much of the same level of convenience.

Of the six tests, 5 have an obvious name translation (the hyphen is dropped from Class-Path.sh and Class-Path2.sh, and the cryptically named wcMineField.sh is renamed to the less cryptic WildcardMineField.java.

The supporting API in the new Util.java class is somewhat similar to the older Util.sh code, but is much less of a line-for-line translation.

Where possible, tools are invoked using the tool's ToolProvider API, as compared to exec-ing a separate process. A separate process is still always necessary for the main Java launcher, and is sometimes needed for Java ... when javac should be executed in a different directory, or with different env variables, or when using the "class path wildcard" feature.

On the use of cleanup methods... all the tests here call a cleanup() method as part of initial setup, and after the test is complete. Such cleanup is unnecessary beforehand when using jtreg (because jtreg guarantees an empty scratch directory when a test is run) and can be inconvenient afterwards, if you want to debug why a test-case has failed, although for sure it is arguably good to do if the test has passed. But any change would be a difference in the old and new code, and should arguably be done separately. So, the existence and use of cleanup methods is as before.

Some code was commented out in the original tests, because various options and system properties became unavailable in JDK 9. The commented out code is preserved, if only to provide visual anchors between the old and new versions. It may be desirable to eventually clean out the commented-out code.

The old code supported use of ${TESTTOOLVMOPTS} and ${TESTVMOPTS} although it is not clear how effective such support was, or how much it was used. The new code does not support these environment variables, or their equivalent system properties. Given that the new code uses the ToolProvider API where possible, it is not clear how (or how easily) we could integrate support for those options, and whether it is worth the effort.

Needless to say, all the new tests pass. For positive test cases, that is definitely reassuring. For negative test cases, it is somewhat harder to verify that each new test case fails for the same reason that the old test case failed, not least because of the lack of info provided by the old tests. For this reason, it is assumed it is sufficient to have confidence in the translation of all the test cases.

Separately, there is a minor fix in ToolBox, to address an NPE. The fix is small enough and obvious enough to include here, as compared to having a separate JBS issue and PR.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10366/head:pull/10366
$ git checkout pull/10366

Update a local copy of the PR:
$ git checkout pull/10366
$ git pull https://git.openjdk.org/jdk pull/10366/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10366

View PR using the GUI difftool:
$ git pr show -t 10366

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10366.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 21, 2022

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 21, 2022
@openjdk
Copy link

openjdk bot commented Sep 21, 2022

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the compiler compiler-dev@openjdk.org label Sep 21, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 21, 2022

Webrevs

@Martin-Buchholz
Copy link
Member

Explicit OS command invocation and ToolProvider invocation both seem important to test.
IIRC there were times when they were not in fact equivalent in effect.
Consider library infrastructure to run both variants and ensure results are identical.

I recall being slightly proud when writing the first iteration of this test - it allowed a very terse test description in shell.
But terseness is not universally admired, and even I (in my dotage) have joined the camp of writing tests in more verbose junit style.

@jonathan-gibbons
Copy link
Contributor Author

Explicit OS command invocation and ToolProvider invocation both seem important to test. IIRC there were times when they were not in fact equivalent in effect. Consider library infrastructure to run both variants and ensure results are identical.

@Martin-Buchholz
Thanks for the feedback. While I understand what you are suggesting, I think it is potentially for a different test.

The goal here was to try and do a reasonably direct translation of the original code from shell to Java. And OK, I did do some minor optimizations to use javac's ToolProvider where possible, to save exec-ing processes unnecessarily.

Rewriting the test to do both (where that is even possible) would either be a significant rewrite or maybe involve running all of the test twice, in both modes.

And, for javac, it might be even worse, because there is the javax.tools.JavaCompiler API as well.

There is no reason to suspect the functionality might be semantically different in these different ways to invoke javac. Once you get down into the compiler, the code paths are the same. The difference is the availability of features ... such as the classpath wildcard feature, which is only available when using the native launcher. So, if we were to write anything more in the way of tests, it would be to test the specified difference in availability of the features, without necessarily exercising the full functionality and comparing everything against the launcher for the Java runtime, java.

One other outcome from the overall work here: as a result of (re)discovering the classpath wildcard feature, we realized that there were enough differences between the native launcher and the ToolProvider interface (and JavaCompiler) to justify a spec update for the idk.compiler module. This is being tracked in JDK-8294546.

expectPass(JAVA, "-cp jars/B.zip${PS}. Main");

/*
* The following lines are commented out in the original,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these old comments could be removed

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

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

I have gone through all the changes, I think semantically the original tests have been preserved as closed as possible. After a first review iteration I think it could make sense to clean up a bit an remove dead code etc

@openjdk
Copy link

openjdk bot commented Sep 30, 2022

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293877: Rewrite MineField test

Reviewed-by: vromero, martin, darcy, jlahoda

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 202 new commits pushed to the master branch:

  • 6e8f038: 8294567: IGV: IllegalStateException in search
  • bc668b9: 8293099: JFR: Typo in TestRemoteDump.java
  • 03f25a9: 8293562: blocked threads with KeepAliveCache.get
  • a69ee85: 8292336: JFR: Warn users if -XX:StartFlightRecording:disk=false is specified with maxage or maxsize
  • b8b9b97: 8294676: [JVMCI] InstalledCode.deoptimize(false) should not touch address field
  • fd59430: 8294610: java/net/vthread/HttpALot.java is slow on Linux
  • c7ab1ca: 8294609: C2: Improve inlining of methods with unloaded signature classes
  • 375f02f: 8294608: Remove redundant unchecked suppression in FileDescriptor
  • d207da8: 8294533: Documentation mistake in Process::getErrorStream and getInputStream
  • da4e96d: 8276545: Fix handling of trap count overflow in Parse::Parse()
  • ... and 192 more: https://git.openjdk.org/jdk/compare/6beeb8471ccf140e59864d7f983e1d9ad741309c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 30, 2022
@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Oct 1, 2022

I have gone through all the changes, I think semantically the original tests have been preserved as closed as possible. After a first review iteration I think it could make sense to clean up a bit an remove dead code etc

Yeah, I'm inclined to agree that the time has come to remove the dead code related to those long-gone options and system properties.

Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle left a comment

Choose a reason for hiding this comment

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

dead code removal looks good

@Martin-Buchholz
Copy link
Member

It's clear that javac team intends to have ToolProvider and command line invocation be very equivalent (good!) but I have memories of it being quite tricky to convert from one to the other, and I would be wary of lingering bugs, especially in the path handling code.

I care more than most about the performance of test runs, but for command line flags, I would always at least test them via subprocess invocation.

Copy link
Member

@Martin-Buchholz Martin-Buchholz left a comment

Choose a reason for hiding this comment

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

I spent a few nostalgic minutes looking over the translation into java. Looks good.

I regret not having added more comments about why particular invocations were expected to pass or fail.

I was introduced to junit after writing these. I might have tried to do more junitization if I was doing the translation.

Despite having implemented wildcards, I was never keen on the feature, and haven't used it myself in the Real World.

* Converted from Class-Path.sh, originally written by Martin Buchholz.
*
* For the last version of the original, Class-Path.sh, see
* https://github.com/openjdk/jdk/blob/jdk-19%2B36/test/langtools/tools/javac/Paths/Class-Path.sh
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Darn! missed this; will have to fix separately

Copy link
Contributor

@lahodaj lahodaj left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Oct 3, 2022

It's clear that javac team intends to have ToolProvider and command line invocation be very equivalent (good!) but I have memories of it being quite tricky to convert from one to the other, and I would be wary of lingering bugs, especially in the path handling code.

I care more than most about the performance of test runs, but for command line flags, I would always at least test them via subprocess invocation.

The comment is noted. Generally, the javac code is much better that it has been in times past, with better use of better abstractions. That being said, I agree there are different code paths in the outer layers of javac and it is important to be aware of the differences when introducing new options.

As for path options, I suspect any differences are more likely to arise when using the JavaCompiler (JSR199) API, compared to the ToolProvider and native launcher invocation.

@jonathan-gibbons
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 3, 2022

Going to push as commit e137f9f.
Since your change was applied there have been 210 commits pushed to the master branch:

  • 4f44fd6: 8237467: jlink plugin to save the argument files as input to jlink in the output image
  • edfb18a: 8294695: Remove redundant deprecation suppression in ThreadGroup
  • 46633e6: 8294698: Remove unused 'checkedExceptions' param from MethodAccessorGenerator.generateMethod()
  • f2a32d9: 8293691: converting a defined BasicType value to a string should not crash the VM
  • ccc1d31: 8294529: IGV: Highlight the current graphs in the Outline
  • 08a7ecf: 8294671: Remove unused CardValues::last_card
  • 5fe837a: 8294236: [IR Framework] CPU preconditions are overriden by regular preconditions
  • 8e9cfeb: 8294431: jshell reports error on initialisation of static final field of anonymous class
  • 6e8f038: 8294567: IGV: IllegalStateException in search
  • bc668b9: 8293099: JFR: Typo in TestRemoteDump.java
  • ... and 200 more: https://git.openjdk.org/jdk/compare/6beeb8471ccf140e59864d7f983e1d9ad741309c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 3, 2022
@openjdk openjdk bot closed this Oct 3, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 3, 2022
@openjdk
Copy link

openjdk bot commented Oct 3, 2022

@jonathan-gibbons Pushed as commit e137f9f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jonathan-gibbons jonathan-gibbons deleted the 8293877.minefield branch October 3, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler compiler-dev@openjdk.org integrated Pull request has been integrated
5 participants