On Thu, 9 Apr 2026 10:24:23 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).
>>
>> About 200 explicit uses of `fail()` were either removed or converted to
>> appropriate JUnit asserts.
>>
>> There are also several non-trivial changes in this PR related to bad tests
>> where, for example, catching and ignoring exceptions allowed the test to
>> pass under all situations.
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> one more assertion simplification
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6489502.java line 44:
> 42: public class Bug6489502 {
> 43:
> 44: public java.io.File input;
Unused field?
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6489502.java line 52:
> 50: public void testEventReader1() throws Exception {
> 51: // Check if event reader returns the correct event
> 52: XMLEventReader e1 =
> inputFactory.createXMLEventReader(inputFactory.createXMLStreamReader(new
> java.io.StringReader(XML)));
Use import for `java.io.StringReader`?
(also affects other occurrences below)
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6509774.java line 81:
> 79:
> 80: // Should not be a DTD event since they are ignored
> 81: assertEquals(XMLStreamConstants.DTD, event);
The comment here is (and was) wrong? It says "should not be a DTD event" but
the assertion here verifies that it is one (or am I misunderstanding it?)?
Or is this "should not" and not "must not"?
(Probably out of scope for this PR though)
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6509774.java line 105:
> 103:
> 104: // Should not be a DTD event since they are ignored
> 105: assertEquals(XMLStreamConstants.DTD, event);
Same as above, is "Should not be a DTD event" wrong?
test/jaxp/javax/xml/jaxp/unittest/stream/Bug6688002Test.java line 98:
> 96: } catch (Exception e) {
> 97: fail(e.getMessage());
> 98: }
Should probably catch `Throwable` instead of `Exception`, and store it in an
`AtomicReference` and then retrieve it after `join()`?
Currently the exception might just terminate the thread, but the test method
will not notice it?
test/jaxp/javax/xml/jaxp/unittest/stream/EventReaderDelegateTest.java line 103:
> 101: if (event.isEndElement() || event.isStartElement()) {
> 102: XMLEvent nextevent = delegate.nextTag();
> 103: assertTrue(nextevent.getEventType() ==
> XMLStreamConstants.START_ELEMENT || nextevent.getEventType() ==
> XMLStreamConstants.END_ELEMENT);
Split this?
- `if (event.isStartElement())`
... `assertEquals(XMLStreamConstants.START_ELEMENT,
delegate.nextTag().getEventType())`
- `if (event.isEndElement())`
... `assertEquals(XMLStreamConstants.END_ELEMENT,
delegate.nextTag().getEventType())`
test/jaxp/javax/xml/jaxp/unittest/stream/FactoryFindTest.java line 53:
> 51:
> 52: @Test
> 53: @Disabled // due to 8156508
Could alternatively use `Disabled#value` for this:
Suggestion:
@Disabled("due to 8156508")
test/jaxp/javax/xml/jaxp/unittest/stream/FactoryFindTest.java line 72:
> 70: props.store(fos, null);
> 71: }
> 72: f.deleteOnExit();
How does the test setup for the JDK work, is it fine to just delete files from
`System.getProperty("java.home")` here? (and that the test wrote there in the
first place)
test/jaxp/javax/xml/jaxp/unittest/stream/FactoryFindTest.java line 91:
> 89: Thread.currentThread().setContextClassLoader(clInput);
> 90: XMLInputFactory.newInstance();
> 91: // because it's decided by having sm or not in FactoryFind code
Comment is now obsolete as well, because `System.getSecurityManager()` check
was removed?
Suggestion:
test/jaxp/javax/xml/jaxp/unittest/stream/IgnoreExternalDTDTest.java line 42:
> 40: public class IgnoreExternalDTDTest {
> 41:
> 42: final static String FACTORY_KEY = "javax.xml.stream.XMLInputFactory";
Change this to `static final` instead of `final static` for consistency with
the other constants here?
test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line 73:
> 71: if (delegate.getLocalName().equals("foo")) {
> 72: assertEquals(5, delegate.getAttributeCount());
> 73: assertSame("CDATA", delegate.getAttributeType(1));
Maybe `assertSame("CDATA", ...)` is actually too strict here; does it really
have to be the same exact string object?
Though it matches the previous assertion behavior.
test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line 88:
> 86: assertEquals("http://ns1.java.com";,
> delegate.getAttributeNamespace(1));
> 87: assertEquals("ns1",
> delegate.getAttributePrefix(1));
> 88:
> assertEquals(delegate.getAttributeName(1).toString(), "{" +
> delegate.getAttributeNamespace(1) + "}" + delegate.getAttributeLocalName(1));
Switch arguments here? Is `delegate.getAttributeName(1).toString()` rather the
'actual' value?
test/jaxp/javax/xml/jaxp/unittest/stream/StreamReaderDelegateTest.java line 246:
> 244: if (delegate.getEventType() ==
> XMLStreamConstants.START_ELEMENT) {
> 245: if (delegate.getLocalName().equals("name") ||
> delegate.getLocalName().equals("price")) {
> 246: System.out.println(delegate.getElementText());
Replace `println` with `assertNotNull`, if that was the intention? (or add it
in addition?)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/Issue47Test.java
line 50:
> 48: XMLInputFactory xif = XMLInputFactory.newInstance();
> 49: XMLStreamReader r = xif.createXMLStreamReader(new
> StringReader(xml));
> 50: assertTrue(!r.standaloneSet() && !r.isStandalone());
Split into two asserts (`assertFalse`)? (applies also to the other test methods)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/IssueTracker70.java
line 64:
> 62: if (xsr.isStartElement()) {
> 63: String v;
> 64: v = xsr.getAttributeValue(nameSpace, attrName);
Move assignment to declaration?
String v = xsr.getAttributeValue(nameSpace, attrName);
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/NamespaceTest.java
line 68:
> 66: XMLInputFactory xif = XMLInputFactory.newInstance();
> 67: xif.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, Boolean.TRUE);
> 68: InputStream is = new
> java.io.ByteArrayInputStream(getXML().getBytes());
Add import for `java.io.ByteArrayInputStream`? (also for the other test methods
below)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/SupportDTDTest.java
line 202:
> 200: public void supportDTD(boolean supportDTD, boolean replaceEntity,
> int inputType) throws Exception {
> 201: print("\n");
> 202: print((supportDTD ? "SupportDTD=true" : "SupportDTD=false") + ",
> " + (replaceEntity ? "replaceEntity=true" : "replaceEntity=false"));
Can simplify this?
Suggestion:
print("SupportDTD=" + supportDTD + ", replaceEntity=" + replaceEntity);
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/Bug6600882Test.java
line 50:
> 48: XMLStreamWriter w1 = of.createXMLStreamWriter(new
> ByteArrayOutputStream());
> 49: assertTrue(w.equals(w) && w.hashCode() == w.hashCode());
> 50: assertNotEquals(w1, w);
For consistency explicitly check `assertFalse(w1.equals(w))` here, since this
test is about the `equals` implementation?
JUnit is not intended for checking `equals` implementations, and might take
shortcuts.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/Bug6675332Test.java
line 56:
> 54: final String EXPECTED_OUTPUT = "<?xml version=\"1.0\"
> encoding=\"UTF-8\"?><root></root>";
> 55: XMLStreamWriter w = null;
> 56: StringWriter strw = new StringWriter();
Directly assign `XMLStreamWriter w` here (and reorder `w` and `strw`)?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/CustomImplTest.java
line 42:
> 40:
> 41: @Test
> 42: public void testEventReader() throws Exception {
Wrong test name?
<code>testEvent<del>Reader</del><ins>Writer</ins></code>
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/DomUtilTest.java
line 58:
> 56: @Test
> 57: public void testSOAPEnvelope1() throws Exception {
> 58: setup();
Remove explicit `setup()` call and convert to `@BeforeEach` / `@BeforeAll`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/EmptyElementTest.java
line 48:
> 46: XMLStreamWriter xmlStreamWriter;
> 47: ByteArrayOutputStream byteArrayOutputStream;
> 48: XMLOutputFactory xmlOutputFactory;
Can be local variables?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/EmptyElementTest.java
line 51:
> 49:
> 50: @Test
> 51: public void testWriterOnLinux() throws Exception {
There is nothing specific to Linux? (copy & paste error from
`SqeLinuxTest.java`?)
Suggestion:
public void testWriter() throws Exception {
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/EncodingTest.java
line 57:
> 55: final String EXPECTED_OUTPUT = "<?xml version=\"1.0\"
> encoding=\"UTF-8\"?><root></root>";
> 56: XMLStreamWriter writer = null;
> 57: ByteArrayOutputStream byteArrayOutputStream = null;
Move variable declaration to assignment below?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 1029:
> 1027: @Test
> 1028: public void testDoubleXmlNsNoRepair() throws XMLStreamException {
> 1029: // reset to known state
This comment is misleading (and also does not occur for any other test methods)
because the test method is just starting?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NamespaceTest.java
line 1237:
> 1235: String actualOutput =
> endDocumentEmptyDefaultNamespace(xmlStreamWriter);
> 1236: System.out.println("actualOutput: " + actualOutput);
> 1237: Assert.fail("XMLStreamException is expected as 'p' is
> rebinded to a different URI in same namespace context");
Preserve this explanation message as comment?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/NullUriDetectionTest.java
line 51:
> 49: w.writeStartElement("foo", "bar", "zot");
> 50: w.writeDefaultNamespace(null);
> 51: w.writeCharacters("---");
Should this `writeCharacters` call be preserved (also as `assertDoesNotThrow`)?
Maybe the intention was to prove that the writer is not in a broken state and
further write calls work too?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SqeLinuxTest.java
line 49:
> 47: XMLStreamWriter xmlStreamWriter;
> 48: ByteArrayOutputStream byteArrayOutputStream;
> 49: XMLOutputFactory xmlOutputFactory;
Convert to local variables?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/SurrogatesTest.java
line 63:
> 61: }
> 62:
> 63: // Test that unbalanced surrogate character will
Suggestion:
// Test that unbalanced surrogate character will throw
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/UnprefixedNameTest.java
line 52:
> 50: assertThrows(XMLStreamException.class, () ->
> w.writeStartElement("foo", "bar"),
> 51: "Expected: XMLStreamException if the namespace URI has
> not been bound to a prefix "
> 52: + "and javax.xml.stream.isPrefixDefaulting has
> not been " + "set to true");
Suggestion:
+ "and javax.xml.stream.isPrefixDefaulting has not been
set to true");
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/UnprefixedNameTest.java
line 88:
> 86:
> 87: // Expected success
> 88: System.out.println("Expected success.");
Remove this too?
Suggestion:
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/WriterTest.java
line 303:
> 301: }
> 302:
> 303: // Test StreamWriter supplied with correct namespace information
> and" + "isRepairingNamespace is set to true.
Suggestion:
// Test StreamWriter supplied with correct namespace information and
isRepairingNamespace is set to true.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamWriterTest/WriterTest.java
line 603:
> 601: }
> 602:
> 603: protected boolean compareOutput(Reader expected, Reader actual)
> throws IOException {
Similar to https://github.com/openjdk/jdk/pull/30643#discussion_r3064505023 and
https://github.com/openjdk/jdk/pull/30643#discussion_r3064534183, refactor this
method here?
test/jaxp/javax/xml/jaxp/unittest/util/BaseStAXUT.java line 138:
> 136: try {
> 137: byte[] data = content.getBytes("UTF-8");
> 138: return constructStreamReader(f, data);
Avoid try-catch by using `StandardCharsets`?
test/jaxp/javax/xml/jaxp/unittest/util/BaseStAXUT.java line 309:
> 307: String text2 = new String(textChars, start, expLen);
> 308: assertEquals("Expected getText() and getTextCharacters() to
> return same value for event of type (" + tokenTypeDesc(sr.getEventType()) +
> ")",
> 309: text, text2);
Message comes last, currently it is the 'expected' argument.
But how has this assertion passed then before? Is it even reachable?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065854651
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065857935
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066055435
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066050621
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066086784
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066150148
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066168113
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066200165
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066190501
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066219723
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066275235
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066239776
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066283960
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065120102
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065135938
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065184980
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065215185
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065247972
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065260267
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065284834
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065291681
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065302565
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065309356
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065318959
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065391299
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065359316
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065397944
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065405151
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065412600
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065428962
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065431451
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065454988
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065485874
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066341131
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3066319045
