Re: RFR: 8166398: CatalogSupport tests need to be fixed -> Re: RFR: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage
Thanks Daniel! I'm glad the issue was caught! I'm also making sure any the debugging method shall throw Exception as well :-) Best, Joe On 9/22/16, 2:09 AM, Daniel Fuchs wrote: 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
Re: RFR: 8166398: CatalogSupport tests need to be fixed -> Re: RFR: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage
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
RFR: 8166398: CatalogSupport tests need to be fixed -> Re: RFR: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage
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