On Fri, 13 Mar 2026 12:55:41 GMT, Volkan Yazici <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> tidy up static imports for assertions
>
> test/jaxp/javax/xml/jaxp/functional/catalog/CatalogReferCircularityTest.java
> line 1:
>
>> 1: /*
>
> Copyright year needs to be updated.
>
> Since there are no semantic changes in this file, you can also consider
> reverting your changes here.
I like the consistency of static imports everywhere. I had actually not spotted
this was already a JUnit test, hence the copyright not being caught. I'll
update the year.
> test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/DurationTest.java
> line 201:
>
>> 199: */
>> 200: @ParameterizedTest
>> 201: @MethodSource("getEqualDurations")
>
> Since you've already done similar changes, you can consider inlining this
> using `@CsvSource`.
>
> There are more places you can exercise `@CsvSource`, I won't mention them all.
I've used `@CvsSource` a bit before and dislike it compared to `@MethodSource`.
The need for escaping and other things just gets in the way, even if only
occasionally. There are literally hundreds of places I could use `@CvsSource`
and I don't fancy manually converting all of them.
> test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/DurationTest.java
> line 477:
>
>> 475: int hash1 = duration1.hashCode();
>> 476: int hash2 = duration2.hashCode();
>> 477: assertEquals(hash1, hash2, " generated hash1 : " + hash1 + "
>> generated hash2 : " + hash2);
>
> Since you've switched from `assertTrue` to `assertEquals`, I doubt if the
> message carries any extra value anymore.
True, but I don't think it's doing any harm.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2932675840
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2932673492
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2932663635
