RFR: JDK-8077371: Binary files in JAXP test should be removed [v6]

[Lance Andersen]

On Wed, 3 May 2023 14:12:33 GMT, Mahendra Chhipa <mchhipa at 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