On Mon, 23 Mar 2026 19:23:33 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:
>
> Be consistent with static imports
There's a few catch that just does `fail(xxx.getMessage())`. These can be
converted to `assertDoesNotThrow`.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogFileInputTest.java line 196:
> 194:
> 195: @Test
> 196: public void testNullUri1() {
Looking at this test name, I think we should merge this into testNullUri since
JUnit can assertThrows multiple times in one method.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogReuseTest.java line 78:
> 76: Data columns: uri, expected
> 77: */
> 78: public static Object[][] dataWithoutCatalog() {
Since this test must be per-instance (inheritance-based setup) I know both
static or instance method source is ok. However this introduces a mix of static
and instance and feels a little bit inconsistent...
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogSupportBase.java line 444:
> 442: transformer.transform(xml, new StreamResult(out));
> 443: debugPrint("out:\n" + out.toString());
> 444: assertTrue(out.toString().contains(expected), "testXSLImport");
Old message was not helpful (already included in stack trace, no debug print of
expected) but we can leave it for now. Same below.
test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 140:
> 138:
> 139: public static Object[][] lexicalvalues() {
> 140: Object actualAndExpected[][] = {
Suggestion:
Object[][] actualAndExpected = {
Or just use one return statement.
test/jaxp/javax/xml/jaxp/unittest/datatype/DurationTest.java line 126:
> 124: Duration smallDur = factory.newDuration(10000);
> 125: if (smallDur.subtract(bigDur).getSign() != -1) {
> 126: fail("smallDur.subtract(bigDur).getSign() is not -1");
These can be converted to `assertEquals` without message (already available
from the default context) if you want to.
test/jaxp/javax/xml/jaxp/unittest/datatype/FactoryFindTest.java line 61:
> 59: assertTrue(myClassLoaderUsed);
> 60: else
> 61: assertFalse(myClassLoaderUsed);
Can remove this branch as security manager is gone
test/jaxp/javax/xml/jaxp/unittest/sax/DefaultHandler2Test.java line 102:
> 100: ParserAdapter adapter = new ParserAdapter(parser.getParser());
> 101: assertThrows(SAXNotRecognizedException.class, () ->
> adapter.getProperty("http://xml.org/sax/properties/declaration-handler";));
> 102: assertThrows(SAXNotRecognizedException.class, () ->
> adapter.setProperty("http://xml.org/sax/properties/declaration-handler";,
> handler));
Old code used new adapters, guess it's ok if this test passes.
test/jaxp/javax/xml/jaxp/unittest/sax/NSSupportTest.java line 143:
> 141: }
> 142: }
> 143: assertTrue(hasdcnew && hasdc1 && hasdc2);
Ideally we would break these into multiple lines of assertTrue for easier
debugging, but there are so many so let's just leave them be.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30283#pullrequestreview-4009074345
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2990338670
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2990354250
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2990445398
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2990469560
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2990502566
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r3010963371
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r3010984480
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r3011089647
