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

2016-09-22 Thread Joe Wang 

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

2016-09-22 Thread 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

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

2016-09-21 Thread Joe Wang 

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: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage

2016-09-20 Thread Joe Wang 



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: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage

2016-09-20 Thread Daniel Fuchs 

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.

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?

I can't say I understand how the new test works, but nothing
else jumped at me in your webrev :-)

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: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage

2016-09-19 Thread Joe Wang 

Thanks Lance!

Best
Joe

On 9/19/16, 1:13 PM, Lance Andersen wrote:

Hi Joe,

Seems OK.

Best
Lance
On Sep 19, 2016, at 3:11 PM, 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





Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8166220: Catalog API: JAXP XML Processor Support - add StAX test coverage

2016-09-19 Thread Lance Andersen 
Hi Joe,

Seems OK.

Best
Lance
> On Sep 19, 2016, at 3:11 PM, 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
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com