On Wed, 18 Mar 2026 11:52:01 GMT, David Beaumont <[email protected]> wrote:
>> Similar to other recent JAXP test migration PRs, but simpler in most cases.
>>
>> Edge cases include:
>> * Lots of tests needing the PER_CLASS life-cycle due to having a base class
>> with non-trivial test setup
>> * Tests with incorrect expectations about thrown exceptions
>> * Tests needlessly catching test framework exceptions when they should be
>> left to fail normally
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove single threading annotation (it's the default)
Hopefully these comments are useful. Note that I am not an OpenJDK member so
please feel free to consider the comments at most as suggestions.
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.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/TransformTest.java
line 30:
> 28: import org.junit.jupiter.api.TestInstance.Lifecycle;
> 29: import org.junit.jupiter.api.parallel.Execution;
> 30: import org.junit.jupiter.api.parallel.ExecutionMode;
These imports for `Execution` and `ExecutionMode` are now unused.
test/jaxp/javax/xml/jaxp/functional/javax/xml/validation/ptests/SchemaFactoryTest.java
line 31:
> 29: import org.junit.jupiter.api.TestInstance.Lifecycle;
> 30: import org.junit.jupiter.api.parallel.Execution;
> 31: import org.junit.jupiter.api.parallel.ExecutionMode;
These imports for `Execution` and `ExecutionMode` are now unused.
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.
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`)
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
test/jaxp/javax/xml/jaxp/unittest/datatype/Bug7042647Test.java line 56:
> 54: } else {
> 55: System.out.println("Success firstDayOfWeek=" + firstDayOfWeek
> + " == defaultFirstDayOfWeek=" + defaultFirstDayOfWeek);
> 56: }
Could use `assertEquals`?
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.
test/jaxp/javax/xml/jaxp/unittest/sax/Attributes2ImplTest.java line 84:
> 82: } catch (ArrayIndexOutOfBoundsException e) {
> 83: System.out.println("Expected ArrayIndexOutOfBoundsException
> on index=2 after removing");
> 84: }
Should these all use `assertThrows` to actually fail when the expected
exception was not thrown?
test/jaxp/javax/xml/jaxp/unittest/sax/Attributes2ImplTest.java line 130:
> 128: } catch (ArrayIndexOutOfBoundsException e) {
> 129: System.out.println("Expected ArrayIndexOutOfBoundsException
> on index=2 after removing");
> 130: }
Use `assertThrows`?
test/jaxp/javax/xml/jaxp/unittest/sax/Bug6889654Test.java line 53:
> 51: try {
> 52: parse();
> 53: } catch (SAXException e) {
Should use `assertThrows`? E.g.:
String message = assertThrows(SAXException.class, () -> ...).getMessage();
...
test/jaxp/javax/xml/jaxp/unittest/sax/Bug6889654Test.java line 82:
> 80: } catch (IOException ioe) {
> 81:
> 82: }
Remove these empty `catch` and change method to `throws Exception`?
test/jaxp/javax/xml/jaxp/unittest/sax/Bug6949607Test.java line 51:
> 49: @Test
> 50: public void testException() {
> 51: try {
Should use `assertThrows`?
test/jaxp/javax/xml/jaxp/unittest/sax/Bug7057778Test.java line 66:
> 64: File src = new File(getClass().getResource(xml).getFile());
> 65: File dst = new File(xml1);
> 66: try {
Assuming this test not throw an exception, remove the complete `try` and add
`throws Exception` instead?
test/jaxp/javax/xml/jaxp/unittest/sax/DefaultHandler2Test.java line 99:
> 97: // ParserAdapter.setProperty() and ParserAdapter.getProperty()
> does
> 98: // not support any property currently.
> 99: try {
Should these use `assertThrows`?
test/jaxp/javax/xml/jaxp/unittest/sax/SAXParserTest.java line 61:
> 59: SAXParserFactory factory = SAXParserFactory.newDefaultInstance();
> 60: SAXParser parser = factory.newSAXParser();
> 61: try {
Should use `assertThrows`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30283#pullrequestreview-3974795194
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959748049
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959783033
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959770946
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959943146
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959922432
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959948372
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2959965595
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960018306
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960064954
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960067947
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960079228
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960084277
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960094311
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960109725
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960143818
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2960175763
