RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa
JDK-8264454 : Jaxp unit test from open jdk needs to be improved

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

Commit messages:
 - JDK-8264454 : Jaxp unit test from open jdk needs to be improved
 - JDK-8064681 : Jaxp unit test need to be improved

Changes: https://git.openjdk.java.net/jdk/pull/3274/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3274&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264454
  Stats: 743 lines in 8 files changed: 71 ins; 596 del; 76 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3274/head:pull/3274

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Joe Wang-2
On Tue, 30 Mar 2021 16:56:57 GMT, Mahendra Chhipa <[hidden email]> wrote:

> JDK-8264454 : Jaxp unit test from open jdk needs to be improved

test/jaxp/javax/xml/jaxp/unittest/common/Bug6350682.java line 47:

> 45:     @Test
> 46:     public void testSAXParserFactory() {
> 47:         runWithAllPerm(() -> Thread.currentThread().setContextClassLoader(null));

To address item 4 (the environment is changed), a note, something like "this test runs in othervm" can be added here or to the summary.

test/jaxp/javax/xml/jaxp/unittest/common/Bug6723276Test.java line 45:

> 43: @Listeners({jaxp.library.BasePolicy.class})
> 44: public class Bug6723276Test {
> 45:     private static final String ERR_MSG = "org.apache.xerces.jaxp.SAXParserFactoryImpl not found";

This test currently doesn't verify anything since there's no org.apache.xerces.jaxp.SAXParserFactoryImpl on the classpath. Post JDK 9, such test would require creating a dummy module.

test/jaxp/javax/xml/jaxp/unittest/common/Bug6723276Test.java line 48:

> 46:
> 47:     @Test
> 48:     public void testSAXParserFactoryCreationWithDefaultContextClassLoader() {

A shorter name such as testWithDefaultClassLoader would be fine.

test/jaxp/javax/xml/jaxp/unittest/common/Bug6723276Test.java line 59:

> 57:
> 58:     @Test
> 59:     public void testSAXParserFactoryCreationWithGivenURLContextClassLoader() {

A shorter name would be testWithURLClassLoader

test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6320118.java line 59:

> 57:         Assert.assertEquals(calendar.getMonth(), 1);
> 58:         Assert.assertEquals(calendar.getDay(), 2);
> 59:         Assert.assertEquals(calendar.getHour(), 0, "hour 24 needs to be treated as hour 0 of next day");

This change has added assertions, but it seems ok.

test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 57:

> 55:      * Constant to indicate expected lexical test failure.
> 56:      */
> 57:     private static final String TEST_VALUE_FAIL = "*FAIL*";

I don't see this in the expected values. It, along with the related assertions, may be removed.

test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 194:

> 192:             try {
> 193:                 QName xmlSchemaType = duration.getXMLSchemaType();
> 194:                 if (!xmlSchemaType.equals(DatatypeConstants.DURATION_YEARMONTH)) {

Can be changed to assertEquals

test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 200:

> 198:                 } catch (IllegalStateException illegalStateException) {
> 199:                     // TODO; this test really should pass
> 200:                     System.err.println("Please fix this bug that is being ignored, for now: " + illegalStateException.getMessage());

Do we still have such a bug?

test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 204:

> 202:
> 203:                 // does it have the right value?
> 204:                 if (!expectedLex.equals(duration.toString())) {

Can be changed to assertEquals

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

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa
On Wed, 31 Mar 2021 04:19:46 GMT, Joe Wang <[hidden email]> wrote:

>> JDK-8264454 : Jaxp unit test from open jdk needs to be improved
>
> test/jaxp/javax/xml/jaxp/unittest/common/Bug6350682.java line 47:
>
>> 45:     @Test
>> 46:     public void testSAXParserFactory() {
>> 47:         runWithAllPerm(() -> Thread.currentThread().setContextClassLoader(null));
>
> To address item 4 (the environment is changed), a note, something like "this test runs in othervm" can be added here or to the summary.

Added the comment

> test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 204:
>
>> 202:
>> 203:                 // does it have the right value?
>> 204:                 if (!expectedLex.equals(duration.toString())) {
>
> Can be changed to assertEquals

done

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

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa
In reply to this post by Joe Wang-2
On Wed, 31 Mar 2021 05:04:31 GMT, Joe Wang <[hidden email]> wrote:

>> JDK-8264454 : Jaxp unit test from open jdk needs to be improved
>
> test/jaxp/javax/xml/jaxp/unittest/datatype/Bug6937964Test.java line 200:
>
>> 198:                 } catch (IllegalStateException illegalStateException) {
>> 199:                     // TODO; this test really should pass
>> 200:                     System.err.println("Please fix this bug that is being ignored, for now: " + illegalStateException.getMessage());
>
> Do we still have such a bug?

Yes, test is still trowing IllegalStateException.

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

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved [v2]

Mahendra Chhipa
In reply to this post by Mahendra Chhipa
> JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision:

  Implemented the review comments.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3274/files
  - new: https://git.openjdk.java.net/jdk/pull/3274/files/2775a644..3f258897

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3274&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3274&range=00-01

  Stats: 26 lines in 3 files changed: 2 ins; 12 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3274/head:pull/3274

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved [v3]

Mahendra Chhipa
In reply to this post by Mahendra Chhipa
> JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision:

  Updated copyright year.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3274/files
  - new: https://git.openjdk.java.net/jdk/pull/3274/files/3f258897..002bdffc

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3274&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3274&range=01-02

  Stats: 4 lines in 4 files changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3274/head:pull/3274

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved [v3]

Joe Wang-2
On Tue, 6 Apr 2021 16:53:03 GMT, Mahendra Chhipa <[hidden email]> wrote:

>> JDK-8264454 : Jaxp unit test from open jdk needs to be improved
>
> Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision:
>
>   Updated copyright year.

Marked as reviewed by joehw (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Joe Wang-2
In reply to this post by Mahendra Chhipa
On Tue, 30 Mar 2021 16:56:57 GMT, Mahendra Chhipa <[hidden email]> wrote:

> JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Ok, this is enough of improvement for now. I created a bug to keep track of the ISE case.

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

PR: https://git.openjdk.java.net/jdk/pull/3274
Reply | Threaded
Open this post in threaded view
|

Integrated: JDK-8264454 : Jaxp unit test from open jdk needs to be improved

Mahendra Chhipa
In reply to this post by Mahendra Chhipa
On Tue, 30 Mar 2021 16:56:57 GMT, Mahendra Chhipa <[hidden email]> wrote:

> JDK-8264454 : Jaxp unit test from open jdk needs to be improved

This pull request has now been integrated.

Changeset: 308f6796
Author:    Mahendra Chhipa <[hidden email]>
Committer: Joe Wang <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/308f6796
Stats:     757 lines in 8 files changed: 71 ins; 606 del; 80 mod

8264454: Jaxp unit test from open jdk needs to be improved

Reviewed-by: joehw

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

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