RFR 8180573: Refactor sun/security/tools shell tests to plain java tests

[Valerie Peng]

The updated webrev looks good~

Yes, I like to explicitly check the return value - it's quite easy with 
OutputAnalyzer and doing so will mark first point of failure which saves 
debugging time. New test classes look very good thanks to those test 
library classes.

Thanks,
Valerie
On 4/10/2019 8:31 PM, Weijun Wang wrote:
> 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.peng at 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.wang at 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.kunz at 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
>>>>>>
>>>>>>