On Tue, 10 Mar 2026 11:32:12 GMT, David Beaumont <[email protected]> wrote:
> Refactor tests in test/jaxp/javax/xml/jaxp/functional/javax/xml to use junit,
> along with ancillary utilities and a handful of related tests elsewhere.
>
> The difficulty in these refactorings is the use of common utilities which
> themselves depend on TestNG classes, which are not available when running
> JUnit tests. Thus, several bits of functionality in utility classes (esp.
> classes in `jaxp.library`) has had to be re-implemented and inlined. This
> isn't terrible, since most of these were one line functions (or complex
> functions which could be replaced with one line).
>
> Another complexity is accounting for the differences in test lifecycle
> management between TestNG and JUnit. A few classes needed to exploit the
> lifecycle and execution modes for single setup and single threaded operation.
>
> Finally, the move from "expected exceptions" on the whole test method to
> using `assertThrows` threw up a lot of issues where tests weren't testing
> what they claimed (the exception was not being thrown by the final statement
> in the test). For these tests I've simplified them (rewriting them entirely
> in some cases) but they now test what they claim to be testing and are much
> clearer about where assertions are made.
>
> However, because these tests affect the thrown exceptions in test method
> signatures, I ran a tidyup to remove all unused exceptions from method
> signatures (rather than spending hours manually removing the effected subset).
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/FactoryNewInstanceTest.java
line 54:
> 52: private static final String DATATYPE_FACTORY_CLASSNAME =
> DEFAULT_IMPL_CLASS;
> 53:
> 54: public static Object[][] getValidateParameters() {
One of the most common changes is making data provider methods static (required
in JUnit). Most data provider method and many fields or one-line util methods
could always have been static.
I've reformatted a few of these just for my own sanity while trying to reason
about what's actually being provided (sometimes it's just a single variable and
can use `@ValueSource` instead of `@MethodSource`.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/FactoryNewInstanceTest.java
line 73:
> 71: assertSame(dtf1.getClass(), dtf2.getClass(),
> 72: "unexpected class mismatch for newDefaultInstance():");
> 73: assertEquals(DEFAULT_IMPL_CLASS, dtf1.getClass().getName());
I've tries to rely on the IntelliJ lint checker to convert all of these to
(expected, actual). Most of these weren't done by hand.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/FactoryNewInstanceTest.java
line 97:
> 95: */
> 96: @ParameterizedTest
> 97: @JAXPDataProvider.NewInstanceNeg
This new `@JAXPDataProvider.NewInstanceNeg` annotation is sadly very necessary.
Without it, or something similar, you need to use
`@MethodSource("class-name#method")` but that doesn't actually cause the
compiler to bring in the class in question (`JAXPDataProvider`) so it fails at
runtime. Defining a new JUnit data provider annotation means the target class
is in scope at compilation time.
Overall the whole thing of having an opaque shared class for exactly one bit of
trivial test data is a complexity I'd love to remove altogether, but this is
the least invasive thing I can do for now.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/XMLGregorianCalendarTest.java
line 142:
> 140: @ParameterizedTest
> 141: @ValueSource(ints={ -1, 1001 })
> 142: public void checkNewTimeNeg(int invalidMilliseconds) {
Variable name changed because the data source no longer has a name indicating
what these values are.
test/jaxp/javax/xml/jaxp/functional/javax/xml/datatype/ptests/XMLGregorianCalendarTest.java
line 348:
> 346: *
> 347: */
> 348: @Test(expectedExceptions = IllegalStateException.class)
This type of exception testing is an anti-pattern because it's impossible to
know which statement in the method is actually being tested (e.g. the exception
could be from the factory method and not the `toString()` method. In this case,
it was the `toString()` throwing the exception (I personally think this is very
bad behaviour and that toString() should never throw, but that's another story).
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/DBFNamespaceTest.java
line 62:
> 60: public Object[][] getInput() {
> 61: DocumentBuilderFactory dbf1 =
> DocumentBuilderFactory.newInstance();
> 62: String outputfile1 = USER_DIR + "dbfnstest01.out";
`USER_DIR` is just the test working directory, which is also the current
directory when running the tests, so it's fine to just remove these.
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/DBFNamespaceTest.java
line 85:
> 83: Document doc = dbf.newDocumentBuilder().parse(new File(XML_DIR,
> "namespace1.xml"));
> 84: dummyTransform(doc, outputfile);
> 85: assertTrue(compareWithGold(goldfile, outputfile));
compareWithGold() is a util method in JAXPTestUtilities, but that cannot be
compiled for JUnit tests (it used TestNG classes). Luckily JUnit has a
good-enough assert for making this a one-line replacement.
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/DocumentBuilderFactoryTest.java
line 169:
> 167: return new Object[][] {
> 168: { new FileInputStream(new File(XML_DIR, "test.xsd")) },
> 169: { new InputSource(filenameToURL(XML_DIR + "test.xsd")) }
> };
filenameToURL is another JAXPTestUtilities method which cannot be used. The
inline replacement is ... not satisfying, but it's also not awful. Happy to try
and making this neater if people want (making XML_DIR a Path and not a String
would be a start, but that affects a bunch more lines elsewhere).
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/DocumentBuilderFactoryTest.java
line 334:
> 332: db.setErrorHandler(eh);
> 333: Document doc = db.parse(new File(XML_DIR,
> "DocumentBuilderFactory04.xml"));
> 334: assertTrue(doc instanceof Document);
I used IntelliJ linting to simplify all the JUnit assertions automatically. It
does nice things like this.
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/FactoryConfErrorTest.java
line 54:
> 52: @BeforeTest
> 53: public void setup() {
> 54: setSystemProperty("javax.xml.parsers.DocumentBuilderFactory",
> "xx");
These set/clear methods were just a one-liner in the utils file (which can't be
used now).
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/SAXParserTest.java
line 53:
> 51: */
> 52: public class SAXParserTest {
> 53: private static final DefaultHandler DEFAULT_HANDLER = new
> DefaultHandler();
I pulled out these constants for simplicity (it avoid doing work inside the
assertThrows lambda or having lots of extra local variables. It's also nice to
be able to suppress the deprecation warning only once.
test/jaxp/javax/xml/jaxp/functional/javax/xml/parsers/ptests/SAXParserTest.java
line 273:
> 271: try(FileInputStream instream = new FileInputStream(new
> File(XML_DIR,
> 272: "invalid.xml"))) {
> 273: InputSource is = new InputSource(instream);
This is an example where you want to pull out a local variable to avoid doing
more work than you need to in the assertThrows lambda. The lambda should only
call the code-under-test (e.g. if `new InputSource()` threw the exception
instead, the test could pass falsely.
A lot of these test cases were clearly written by cutting & pasting the "good"
version, changing the parameters used and adding the `expectedExceptions`
declaration. This is a bad idea because it's no longer clear what's being
tested. When testing a negative like this, you have to only assert failure for
the one thing you expect to fail.
test/jaxp/javax/xml/jaxp/functional/javax/xml/stream/ptests/XMLInputFactoryNewInstanceTest.java
line 83:
> 81: @MethodSource("getValidateParameters")
> 82: public void testNewFactory(String factoryId, ClassLoader classLoader)
> {
> 83: System.setProperty(XMLINPUT_FACTORY_ID,
> XMLINPUT_FACTORY_CLASSNAME);
Tests use a mix of techniques to handle system properties (try-finally or test
life-cycle methods). I'd be happy to make everything follow the same pattern if
you (my dear reviewer) want.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/SAXTFactoryTest.java
line 152:
> 150: DOMSource domSource = new DOMSource(document);
> 151:
> 152: XMLReader reader = getXmlReader();
Pulled into a one-line util method so I can mark the deprecation warning
suppression in one place.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/TransformTest.java
line 260:
> 258: }
> 259:
> 260: public void startDocument() throws SAXException {
Redundant throws declarations were automatically removed everywhere.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/TransformerExcpTest.java
line 62:
> 60: TransformerFactory tFactory =
> TransformerFactory.newInstance();
> 61: Transformer transformer =
> tFactory.newTransformer(streamSource);
> 62: transformer.transform(
This test was NOT testing what it looked like.
The exception is thrown by `newTransformer()` and not `transform()`.
This is only of about 20 cases where the expectation of a failure was handled
incorrectly.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/URIResolverTest.java
line 76:
> 74: private final static String XSL_TEMP_FILE = "temp/cities.xsl";
> 75:
> 76: /**
This test created its own instances of the test class for secondary purposes,
which was confusing and unnecessary. I refactored it.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/ptests/URIResolverTest.java
line 129:
> 127: try (FileInputStream fis = new
> FileInputStream(XSL_INCLUDE_FILE)) {
> 128: TransformerFactory tfactory =
> TransformerFactory.newInstance();
> 129: URIResolverTest resolver = new
> URIResolverTest(XSL_TEMP_FILE, SYSTEM_ID);
Very confusing to see a test instance being created here.
test/jaxp/javax/xml/jaxp/functional/javax/xml/transform/xmlfiles/out/saxtf008GF.out
line 2:
> 1: <?xml version="1.0" encoding="UTF-8"?><countries>
> 2: <country name="France">
The test using this file wasn't annotated properly and was just never being
run. Having fixed the annotation, I also had to fix the indentation here.
test/jaxp/javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathEvaluationResultTest.java
line 98:
> 96: * DataProvider: Class types not supported
> 97: */
> 98: @DataProvider(name = "unsupportedTypes")
This data provider was never being used.
test/jaxp/javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathExpressionTest.java
line 103:
> 101: }
> 102:
> 103: /**
I factored these 3 tests out and moved to the top. The tests they replace were
incorrectly documented as testing the evaluate() method, and after simplifying,
there were several 100% duplicates, so it seemed better to pull them out at the
top and give them correct descriptions.
However, now I look at this in review, I think these are the exact same tests
as done in XPathTest below, so I'd be happy to just remove them.
test/jaxp/javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathExpressionTest.java
line 234:
> 232: }
> 233:
> 234: /**
This description and test are wrong. Evaluate isn't what throws the exception.
This is just testXPathCompileNullInput(), as is testCheckXPathExpression17
below.
test/jaxp/javax/xml/jaxp/functional/javax/xml/xpath/ptests/XPathTest.java line
145:
> 143: *
> 144: */
> 145: @Test
Now I look at it, I think these are exactly the same tests as in the
XPathExpressionTest class.
In fact, there seems to be a lot of duplication between these two test classes
which could be removed.
test/jaxp/javax/xml/jaxp/libs/javax/xml/transform/ptests/TransformerTestConst.java
line 25:
> 23: package javax.xml.transform.ptests;
> 24:
> 25: import static jaxp.library.JAXPTestUtilities.FILE_SEP;
Must remove dependencies on JAXPTestUtilities, which doesn't compile for JUnit
tests.
test/jaxp/javax/xml/jaxp/libs/jaxp/library/JAXPDataProvider.java line 41:
> 39: * Provide invalid parameters for negative testing Factory.newInstance.
> 40: */
> 41: public class JAXPDataProvider implements ArgumentsProvider,
> AnnotationConsumer<JAXPDataProvider.NewInstanceNeg> {
Not great to have a whole class for such a trivial bit of test data (it
obfuscates the test-case making it harder to reason about). Personally I'd seek
an alternate approach here, but for now it's fine I guess.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911114094
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911118332
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911137705
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911145291
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911169718
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911173877
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911183136
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911194213
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911199136
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911205552
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911215837
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911228709
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911251674
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911259424
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911274857
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911283230
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911290757
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911293713
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911300200
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911307597
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911318843
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911328773
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911346599
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911359142
PR Review Comment: https://git.openjdk.org/jdk/pull/30165#discussion_r2911366576
