8375322: Improve DomainKeyStore.java coverage#30712
8375322: Improve DomainKeyStore.java coverage#30712myankelev wants to merge 1 commit intoopenjdk:masterfrom
Conversation
|
👋 Welcome back myankelevich! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@myankelev 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
|
mpdonova
left a comment
There was a problem hiding this comment.
one minor comment. Otherwise, looks good to me.
| @@ -0,0 +1,106 @@ | |||
| /* | |||
| * Copyright (c) 2013, 2026, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
this looks like a completely new file; is the copyright correct?
There was a problem hiding this comment.
Kept it as is as this is actually an old test with other cases removed. I only edited and renamed it.
The other file has older name and refactored other tests in addition to the new ones
There was a problem hiding this comment.
Out of curiosity: What is the reason why this separate test is needed though? It seems some of the JUnit tests in DKSTest even call the checkEntries method here.
There was a problem hiding this comment.
This test uses system properties which could only set as a script or a subprocess. Either way this is cleaner than starting 2 versions of the same test
Marcono1234
left a comment
There was a problem hiding this comment.
Hopefully these comments are useful, if not please let me know. They are only suggestions; feel free to ignore them, I am not an OpenJDK member.
test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java
Outdated
Show resolved
Hide resolved
test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,106 @@ | |||
| /* | |||
| * Copyright (c) 2013, 2026, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
Out of curiosity: What is the reason why this separate test is needed though? It seems some of the JUnit tests in DKSTest even call the checkEntries method here.
|
|
|
/open |
|
@myankelev This pull request is now open |
|
@myankelev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@mpdonova @Marcono1234 Had to force push and overwrite the commits due to the issue with rebasing |
DomainKeyStore.javaDomainKeyStore.javawould be required, but should be done as a separate ticketProgress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30712/head:pull/30712$ git checkout pull/30712Update a local copy of the PR:
$ git checkout pull/30712$ git pull https://git.openjdk.org/jdk.git pull/30712/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30712View PR using the GUI difftool:
$ git pr show -t 30712Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30712.diff
Using Webrev
Link to Webrev Comment