RFR 8048357: PKCS basic tests

[Wang Weijun]

Why "othervm" for all these tests?

PKCS10AttrOrder.java:

- Is sun.security.provider and sun.misc used anywhere? You have them in @modules.

- sun.security.pkcs.PKCS9Attribute contains public constants like CONTENT_TYPE_OID, SIGNING_TIME_OID, CHALLENGE_PASSWORD_OID. You can directly use them. Same as PKCS10AttributeReader.java.

- What does "order" mean in the test name? PKCS10 Attributes is a Set, therefore there is no order.

- You print out "List of attributes in constructed PKCS10 request: " but I see no content.

- checkAttributes() ensures every attribute in the encoded PKCS10 has the correct value. But you haven't checked if all entries in constructedMap are encoded. Maybe compare the size?

PKCS10AttributeReader.java:

- sun.misc in @modules?

- Line 45: .asn1 -> asn.1

- Please also compare the size of RequestStander and eReq.

PKCS8Test.java:

- sun.security.x509 in @modules?

- Line 206: Dump encodedKey here? Seems too late. Why not do it before the check or at least before raiseException? Same with the other 4 debug outputs after raiseException.

- EXPECTED_ALG_ID_CHRS can be just a String with \n and \t inside.

pkcs7/*.java:

Please take a look at Bernd Eckenfels's comments on Aug 21. I fully agree with him.

Thanks
Max

----

> On Nov 13, 2015, at 4:57 PM, Amanda Jiang <amanda.jiang at oracle.com> wrote:
> 
> Hi Max,
> 
> Please check the updated webrev which address your comments, please let me know if you have any other suggestions.
> http://cr.openjdk.java.net/~amjiang/8048357/webrev.02/
> 
> Thanks,
> Amanda
> 
> On 15/8/21 上午12:13, Weijun Wang wrote:
>> PKCS10AttrOrder.java: 
>> 
>> - Why not inline revAttributes(), prov() and constructMap()? They are only used once. Putting the content into the main method is more clear. 
>> 
>> - You can create separate method for the while look checks. The 2 look identical. 
>> 
>> PKCS10AttributeReader.java: 
>> 
>> - Is it OK to indent the data in the comment to multiple levels? 
>> 
>> - Again, initMap() not necessary. 
>> 
>> PKCS7VerifyTest.java: 
>> 
>> - Not sure what this is for. The 1st and 2nd argument of the @run line are the same? 
> those 3 arguments are actually for two different test cases, I use two @run tags in updated webrev and add some comments in test to make it clear.
>> 
>> SignerOrder.java: 
>> 
>> - Comment on what derString1 and derString1 are. 
>> 
>> - What do you expect verifs1.length and verifs2.length to be? Will you also check it? 
> Yes I do check verifs1.length and verifs2.length should be same.
>  121         if (verifs1.length != verifs2.length) {
>  122             throw new RuntimeException("Length or Original vs read-in "
>  123                     + "should be same");
>  124         }
> 
> 
> 
>> 
>> PKCS8Test.java: 
>> 
>> - Typo: s/recieved/received/g 
>> 
>> Thanks 
>> Max 
>> 
>> On 08/21/2015 07:11 AM, Amanda Jiang wrote: 
>>> Hi All, 
>>> 
>>> Please be free to review new tests for conformance testing of PKCS. 
>>> 
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8048357 
>>> webrev: http://cr.openjdk.java.net/~amjiang/8048357/webrev.01/ 
>>> 
>>> Thanks, 
>>> Amanda 
>>> 
>