8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test #11910
Conversation
….sh to jtreg java test
|
👋 Welcome back mpdonova! A progress list of the required criteria for merging this PR into |
Webrevs
|
mpdonova
changed the title
|
@mpdonova This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Can someone take a look at this? Thanks! |
| case 4 -> { | ||
| try { | ||
| run(new ServerFactory(SSLContext.getDefault(), | ||
| new String[]{"dummy_ciphersuite"}, null, false), | ||
| true); | ||
| } catch (NoSuchAlgorithmException exc) { | ||
| throw new RuntimeException("Could not create SSLContext.", exc); | ||
| } catch (IllegalArgumentException exc) { | ||
| if (!exc.getMessage().toLowerCase().contains("unsupported ciphersuite")) { | ||
| throw exc; | ||
| } | ||
| } |
There was a problem hiding this comment.
What is the story with the different exception catching here and in case 5? It doesn't look like a simple refactoring. Is another bug being fixed here?
There was a problem hiding this comment.
The expected result of cases 4 and 5 is that an IllegalArgumentException is thrown because of the unsupported ciphersuite. The original code just caught Exception which would hide a problem in the test e.g., if SSLContext.getDefault() throws the NoSuchAlgorithmException.
There was a problem hiding this comment.
OK - but the run( ) method catches IllegalArgumentException already?
There was a problem hiding this comment.
The exception is actually being thrown from the ServerFactory constructor. I updated the code to remove the call to run() and added a comment clarifying the exception handling.
| case 4 -> { | ||
| try { | ||
| new ServerFactory(SSLContext.getDefault(), | ||
| new String[]{"dummy_ciphersuite"}, null, false); |
There was a problem hiding this comment.
what if the constructor doesn't throw any exception? shouldn't the test fail? or should run be called in that case?
There was a problem hiding this comment.
good catch; yes, the test should fail. I updated these cases.
There was a problem hiding this comment.
Thanks for the update. I believe this looks good, but it would be nice to get @stuart-marks or @RogerRiggs approval before integrating.
|
@mpdonova 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: 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 21 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @RogerRiggs, @stuart-marks) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
| } | ||
| break; | ||
| default: |
There was a problem hiding this comment.
The default should be retained to produce an error.
There was a problem hiding this comment.
I put the default case back
| } | ||
| } | ||
| } | ||
|
|
||
| public void runTest(String[] args) { | ||
|
|
||
| int test = Integer.parseInt(args[0]); |
There was a problem hiding this comment.
Move the parseInt to main() and call runTest with the test number.
| * @run main/othervm SSLSocketParametersTest 1 | ||
| * @run main/othervm SSLSocketParametersTest 2 | ||
| * @run main/othervm SSLSocketParametersTest 3 | ||
| * @run main/othervm SSLSocketParametersTest 4 | ||
| * @run main/othervm SSLSocketParametersTest 5 | ||
| * @run main/othervm SSLSocketParametersTest 6 | ||
| * @run main/othervm SSLSocketParametersTest 7 |
There was a problem hiding this comment.
Is a fresh VM needed for each of these tests?
From a naive point of view it appears that 1, 2, 3, 4, 6, 7 use the same system properties.
The tests would complete more quickly if they could run the compatible tests in a single VM.
Perhaps main() could iterate over the args[n] and run the corresponding test.
There was a problem hiding this comment.
I tried this suggestion but test case 6 failed. From the logs, it looked like an object was being re-used and an exception wasn't being thrown when it should have been.
| e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like this finally-block is intended to unexport any exported RMI service, which will let the JVM exit. However, this is somewhat fragile. If the unexportObject() call fails with some other exception, the object might remain exported, and the JVM will hang indefinitely. (This has historically been a problem with RMI tests.)
Since the original version of the test ran each case in a separate JVM, I think it's ok to continue to do the same. The old version of the test called System.exit() along most code paths. It's somewhat odd that exit wasn't called along all code paths. Perhaps those code paths weren't taken, or if they were, the JVM exited of its own accord. In any case, I'd suggest ensuring that exit() is called along all code paths (success and failure) and keeping separate @run main/othervm lines in the header to run each case in its own JVM. This is at least equivalent to what the shell script based test was doing.
As an optimization, it might be reasonable to try to run some subset of the tests in a single JVM. They can't all be run in the same JVM though, because of system properties, so this would require some complexity to support running some cases in the same JVM and some in separate JVMs. It's not clear to me whether that's warranted.
There was a problem hiding this comment.
I don't think all of those code paths ever executed. If System.exit() is called, regardless of the exit code, make treats it as an error:
TEST RESULT: Failed. Unexpected exit from test [exit code: 0]
I added a try/catch in main so that if an exception is thrown during the test, System.exit will be called.
There was a problem hiding this comment.
My bad, I was confused about the execution environments. In the old test, jtreg invoked the shell script, which in turn invoked the Java test as a standalone Java program. In that environment it's paramount that every code path call System.exit() in order to avoid stale JVMs building up.
The new test is invoked directly by jtreg, which disallows calling System.exit(). Instead, every code path needs to result in the main() method completing somehow, either via a normal return or by throwing an exception directly or letting one propagate from some called method. So, you need to pull out the try/catch and any calls to System.exit(). Sorry about that.
If we're going to stick with a scheme where we're running separate JVM per test, there's no need to unexport any RMI service. When main() returns or throws an exception, jtreg will detect this and terminate the JVM because of /othervm, and any exported RMI services will be destroyed at that time. Thus, the finally-block in testRmiCommunication can simply be removed.
There was a problem hiding this comment.
I removed the finally block.
| if (!exc.getMessage().toLowerCase().contains("unsupported protocol")) { | ||
| throw exc; | ||
| } | ||
| } |
There was a problem hiding this comment.
Having exception handling in the run() method and also in this switch statement is quite confusing. It obscures the fact that these cases (4 and 5) don't call run() at all. It would be good to find a structure that makes it clear that some test cases are for exceptions that occur at ServerFactory creation time and other exceptions occur when an actual socket connection is attempted.
There was a problem hiding this comment.
Perhaps some refactoring should be done in a separate PR.
The stated goal was to remove the use of the shell script; but its easy to start to refactor more and more.
There was a problem hiding this comment.
I did a little refactoring here to clarify (I hope) the tests and remove some redundant code.
|
General comments: the ClientFactory, ServerFactory, and HelloClient classes don't seem to be adding much value. Also, the setup of the HelloImpl service could probably be simplified by not having it extend UnicastRemoteObject and instead calling UnicastRemoteObject.exportObject(). These are held over from the original test and don't have much do with the shell script conversion, though, so you might choose not to undertake these cleanups now. But if you're interested, let me know and we can discuss them further. |
| throw exc; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Overall this test method makes sense, as it asserts that an IllegalArgumentException must be thrown with the given exception message. (A comment to this effect would be helpful.) As such, the catch of NoSuchAlgorithmException is confusing. I think this is here because SSLContext.getDefault() lists this as a checked exception. If so, then I'd suggest simply propagating it by adding a throws clause for this method. This also means adding a throws clause to the runTest() method, possibly just throws Exception. In general this is OK for jtreg tests to propagate any checked exception out of main, since jtreg will handle and report any unexpectedly propagated exceptions. It's thus unnecessary for test code to have catch-clauses for anything that the test isn't actually handling.
There was a problem hiding this comment.
The exception handling has been cleaned up here and the other place you pointed out.
| if (!expectException) { | ||
| throw new RuntimeException("An error occurred during test execution", exc); | ||
| } else { | ||
| System.out.println("Caught expected exception: " + exc); |
There was a problem hiding this comment.
I'm not quite clear on what the expected exception actually is -- is it an IllegalArgumentException or an IOException? Or does it differ from case to case? It would be good to be precise about exactly which exception is expected to be thrown from which test case. If IOException is caught here only because it's declared to be thrown by some method, and it's incidental to the test, I'd suggest it be propagated. (See my similar comments on testServerFactory.)
|
OK, I think this is pretty good as it stands. It achieves the original goal of converting the shell script test to a regular Java jtreg test; the shell script is removed entirely, and the Java code is cleaned up and is 20% shorter. Hooray! So I think this can go in as it stands. Optionally, if you're game to continue working on this, there are some additional cleanups that can be done. This is now digging into the Java code that was already in the test before you got involved. Specifically:
or similar.
It's up to you (and your project lead) as to how to proceed. You could continue here, or in another PR, or just consider this done and move onto the next thing. |
|
I created a new bug, JDK-8303525, for the additional refactoring suggestions. I assigned it to myself but wouldn't be bothered if someone else wanted to take it. |
|
/integrate |
|
/sponsor |
|
Going to push as commit ccfe167.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@stuart-marks @mpdonova Pushed as commit ccfe167. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Removed SSLSocketParametersTest.sh script (which just called a Java file) and configured the java code to run directly with jtreg
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11910/head:pull/11910$ git checkout pull/11910Update a local copy of the PR:
$ git checkout pull/11910$ git pull https://git.openjdk.org/jdk pull/11910/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11910View PR using the GUI difftool:
$ git pr show -t 11910Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11910.diff