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.ji...@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 >>> >
