On Thu, 19 Mar 2026 12:33:50 GMT, Marcono1234 <[email protected]> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove single threading annotation (it's the default)
>
> test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/FactoryConfErrorTest.java
> line 47:
>
>> 45: * @run junit/othervm javax.xml.parsers.ptests.FactoryConfErrorTest
>> 46: */
>> 47: public class FactoryConfErrorTest {
>
> The imports for `Execution` and `ExecutionMode` are now unused.
Oh thanks for spotting. I forgot I needed to run organize-imports again.
> test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937951Test.java line 48:
>
>> 46: XMLGregorianCalendar c2 =
>> dtf.newXMLGregorianCalendar("2000-01-01T00:00:00");
>> 47: System.out.println("c1: " + c1.getYear() + "-" + c1.getMonth() +
>> "-" + c1.getDay() + "T" + c1.getHour());
>> 48: System.out.println(c1.equals(c2) ? "pass" : "fail"); // fails
>
> Maybe out of scope, but should this `System.out.println(...); // fails` be
> removed?
> It originates from the reproducer in the bug report, but since that bug was
> fixed this does not actually fail anymore, and therefore this is misleading.
I agree completely, but if I were to start doing some of the these changes I'd
be here all month.
These specific ones look egregious though.
> test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937951Test.java line 52:
>
>> 50: Assertions.fail("hour 24 needs to be treated as equal to
>> hour 0 of the next day");
>> 51: if (c1.getYear() != 2000 && c1.getHour() != 0)
>> 52: Assertions.fail("hour 24 needs to be treated as equal to
>> hour 0 of the next day");
>
> Should these be changed to `assertEquals`?
> (unless the first check explicitly wants to test the `equals` implementation,
> and avoid shortcuts performed by `assertEquals`)
Yeah probably. I looked at these (and there are a fair few like this) and
didn't want to get too far into just "rewrite the test" territory. I'll have a
closer look at how much extra change this would incur and see.
> test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 63:
>
>> 61: DatatypeFactory dtf = DatatypeFactory.newInstance();
>> 62: Duration d = dtf.newDurationYearMonth("P20Y15M");
>> 63: System.out.println(d.getYears() == 21 ? "pass" : "fail");
>
> This does not perform any assertion
Good point. That's worth fixing.
> test/jaxp/javax/xml/jaxp/unittest/datatype/DurationTest.java line 422:
>
>> 420: }
>> 421:
>> 422: private void durationAssertEquals(Duration duration, QName
>> xmlSchemaType, boolean isPositive, int years, int months, int days, int
>> hours, int minutes,
>
> Possibly out-of-scope, but this method here does not actually perform any
> real assertions; it just prints to console.
Yeah, some of these tests are like this. I'll have a look at these specifically
and might just remove them.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2975122836
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2975140153
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2975131667
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2975147786
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2975152655
