Hi Valerie, Thanks for the review. Webrev updated at
https://cr.openjdk.java.net/~weijun/8180573/webrev.02/ You can look at the interdiff only. > On Apr 11, 2019, at 5:29 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > Hi Max, > > Here are some comments/questions: > > General question, some of the tests use same name for keystore file, i.e. ks. > Will these tests interfere with each other at runtime? The shell scripts > often do some cleanup before starting the real test. I assume we don't have > to concern ourselves with this anymore? Correct. jtreg will clean up (and retain if requested) the files, and when multiple tests run in parallel they have different working directories. > > <i18n.java> what does this test trying to test? I don't quite understand its > value... Do we still need it? This is a very manual test and I have seen bug reported on it not very long ago, so there are people running it. It's now just a rewrite of i18n.sh and both should always pass at no time. I assume the test just needs an entry with @test. I do realize I should remove the <applet> element in i18n.html. BTW, I think the most correct way should be i18n.java printing out the instructions, asking the user to try out all those command, and enter a Y or N as the result. Then if user type in N the test fails. :-) > > <StandardAlgName.java> line 41 uses 1024 as key size, but shell script has > 2048. We should check the three commands completed successfully as well? OK and OK. I didn't checked the exit code because the original test is checking for the result of grep. Now I think it's never a bad idea to check the keytool exit code since it's handy with OutputAnalyzer. > > <AlgOptions.java> shell script has additional "-sigalg SHA512withRSA" options > for the last test. Good catch. > > <CloneKeyAskPassword.java> line 67, 68, check for non-null return value? OK. > > <PassType.java> nit: 2nd copyright year should be 2019, OK. > > <SameName.java> line 47 check for 0 exit value? Yes I can. The shell test hadn't, although. > > <PercentSign.java> line 27: typo, should be "containing" Fixed. > > <EC.java> line 48: check for 0 exit value? Yes. > Yes. Thanks, Max > <ConciseJarsigner.java> line 216 seems redundant as it should be covered by > line 220 > > Nice to have java files instead of scripts. :) > > Valerie > > On 3/4/2019 4:26 PM, Weijun Wang wrote: >> Webrev updated at >> >> https://cr.openjdk.java.net/~weijun/8180573/webrev.01 >> >> BTW, last time I mistakenly removed ExportPrivateKeyNoPwd.java which is used >> by ListKeychainStore.sh. It's now back. >> >> Thanks, >> Max >> >>> On Feb 15, 2019, at 9:31 PM, Weijun Wang <weijun.w...@oracle.com> wrote: >>> >>> Hi Philipp, >>> >>> In most cases, it's just about creating a non-empty file; in some case, the >>> content is relevant. For the former, I will change it to something like >>> "new byte[10]"; for the latter, I'll use getBytes(UTF_8). >>> >>> Thanks, >>> Max >>> >>> >>>> On Feb 15, 2019, at 5:34 PM, Philipp Kunz <philipp.k...@paratix.ch> wrote: >>>> >>>> Hi Max, >>>> >>>> I don't know if it is important enough, certainly not a serious issue. >>>> In your patch, for example in DiffEnd.java and a few other tests, Strings >>>> are encoded to byte streams with String.getBytes() which uses the default >>>> platform character set to encode the strings. >>>> Manifests, however, always use UTF-8 as the character set to encode. In my >>>> opinion, getBytes(java.nio.charset.StandardCharsets.UTF_8) would be >>>> appropriate to specify the encoding in a platform-independent way. >>>> Now the manifests used in the tests use so few different characters that >>>> this might not even make a real difference because the first around 100 >>>> characters or so of most character sets are the same in most encodings. >>>> I suspect that the encoding might also have been platform-dependent in at >>>> least some of the previous shell tests and therefore this aspect might as >>>> well be addressed separately and is not strictly necessary to just >>>> properly convert shell tests to java. >>>> >>>> Regards, >>>> Philipp >>>> >>>> >>>> On Wed, 2019-02-13 at 11:01 +0800, Weijun Wang wrote: >>>>> Please review the fix at >>>>> >>>>> >>>>> https://cr.openjdk.java.net/~weijun/8180573/webrev.00/ >>>>> >>>>> >>>>> Notes: >>>>> >>>>> - Most changes are just .sh -> .java >>>>> >>>>> - StorePasswordsByShell.sh combined into StorePasswords.java >>>>> >>>>> - In most cases, JarUtils is called to create a jar file. Sometimes the >>>>> jar command is called because of delicate differences, for example, jar >>>>> adds directory entries also. >>>>> >>>>> - The i18n tests are completely manual described in i18n.html. Old >>>>> i18n.java is useless, now is also empty. >>>>> >>>>> - Copyright year updated to 2019, @bug unchanged. >>>>> >>>>> Two files not converted yet: >>>>> >>>>> - ./keytool/console.sh >>>>> >>>>> This is a manual test calling old versions of JDK. >>>>> >>>>> - ./keytool/ListKeychainStore.sh >>>>> >>>>> I tried on this one but "security list-keychains -s ..." has no effect on >>>>> mach5 machines when calling by ProcessTools. No idea why, I've created a >>>>> separate bug (JDK-8218886) for it. >>>>> >>>>> Thanks, >>>>> Max >>>>> >>>>>
