On Tue, 17 Mar 2026 18:13:00 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:
>
> Unifying imports
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogFileInputTest.java line 81:
> 79: static final CatalogFeatures FEATURES = CatalogFeatures.builder().
> 80: with(CatalogFeatures.Feature.PREFER, "system").build();
> 81: static String CLS_DIR = getSystemProperty("test.classes");
Inlined to avoid dependency on JAXPTestUtilities which cannot be compiled for
JUnit tests.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogFileInputTest.java line 157:
> 155: CatalogFeatures features = CatalogFeatures.builder()
> 156: .with(CatalogFeatures.Feature.FILES, file)
> 157: .build();
Wrong test expections: The build() method isn't what throws the exception.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogFileInputTest.java line 255:
> 253:
> 254: return new Object[][]{
> 255: {""},
Some of these weren't triggering exceptions in the code-under-test, but rather
the setup (`new URI()` and `new File()`) so they were removed, because that's
not what these tests are for.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogSupport.java line 84:
> 82: */
> 83: @ParameterizedTest
> 84: @MethodSource("getDataSAX")
Note: The matching of names to data providers is done via search & replace, so
should be completely reliable.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogSupport2.java line 103:
> 101: public void testSAXC(boolean setUseCatalog, boolean useCatalog,
> String catalog, String
> 102: xml, MyHandler handler, String expected) throws Exception {
> 103: assertThrows(
I'm not a fan of this pattern, calling *other test code* for an
`assertThrows()`, since you really want to be asserting exactly on the
code-under-test which will actually throw. However, there are a lot of these
and refactoring them properly would be a lot of changes.
test/jaxp/javax/xml/jaxp/unittest/catalog/CatalogSupportBase.java line 102:
> 100: protected void setUp() {
> 101: String file1 =
> getClass().getResource("CatalogSupport.xml").getFile();
> 102: if (getSystemProperty("os.name").contains("Windows")) {
This is a really complex and messy setup, which creates non-obvious code in
test subclasses.
I'd be happy to see if I can sort it out properly if you (my dear reviewer) are
up for it.
test/jaxp/javax/xml/jaxp/unittest/datatype/XMLGregorianCalendarTest.java line
98:
> 96:
> 97: if (DEBUG) {
> 98: System.err.println("XMLGregorianCalendar created: \""
> + xmlGregorianCalendar.toString() + "\"");
Tidying some redundant `toString()` calls.
test/jaxp/javax/xml/jaxp/unittest/datatype/XMLGregorianCalendarTest.java line
205:
> 203: @Test
> 204: public void testEqualsWithEqualObjectParam() {
> 205:
There's absolutely no need to catch `DatatypeConfigurationException` for the
`DatatypeFactory.newInstance()` call and that can easily be moved into a test
fixture, since the factory is immutable and thread safe. There are quite a lot
of cases where this occurs and the try/catch can just be removed.
test/jaxp/javax/xml/jaxp/unittest/sax/Bug6992561Test.java line 76:
> 74: reader.parse(is);
> 75:
> 76: } catch (ParserConfigurationException ex) {
No need to manually handle unexpected failures.
test/jaxp/javax/xml/jaxp/unittest/sax/DefaultHandler2Test.java line 52:
> 50: DefaultHandler handler = new MyDefaultHandler2();
> 51: SAXParserFactory saxFac = SAXParserFactory.newInstance();
> 52:
> System.out.println(saxFac.getFeature("http://xml.org/sax/features/use-locator2";));
While the println is likely unwanted, it's not 100% clear if the call to
`getFeature()` is part of the test, so for examples like this, I left them in.
I'd be happy to see about removing them if desired.
test/jaxp/javax/xml/jaxp/unittest/sax/IssueTracker56Test.java line 118:
> 116: public class MyHandler1 extends DefaultHandler implements
> ErrorHandler {
> 117:
> 118: public void startDocument() throws SAXException {
Empty methods are exactly what the default handler provides, so no need to
repeat them here.
test/jaxp/javax/xml/jaxp/unittest/sax/XMLReaderTest.java line 46:
> 44: * @summary This class contains tests that cover the creation of
> XMLReader.
> 45: */
> 46: @Execution(ExecutionMode.SAME_THREAD)
Same thread since the property is being set/cleared in a test, so we need to
avoid possible issues with interleaving.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948454675
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948461678
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948471075
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948477708
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948489582
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948503160
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948513583
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948530511
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948537473
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948546109
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948552645
PR Review Comment: https://git.openjdk.org/jdk/pull/30283#discussion_r2948563352
