RFR: 8166398: CatalogSupport tests need to be fixed -> Re: RFR: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage

[Daniel Fuchs]

Hi Joe,

Looks good to me.

Thanks for having taken the time to take a second look :-)

best regards,

-- daniel

On 21/09/16 17:47, Joe Wang wrote:
> Hi Daniel,
>
> Here's the fix for the test issues, a couple of errors including missing
> handler in StAX test, and dtd was mistakingly typed as xsd . The root
> cause is a debugging assertEquals method that printed out debugging
> message without throwing Exception. I've searched the jaxp/test to make
> sure this is the only case where this is used.
>
> While fixing the tests, also refactored the code so that the use-catalog
> is checked as a top condition, removed a ObjectFactory call in the course.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8166398
> webrev: http://cr.openjdk.java.net/~joehw/jdk9/8166398/webrev/
>
> Thanks,
> Joe
>
> On 9/20/16, 11:22 AM, Joe Wang wrote:
>>
>>
>> On 9/20/16, 2:20 AM, Daniel Fuchs wrote:
>>> Hi Joe,
>>>
>>> test/javax/xml/jaxp/unittest/catalog/CatalogSupportBase.java
>>>
>>>  603     /**
>>>  604      * Returns the text of the first element found by the reader.
>>>  605      * @param streamReader the XMLStreamReader
>>>  606      * @return the text of the first element
>>>  607      * @throws XMLStreamException
>>>  608      */
>>>  609     String getText(XMLStreamReader streamReader, int type)
>>> throws XMLStreamException {
>>>
>>> It would be good to update the javadoc of this method (in particular
>>> document the new 'type' parameter) so that future maintainer
>>> can understand what that method is supposed to do.
>>
>> Thanks Daniel!  I'll fix this.
>>
>> As I went through the tests again, I found there's actually an error
>> in the StAX test where the handler was missed. I filed a new bug:
>> https://bugs.openjdk.java.net/browse/JDK-8166398
>>
>>>
>>> In particular would it be OK to encounter both CHARACTERS and
>>> ENTITY_REFERENCE, or are these exclusive. If they are should some
>>> exception be thrown?
>>
>> They can't. At any given point, the StAX parser returns an unique
>> event and its event type.
>>>
>>> I can't say I understand how the new test works, but nothing
>>> else jumped at me in your webrev :-)
>>
>> The incorrect javadoc was enough to raise an alarm, that this part of
>> code was copied and forgot to update. It led me to double-check the
>> code and found the issue (JDK-8166398). I really appreciate it!
>>
>> Best,
>> Joe
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>
>>> On 19/09/16 20:11, Joe Wang wrote:
>>>> Hi,
>>>>
>>>> Please review an addition of StAX tests to the Catalog Support test
>>>> set.
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8166220
>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk9/8166220/webrev/
>>>>
>>>> Thanks,
>>>> Joe
>>>>
>>>