On Wed, 9 Dec 2020 11:49:51 GMT, Michael Edgar
<github.com+20868526+mikeed...@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
