On Wed, 3 May 2023 14:12:33 GMT, Mahendra Chhipa <mchh...@openjdk.org> wrote:
>> Test is updated to create the binary files during test execution. > > Mahendra Chhipa has updated the pull request incrementally with one > additional commit since the last revision: > > Move the pseudo code generation part from setup() to seperate methods. Sorry for the delay but lots of tasks are getting time sliced in this week :-) I think you are getting closer. There is still some cleanup that can be done and more comments added for future maintainers. As we look at creating or updating tests, we want to try to improve their readability and documentation. Please see below for some examples of where we can look to improve the test test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 67: > 65: * @throws IOException > 66: * @throws ClassNotFoundException > 67: */ Please either use block comments or add the in the correct javadoc test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 73: > 71: XMLGregorianCalendar xmlGregorianCalendar = > dtf.newXMLGregorianCalendar(EXPECTED_CAL); > 72: //Serialize the given xmlGregorianCalendar > 73: final ByteArrayOutputStream baos = new ByteArrayOutputStream(); You could probably use try-with-resources for the streams that are created. I am not convinced we need to use `final` test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 117: > 115: > 116: /** > 117: *Provide data for JDK version and Gregorian Calendar serialized > bytes I would suggest adding a comment for the parameters that are being created by the DataProvider test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 129: > 127: > 128: /** > 129: *Provide data for JDK version and serialized Gregorian Calendar > Base64 encoded string Nit space should be before comment starts test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 167: > 165: * @throws IOException > 166: * @throws ClassNotFoundException > 167: */ See above regarding using a block comment or using the proper javadoc comment format. Either way, please define what the param values are used for test/jdk/javax/xml/jaxp/datatype/8033980/SerializationTest.java line 232: > 230: * JDK<version>GregorianCalendarAndDurationSerData.java files. > 231: * @param baos > 232: */ I think there needs to be a general comment describing how these methods are used to create the JDK<version>GregorianCalendarAndDurationSerData.java files. There should also be a description/comment for the methods defined in GregorianCalendarAndDurationSerData.java We need to try and put ourselves in the place of a future maintainer who needs to understand how to create a version of one of these files. You could probably also create a method which generates a JDK<version>GregorianCalendarAndDurationSerData.java file to save the developer from multiple cut an pastes. At a minimum, there really should be a step by step guide ------------- PR Review: https://git.openjdk.org/jdk/pull/13537#pullrequestreview-1415412426 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186516525 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186471437 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186517342 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186473297 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186518064 PR Review Comment: https://git.openjdk.org/jdk/pull/13537#discussion_r1186522549
