On Thu, 9 Apr 2026 07:57:43 GMT, David Beaumont <[email protected]> wrote:
> Convert and tidy-up unit tests in test/jaxp/javax/xml/jaxp/unittest/stream.
>
> It is recommended to set the diff settings in github to hide whitespace
> (cog-wheel menu).
>
> There are a lot of cases where manual catch blocks with explicit fail are
> removed to just allow the test to fail idiomatically by throwing the
> exception normally. This causes thousands of lines to be un-indented (with no
> other change).
>
> There are also several non-trivial changes in this PR related to badly
> written tests where, for example, catching and ignoring exceptions allows the
> test to pass under all situations.
Some "preloaded" comments explaining non-trivial changes.
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6489502.java line 74:
> 72: s1.next();
> 73: s1.next(); // advance to <TITLE>
> 74: Assert.assertTrue(s1.getLocalName().equals("TITLE"));
A lot of assertions have been rewritten to be idiomatic. Mostly this is handled
via IntelliJ analysis, but not always.
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6509774.java line 43:
> 41:
> 42: @Test
> 43: public void test0() {
This test was obviously cut & pasted with alternating blank lines (making it
much less readable), so I reformatted it to remove unwanted empty lines.
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6688002Test.java line 104:
> 102: }
> 103:
> 104: public static/* synchronized */XMLStreamReader getReader(InputStream
> is) throws Exception {
No need for synchronization on factories.
test/jaxp/javax/xml/jaxp/unittest/stream/CoalesceTest/CoalesceTest.java line 45:
> 43: public class CoalesceTest {
> 44:
> 45: String countryElementContent = "START India CS}}}}}} India END";
As well as making these constants, I could switch to UPPER_SNAKE_CASE. Your
(reviewer) call.
test/jaxp/javax/xml/jaxp/unittest/stream/CoalesceTest/CoalesceTest.java line 74:
> 72: if (eventType == XMLStreamConstants.CHARACTERS) {
> 73: String text = streamReader.getText();
> 74: if (!text.equals(descriptionElementContent)) {
None of the debug output is needed if `assertEquals()` is used.
test/jaxp/javax/xml/jaxp/unittest/stream/CoalesceTest/CoalesceTest.java line 97:
> 95:
> 96: }
> 97: } catch (XMLStreamException ex) {
This is catching and ignoring all exceptions (I assume it was meant to have a
manual "fail" here).
This illustrates the importance of re-reading the tests and not just doing
minimal conversion to JUnit.
test/jaxp/javax/xml/jaxp/unittest/stream/EventsTest/EventFilterSupportTest.java
line 52:
> 50: static final int MAX = 100_000;
> 51:
> 52: public static void main(String[] args)
No need for `main()`.
test/jaxp/javax/xml/jaxp/unittest/stream/EventsTest/EventFilterSupportTest.java
line 64:
> 62: // Here we test it with 4 nested elements.
> 63: @Test
> 64: public static void smokeTest() throws IOException {
JUnit tests can't be static.
test/jaxp/javax/xml/jaxp/unittest/stream/EventsTest/Issue41Test.java line 118:
> 116: */
> 117: @Test
> 118: public void testDTDEvent() {
I could also convert to a new-style text block. Your (reviewer) call.
test/jaxp/javax/xml/jaxp/unittest/stream/FactoryFindTest.java line 66:
> 64:
> 65: Properties props = new Properties();
> 66: String configFile = getSystemProperty("java.home") +
> File.separator + "lib" + File.separator + "stax.properties";
Need to remove dependency on `jaxp.library.JAXPTestUtilities` which does not
compile when using JUnit.
test/jaxp/javax/xml/jaxp/unittest/stream/FactoryFindTest.java line 108:
> 106:
> 107: private static class MyClassLoader extends URLClassLoader {
> 108: boolean wasUsed = false;
Moving mutable state into the test fixture simplifies the test object.
test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line 174:
> 172: }
> 173: }
> 174: Assert.assertTrue(hasns && hasns1);
Could probably be two asserts? Or maybe refactor logic a little (it's not
really clear what's going on here).
test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line 315:
> 313: try {
> 314: XMLInputFactory ifac = XMLInputFactory.newFactory();
> 315: XMLStreamReader reader = ifac.createXMLStreamReader(new
> FileInputStream(new File(getClass().getResource("toys.xml").getFile())));
Redundant `new File(...)` here (and a few other places).
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6613059Test.java
line 75:
> 73: }
> 74:
> 75: void addMenu(XMLEventReader xer, XMLEvent event) throws
> XMLStreamException {
Removed unused parameter.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/EventReaderTest.java
line 53:
> 51: }
> 52: // no more event
> 53: eventReader.nextEvent();
This test didn't assert anything, so I made sure that, at least, the
end-of-iteration was being honored.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventWriterTest/ReaderToWriterTest.java
line 173:
> 171: checkOutput(OUTPUT_FILE);
> 172: } finally {
> 173: Files.deleteIfExists(Path.of(OUTPUT_FILE));
This sort of thing could also be done with `@TempDir` now.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLOutputFactoryTest/Bug6846132Test.java
line 53:
> 51: //
> saxResult.setSystemId("jaxp-ri/unit-test/javax/xml/stream/XMLOutputFactoryTest/cr6846132.xml");
> 52: XMLOutputFactory ofac = XMLOutputFactory.newInstance();
> 53: XMLStreamWriter writer =
> ofac.createXMLStreamWriter(saxResult);
Since the exception is thrown on creation, there's no point having anything
after it.
Tests like this are confusing and difficult to reason about because they can
pass even if where the exception is thrown changes, so I'm not 100% certain
this was always how the code behaved, but the rewritten test captures the
current behavior more explicitly.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLResolverTest/XMLResolverTest.java
line 89:
> 87: object = new
> FileInputStream(getClass().getResource("replace2.txt").getFile());
> 88: } catch (Exception ex) {
> 89: ex.printStackTrace();
Seems wrong to just silently fail. Throwing an exception makes the test less
likely to suffer false-positives.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamExceptionTest/ExceptionTest.java
line 48:
> 46: Exception ex = new IOException("Test XMLStreamException");
> 47: throw new XMLStreamException(ex);
> 48: } catch (XMLStreamException e) {
Throwing and catching the exception is rather pointless, we're just testing its
`toString()` method.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/HasNextTest.java
line 84:
> 82: System.out.println("XMLStreamReader.hasNext() (3): " + hasNext_3);
> 83:
> 84: Assert.assertTrue((hasNext_1 == hasNext_2) && (hasNext_1 ==
> hasNext_3),
Very simplifiable logic.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/HasNextTest.java
line 106:
> 104: }
> 105:
> 106: } catch (XMLStreamException e) {
This test was broken, and never got to any of the catch clauses. The implied
"expected" END_DOCUMENT never existed because the catch inside the loop
prevented these outer catches happening (and there was no "fail" in the main
try body).
I can't be 100% sure what the original intent of this test was, but the new
test captures the real behavior more explicitly.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/Bug6847819Test.java
line 52:
> 50: } catch (XMLStreamException ex) { // good
> 51: System.out.println("Expected failure: '" + ex.getMessage() +
> "' " + "(matching message: '" + msg + "')");
> 52: } catch (Exception ex2) { // ok; iff links to XMLStreamException
This just doesn't happen.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/IssueTracker70.java
line 57:
> 55:
> 56:
> 57: private void testGetAttributeValueWithNs(String nameSpace, String
> attrName, Consumer<String> checker) throws Exception {
No point passing a "checker" if it's the same thing each time.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/SupportDTDTest.java
line 95:
> 93: final int ENTITY_BOTH = 3;
> 94:
> 95: boolean _DTDReturned = false;
I could remove the legacy leading `_` from instance variables. Your (reviewer)
choice.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/CustomImplTest.java
line 50:
> 48: {
> 49: @Override
> 50: public void writeStartElement(String localName)
A result of running the "remove redundant throws" over the files.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/DomUtilTest.java
line 110:
> 108: try {
> 109: return builder.parse(inputStream);
> 110: } catch (SAXException e) {
Complexity with no benefit.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 1:
> 1: /*
A *lot* of removal of redundant catch clauses in this test... sorry!
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 46:
> 44:
> 45: /** debug output? */
> 46: private static final boolean DEBUG = true;
Debug output is extensive and unnecessary for unit tests. I've checked that no
output was also affecting the test itself.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 120:
> 118:
> 119: xmlStreamWriter.writeStartDocument();
> 120: xmlStreamWriter.writeStartElement(new String(""), "localName",
> new String("nsUri"));
It's not clear why the `new String()` was being used here. I could revert it if
it's felt likely to be important.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 606:
> 604:
> 605: String actualOutput =
> endDocumentEmptyDefaultNamespace(xmlStreamWriter);
> 606: assertTrue(actualOutput.equals(EXPECTED_OUTPUT) ||
> actualOutput.equals(EXPECTED_OUTPUT_2), "Expected: " + EXPECTED_OUTPUT + "\n"
> + "Actual: "
I've left this in, but realistically only one or these sub-clauses will ever be
true.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NullUriDetectionTest.java
line 50:
> 48: w.writeStartDocument();
> 49: w.writeStartElement("foo", "bar", "zot");
> 50: w.writeDefaultNamespace(null);
Not actually asserting anything.
-------------
PR Review: https://git.openjdk.org/jdk/pull/30643#pullrequestreview-4080696968
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056355912
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056363075
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056365256
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056380821
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056384717
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056392264
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056449349
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056448006
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056453032
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056461822
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056468514
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056473731
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056481965
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056486907
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056499716
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056509165
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056516715
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056532175
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056535545
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056567655
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056580300
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056584298
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056592466
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056637234
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056656204
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056661621
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056703958
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056679833
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056671863
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056684352
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3056708954
