8384815: SelectOneKeyOutOfMany and PreferredKey fail after expired test certificate#3203
8384815: SelectOneKeyOutOfMany and PreferredKey fail after expired test certificate#3203sendaoYan wants to merge 3 commits into
Conversation
|
👋 Welcome back syan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
This backport pull request has now been updated with issue from the original commit. |
Webrevs
|
|
@sendaoYan Good to see this backported! I was about to backport it. I've added some hints that may prove useful. |
| * java.base/sun.security.util | ||
| * @library /test/lib ../../../../java/security/testlibrary | ||
| * @build CertificateBuilder | ||
| * @run main PreferredKey |
There was a problem hiding this comment.
Maybe:
* @library ../../../../java/security/testlibrary/
* @library /test/lib
* @build jdk.test.lib.Utils
* CertificateBuilder
Is a better approach? (we're using it elsewhere in JDK11, e.g.
)There was a problem hiding this comment.
I have split the '@library' as two lines. But I do not add '@build jdk.test.lib.Utils', seems these two tests do not need build this class jdk.test.lib.Utils.
gnu-andrew
left a comment
There was a problem hiding this comment.
- I think JDK-8325766 should be backported first rather than just cherry-picking
setOneHourValidity. This solves the same issue for other pre-generated certificates. This should be a separate PR this one can depend on. - Rather than working around the bad return values for
CertificateBuilder, I would cherry-pick theCertificateBuilder.javachanges in JDK-8179502 as an additional commit on this PR. The whole thing is an enhancement with CSR not suitable for backport, but theCertificateBuilder.javachanges are largely unrelated and fix it to actually work to the Builder pattern as the name suggests. - I'm not sure why you need to add
@run mainlines here. Isn't that the default? I don't see them in the 17u patch. Did the tests fail without this?
I add the '@build CertificateBuilder' line for both tests, because CertificateBuilder.java located different directories in jdk17u and jdk11u. In jdk11u jtreg will not build CertificateBuilder.java by default, So I add this line. And if I add ''@build CertificateBuilder' but not suffix I will try to cherry-pick JDK-8179502 and JDK-8325766 first. Thanks for the suggestions. |
8325766 should be a separate PR. It is too complicated to be combined with this change. |
Okey, thanks for the advices. I do create a new PR later. |
I've created #3222 for backporting 8325766. It was quite involved and also required the 8179502 changes that this one needs. Assuming this is integrated, it should simplify this backport considerably. |
Hi all,
This is backport PR to make test generate certificates during test run which avoid the fixed certificate expired cause test fails.
There are two changes make this PR backported uncleanly.
setOneHourValidity. I add setOneHourValidity() manully.Change has been verified locally by run the releated on linux-x64. Test-fix only, no risk.
Progress
Error
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/3203/head:pull/3203$ git checkout pull/3203Update a local copy of the PR:
$ git checkout pull/3203$ git pull https://git.openjdk.org/jdk11u-dev.git pull/3203/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3203View PR using the GUI difftool:
$ git pr show -t 3203Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/3203.diff
Using Webrev
Link to Webrev Comment