-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8362658: sun/security/ssl/SSLEngineImpl/* tests duplicate jvm flags #28428
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
base: master
Are you sure you want to change the base?
8362658: sun/security/ssl/SSLEngineImpl/* tests duplicate jvm flags #28428
Conversation
|
👋 Welcome back nehajoshinj! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@nehajoshinj The following label will be automatically applied to this pull request:
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. |
Webrevs
|
| Utils.addTestJavaOpts("SSLEngineKeyLimit", "p", args[1], | ||
| args[2])); | ||
|
|
||
| ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder("SSLEngineKeyLimit", "p", args[1], args[2]); |
There was a problem hiding this 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, but try not to overstep the 80 character line-width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the code .
|
LGTM. Also include an update to remove tests from ProblemList at https://github.com/openjdk/jdk/blob/master/test/jdk/ProblemList-jvmti-stress-agent.txt#L31 |
|
/contributor add @Korov |
|
@nehajoshinj Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
|
/contributor add Lei Zhu korov9.c@gmail.com |
|
@nehajoshinj |
myankelev
left a comment
There was a problem hiding this 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
In this PR we have updated the code to remove the duplicate call and ensure JVM flags are added only once.
ISSUE :-
In the test case --> ProcessTools.createTestJavaProcessBuilder(Utils.addTestJavaOpts("SSLEngineKeyLimit", "p", args[1], args[2]));
-> Before executing ProcessTools.createTestJavaProcessBuilder() we are calling ---> Utils.addTestJavaOpts() which internally call prependTestJavaOpts();
-> Then again inside ProcessTools.createTestJavaProcessBuilder() method we are calling Utils.prependTestJavaOpts()
Calling Utils.prependTestJavaOpts() twice leads to duplicate JVM flags (This method call-> getTestJavaOpts() the main reason for duplication and below is the implementation for the same. )
Contributed-by: @Korov
#26404
https://bugs.openjdk.org/browse/JDK-8362658
Progress
Issue
Reviewers
Contributors
<korov9.c@gmail.com>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28428/head:pull/28428$ git checkout pull/28428Update a local copy of the PR:
$ git checkout pull/28428$ git pull https://git.openjdk.org/jdk.git pull/28428/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28428View PR using the GUI difftool:
$ git pr show -t 28428Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28428.diff
Using Webrev
Link to Webrev Comment