RFR: JDK-8290836 Improve test coverage for XPath functions: String Functions

[Joe Wang]

On Thu, 4 Aug 2022 17:40:43 GMT, Bill Huang <duke at openjdk.org> wrote:

> Improve test coverage for XPath functions: [JDK-8290836](https://bugs.openjdk.org/browse/JDK-8290836), [JDK-8290837](https://bugs.openjdk.org/browse/JDK-8290837), [JDK-8290838](https://bugs.openjdk.org/browse/JDK-8290838).

Overall, nice set of tests!

I see you've covering three issues, sth. I don't have experience with, so let's see what happens. It might be that the tests have different bug id associated with them, not much of an issue as this is a test enhancement.

See comments below.

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java line 96:

> 94:     /*
> 95:      * DataProvider for testing TransformerException being thrown on
> 96:      * invalid number function usage.

I think you meant XPathExpressionException instead of TransformerException.

Typo: s/number/boolean

This comment applies to other occurrences as well.

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java line 102:

> 100:     @DataProvider(name = "exceptionExpTestCases")
> 101:     public Object[][] getExceptionExp() {
> 102:         return new Object[][]{

Applies to DataProviders for exception testing: would be good to add some notes about the data, e.g. why they are invalid.

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java line 130:

> 128:      */
> 129:     @Test(dataProvider = "exceptionExpTestCases", expectedExceptions =
> 130:             XPathExpressionException.class)

expectedExceptions is fine in this case. But there's a preference in using assertThrows in general. I'll leave it to you to decide.

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathNumberFnTest.java line 144:

> 142:     @Test(dataProvider = "numberExpTestCases")
> 143:     void testNumberFn(String exp, double expected) throws Exception {
> 144:         testExp(doc, exp, expected, Double.class);

Besides Double, Integer and Long are also supported. It's ok though as the focus here is on the XPath expressions. If you want to add a few cases, that would be good too.

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathTestBase.java line 82:

> 80:             + "            <State>The State</State>"
> 81:             + "        </Address>"
> 82:             + "        <Age>0</Age>"

I don't see any issue in the test cases themselves. But the ages being 0, 1, -1 makes it slightly less readable because they can be confused with the number of nodes in the nodeset, even number(true()) as that also returns 1. It could be sth. that leads to mistakes. I would set the ages > 1, or actually greater than the number of nodes possibly returned.

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

PR: https://git.openjdk.org/jdk/pull/9752