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
Hopefully these comments are useful, if not please let me know. They are only
suggestions; feel free to ignore them, I am not an OpenJDK member.
(This is the first half of the modified files; I will probably have a look at
the other half afterwards.)
test/jaxp/javax/xml/jaxp/unittest/stream/AttributeLocalNameTest/AttributeLocalNameTest.java
line 38:
> 36: * @test
> 37: * @library /javax/xml/jaxp/unittest
> 38: * @run junit/othervm stream.AttributeLocalNameTest.AttributeLocalNameTest
In other JUnit PRs it was suggested to use `${test.main.class}` for `@run` to
avoid repetition, would that work here (for all files) as well?
Suggestion:
* @run junit/othervm ${test.main.class}
test/jaxp/javax/xml/jaxp/unittest/stream/EntitiesTest/EntityTest.java line 149:
> 147: LineNumberReader actualOutput = new LineNumberReader(actual);
> 148:
> 149: while (expectedOutput.ready() && actualOutput.ready()) {
These `ready()` checks seem problematic:
- if `actualOutput.ready()` is false, the test passes if there are less lines
than expected
- depending on the type of the given input stream, could `ready()` return false
(even for `expected`) so no comparison is performed at all?
Maybe it would be better to just have a `while (true)` loop, which breaks once
`expectedOutput.readLine() == null && actualOutput.readLine() == null`?
test/jaxp/javax/xml/jaxp/unittest/stream/EntitiesTest/EntityTest.java line 157:
> 155: System.out.println("Actual : " + actualLine);
> 156: return false;
> 157: }
Would it be more convenient to directly use `assertEquals(expectedLine,
actualLine, "in line " + expectedOutput.getLineNumber())` or similar here?
Or maybe even do an `assertEquals(expected.readAllLines(),
actual.readAllLines())`, unless that leads to too verbose assertions errors or
behaves differently than `LineNumberReader`.
test/jaxp/javax/xml/jaxp/unittest/stream/EventsTest/Issue48Test.java line 112:
> 110: Assert.assertTrue(expType.isAssignableFrom(o.getClass()));
> 111: }
> 112: }
Removing this made the test slightly less strict because this here also
included a `!list.contains(null)` check which is now not performed anymore?
test/jaxp/javax/xml/jaxp/unittest/stream/EventsTest/Issue58Test.java line 66:
> 64: Location loc2 = evt.getLocation();
> 65: System.out.println("Location 2: " + loc2.getLineNumber() +
> "," + loc2.getColumnNumber());
> 66: evt = er.nextEvent(); // root
This `nextEvent() // root` call is gone now it seems. Maybe that was relevant
before, to ensure `loc1` and `loc2` were not affected by it?
(When re-adding it, maybe add a comment to explain this?)
test/jaxp/javax/xml/jaxp/unittest/stream/ProcessingInstructionTest/ProcessingInstructionTest.java
line 57:
> 55: String target = sr.getPITarget();
> 56: String data = sr.getPIData();
> 57: assertTrue(target.equals(PITarget) &&
> data.equals(PIData));
Convert to two separate `assertEquals`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6586466Test.java
line 57:
> 55:
> 56: String text = xmlReader.getElementText();
> 57: System.out.println(text);
Can some of these `println` be converted to assertions?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6668115Test.java
line 47:
> 45: public final String filesDir = "./";
> 46: protected XMLInputFactory inputFactory;
> 47: protected XMLOutputFactory outputFactory;
Are these fields actually needed?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6668115Test.java
line 81:
> 79: input = new File(getClass().getResource("play2.xml").getFile());
> 80: // Check if event reader returns the correct event
> 81: return
> inputFactory.createXMLEventReader(inputFactory.createXMLStreamReader(new
> java.io.FileInputStream(input), "UTF-8"));
Use import for `java.io.FileInputStream`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Issue40Test.java
line 48:
> 46: public final String filesDir = "./";
> 47: protected XMLInputFactory inputFactory;
> 48: protected XMLOutputFactory outputFactory;
Can remove some of these fields?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Issue40Test.java
line 62:
> 60: Assert.assertEquals(er.nextEvent().getEventType(),
> XMLStreamConstants.START_ELEMENT);
> 61: Assert.assertEquals(er.nextEvent().getEventType(),
> XMLStreamConstants.START_ELEMENT);
> 62: System.out.println(er.getElementText());
Maybe the `getElementText()` call should be preserved (as
`assertNotNull(...)`?) because the test description explicitly mentions that it
is being tested.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Issue40Test.java
line 83:
> 81: }
> 82: Assert.assertEquals(e.getEventType(),
> XMLStreamConstants.START_ELEMENT);
> 83: System.out.println(er.getElementText());
Maybe the `getElementText()` call should be preserved (as
`assertNotNull(...)`?) because the test description explicitly mentions that it
is being tested.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Issue40Test.java
line 84:
> 82: // Check if event reader returns the correct event
> 83: return inputFactory.createXMLEventReader(
> 84: inputFactory.createXMLStreamReader(new
> java.io.FileInputStream(input), "UTF-8"));
Can add import for `java.io.FileInputStream`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLEventWriterTest/ReaderToWriterTest.java
line 101:
> 99:
> 100: // is output as expected?
> 101: assertEquals(EXPECTED_OUTPUT, actualOutput);
Is this intentionally stricter now than before? (previously it also allowed
`EXPECTED_OUTPUT_NO_ENCODING`.
test/jaxp/javax/xml/jaxp/unittest/stream/XMLInputFactoryTest/Bug6756677Test.java
line 65:
> 63: String XMLInputFactoryClassName =
> "com.sun.xml.internal.stream.XMLInputFactoryImpl";
> 64: String XMLInputFactoryID = "javax.xml.stream.XMLInputFactory";
> 65: ClassLoader CL = null;
This `CL` seems to be only used by one test method, might be better to move it
there as local variable?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLInputFactoryTest/IssueTracker38.java
line 71:
> 69: private void createStreamReaderFromSource(Source source) throws
> Exception {
> 70: XMLInputFactory xIF = XMLInputFactory.newInstance();
> 71: assertNotNull(xIF.createXMLStreamReader(source));
These `assertNotNull` are unreachable because the expected
`UnsupportedOperationException` already occurs before?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLOutputFactoryTest/DuplicateNSDeclarationTest.java
line 72:
> 70: System.out.println("expected: \"" + EXPECTED_OUTPUT + "\"");
> 71:
> 72: // are results as expected?
Redundant `println`; `assertEquals` will contain this information?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLOutputFactoryTest/StreamResultTest.java
line 103:
> 101: writer.close();
> 102:
> 103: assertEquals(buffer.toString(), EXPECTED_OUTPUT);
'expected' comes first for JUnit
Suggestion:
assertEquals(EXPECTED_OUTPUT, buffer.toString());
test/jaxp/javax/xml/jaxp/unittest/stream/XMLOutputFactoryTest/StreamResultTest.java
line 127:
> 125: writer.close();
> 126:
> 127: assertEquals(buffer.toString(), EXPECTED_OUTPUT);
Suggestion:
assertEquals(EXPECTED_OUTPUT, buffer.toString());
test/jaxp/javax/xml/jaxp/unittest/stream/XMLResolverTest/XMLResolverTest.java
line 60:
> 58: if (eventType == XMLStreamConstants.CHARACTERS) {
> 59: String text = streamReader.getText();
> 60: assertTrue(text.contains("replace2"));
Maybe for robustness set a `boolean foundText = true` here, and check
`assertTrue(foundText)` after the loop?
(This is actually a problem for multiple of the test files; not sure if it is
worth changing all of them.)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/Bug6481615.java
line 53:
> 51: reader.next(); // advance to START_ELEMENT
> 52: XMLStreamReader filter = factory.createFilteredReader(reader, new
> Filter());
> 53: assertTrue(filter.getEventType() != -1);
Could use `assertNotEquals`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/Bug6481678.java
line 61:
> 59: InputStream is;
> 60:
> 61: public Bug6481678() {
For consistency with other tests, do this in a `@BeforeEach` instead?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/Bug6481678.java
line 105:
> 103: @Test
> 104: public void testReadingNamespace() throws Exception {
> 105: is = new java.io.ByteArrayInputStream(getXML().getBytes());
Add import for `java.io.ByteArrayInputStream`? (applies to the other test
methods here as well)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/Bug6481678.java
line 113:
> 111: if (sr.getLocalName().equals(rootElement)) {
> 112: assertEquals(prefixApple, sr.getNamespacePrefix(0));
> 113: assertEquals(namespaceURIApple,
> sr.getNamespaceURI(0));
For robustness set a `foundElement = true` and check `assertTrue(foundElement)`
after the loop?
(applies to the other test methods here as well)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/HasNextTest.java
line 71:
> 69: private boolean checkHasNext(XMLStreamReader r1) {
> 70: // try asking 3 times, insure all results are the same
> 71: boolean hasNext = assertDoesNotThrow(r1::hasNext);
Add comment why `assertDoesNotThrow` is needed, that is, because callers call
`checkHasNext` within a `assertThrows(XMLStreamException, ...)`?
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/Bug6767322Test.java
line 50:
> 48:
> 49: String version = r.getVersion();
> 50: System.out.println("Bug6767322.xml: " + version);
`assertNotNull(version)`? (or even `assertEquals`?)
test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/Bug6847819Test.java
line 51:
> 49: Assert.fail("Expected an exception for " + msg);
> 50: } catch (XMLStreamException ex) { // good
> 51: System.out.println("Expected failure: '" + ex.getMessage() +
> "' " + "(matching message: '" + msg + "')");
Should check exception message? While it was not done previously, this previous
`println` suggests `assertTrue(message.contains("illegal declaration"),
"message: " + message)`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/30643#pullrequestreview-4089865495
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064478416
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064534183
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064505023
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064580531
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064600095
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064611022
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064700744
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064727217
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064730075
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064755498
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064766025
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064767046
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064756614
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064778359
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064810038
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064844509
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064870484
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064885025
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064889920
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064908993
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064921903
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064940114
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064942838
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064947590
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3064964589
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065047893
PR Review Comment: https://git.openjdk.org/jdk/pull/30643#discussion_r3065056692
