RFR: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException [v4]

[Joe Wang]

On Wed, 9 Dec 2020 11:49:51 GMT, Michael Edgar <github.com+20868526+MikeEdgar at openjdk.org> wrote:

>> Allow `XMLStreamException` to be thrown to the application when a filtered `XMLStreamReader` encounters an `XMLStreamException` advancing the wrapped `XMLStreamReader`.
>> 
>> Note, this PR includes a method signature change (constructor of `com.sun.org.apache.xerces.internal.impl.XMLStreamFilterImpl`).
>
> Michael Edgar has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Additional test class clean-up

Looks good!  Some minor comments below.

test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/XMLStreamReaderFilterTest.java line 60:

> 58:      * {@code XMLStreamException} thrown when the underlying XML is not valid and
> 59:      * the filter condition requires that the original reader is advanced past the
> 60:      * invalid data.

This test verifies an assertion made by the javadoc for the createFilteredReader method. It would be good to made the first sentence a summary of the test. Something like:

Verifies that XMLStreamException is thrown as specified by the {@code XMLInputFactory::createFilteredReader} method when an error is encountered. This test illustrates the scenario by creating a reader with a filter that requires the original reader to advance past the invalid element in the underlying XML.

test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/XMLStreamReaderFilterTest.java line 64:

> 62:      */
> 63:     @Test
> 64:     public void testXMLStreamReaderExceptionThrownByConstructor() throws Exception {

Method name is a bit long. Since the class is dedicated to testing StreamFilter, its test cases may just indicate their particular cases. I would think testCreateFilteredReader is sufficient.

test/jaxp/javax/xml/jaxp/unittest/stream/XMLStreamFilterTest/XMLStreamReaderFilterTest.java line 65:

> 63:     @Test
> 64:     public void testXMLStreamReaderExceptionThrownByConstructor() throws Exception {
> 65:         StreamFilter filter = r -> r.getEventType() == XMLStreamConstants.START_ELEMENT && r.getLocalName().equals("element3");

Keeping the length to under 100 characters is still a good practice even with wide screens, 80 - 100 would be good as it's easier to read with a side-by-side view.

-------------

Marked as reviewed by joehw (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1209