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

bwhuang-us · 2022-08-04T17:40:43Z

Improve test coverage for XPath functions: JDK-8290836, JDK-8290837, JDK-8290838.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issues

JDK-8290836: Improve test coverage for XPath functions: String Functions
JDK-8290837: Improve test coverage for XPath functions: Boolean Functions
JDK-8290838: Improve test coverage for XPath functions: Number Functions

Reviewers

Joe Wang (@JoeWang-Java - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9752/head:pull/9752
$ git checkout pull/9752

Update a local copy of the PR:
$ git checkout pull/9752
$ git pull https://git.openjdk.org/jdk pull/9752/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9752

View PR using the GUI difftool:
$ git pr show -t 9752

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9752.diff

bridgekeeper · 2022-08-04T17:41:03Z

👋 Welcome back bwhuang-us! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bwhuang-us · 2022-08-04T17:41:23Z

/issue add JDK-8290837, JDK-8290838

openjdk · 2022-08-04T17:43:48Z

@bwhuang-us
Adding additional issue to issue list: 8290837: Improve test coverage for XPath functions: Boolean Functions.

Adding additional issue to issue list: 8290838: Improve test coverage for XPath functions: Number Functions.

openjdk · 2022-08-04T17:45:36Z

@bwhuang-us The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-08-04T17:49:44Z

Webrevs

01: Full - Incremental (9bdb1677)
00: Full (5ab30e1b)

JoeWang-Java

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.

JoeWang-Java · 2022-08-04T21:44:36Z

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java

+
+    /*
+     * DataProvider for testing TransformerException being thrown on
+     * invalid number function usage.


I think you meant XPathExpressionException instead of TransformerException.

Typo: s/number/boolean

This comment applies to other occurrences as well.

JoeWang-Java · 2022-08-04T22:14:40Z

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java

+     */
+    @DataProvider(name = "exceptionExpTestCases")
+    public Object[][] getExceptionExp() {
+        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.

JoeWang-Java · 2022-08-04T22:26:35Z

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathBooleanFnTest.java

+     * @throws Exception if test fails
+     */
+    @Test(dataProvider = "exceptionExpTestCases", expectedExceptions =
+            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.

JoeWang-Java · 2022-08-04T22:48:22Z

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathNumberFnTest.java

+     */
+    @Test(dataProvider = "numberExpTestCases")
+    void testNumberFn(String exp, double expected) throws Exception {
+        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.

JoeWang-Java · 2022-08-04T23:04:18Z

test/jaxp/javax/xml/jaxp/unittest/xpath/XPathTestBase.java

@@ -77,16 +79,20 @@ class XPathTestBase {
            + "            <City>The City</City>"
            + "            <State>The State</State>"
            + "        </Address>"
+            + "        <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.

JoeWang-Java

Thanks for the update.

openjdk · 2022-08-09T01:05:20Z

@bwhuang-us This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290836: Improve test coverage for XPath functions: String Functions
8290837: Improve test coverage for XPath functions: Boolean Functions
8290838: Improve test coverage for XPath functions: Number Functions

Reviewed-by: joehw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 90 new commits pushed to the master branch:

ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
8d0d9ea: 8291238: JDK can't be built without G1
aff7689: 8292075: Remove deleted test compiler/rangechecks/TestRangeCheckHoistingScaledIV.java from ProblemList
3677b55: 6391806: JLabel and AbstractButton's imageUpdate method should be better specified
0ade264: 8290814: Hide G1RootRegions behind G1ConcurrentMark
f5b3618: 8291970: Add TableStatistics get function to ResourceHashtable
cbc9040: 8290278: JavaDoc of index parameter of method JTabbedPane.insertTab
2712bc3: 8289741: Remove unused imports from DTDBuilder.java
af243ca: 8265433: IGV: add graph tooltips with properties
... and 80 more: https://git.openjdk.org/jdk/compare/57bf603b7332db86c39680d16b78f94a904daf46...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JoeWang-Java) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

bwhuang-us · 2022-08-09T17:51:41Z

/integrate

openjdk · 2022-08-09T18:13:21Z

@bwhuang-us
Your change (at version 9bdb167) is now ready to be sponsored by a Committer.

JoeWang-Java · 2022-08-09T18:25:32Z

/sponsor

openjdk · 2022-08-09T18:27:07Z

Going to push as commit d889264.
Since your change was applied there have been 90 commits pushed to the master branch:

ae1a9a0: 8292060: Make ClassFileVersionTest.java version adapting
4040927: 8290047: (fs) FileSystem.getPathMatcher does not check for ":" at last index
8d0d9ea: 8291238: JDK can't be built without G1
aff7689: 8292075: Remove deleted test compiler/rangechecks/TestRangeCheckHoistingScaledIV.java from ProblemList
3677b55: 6391806: JLabel and AbstractButton's imageUpdate method should be better specified
0ade264: 8290814: Hide G1RootRegions behind G1ConcurrentMark
f5b3618: 8291970: Add TableStatistics get function to ResourceHashtable
cbc9040: 8290278: JavaDoc of index parameter of method JTabbedPane.insertTab
2712bc3: 8289741: Remove unused imports from DTDBuilder.java
af243ca: 8265433: IGV: add graph tooltips with properties
... and 80 more: https://git.openjdk.org/jdk/compare/57bf603b7332db86c39680d16b78f94a904daf46...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-09T18:27:22Z

@JoeWang-Java @bwhuang-us Pushed as commit d889264.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

bwhuang-us added 4 commits August 3, 2022 15:03

Changes for String, Bollean and Number Functions

e195428

Extracted common test codes for xpath exp eval to a new function.

8fffb06

Added exception tests.

e07f476

Added exception test cases for node set functions.

5ab30e1

openjdk bot added the rfr label Aug 4, 2022

openjdk bot added the core-libs label Aug 4, 2022

JoeWang-Java reviewed Aug 5, 2022

View reviewed changes

Added comments and fixed typo.

9bdb167

JoeWang-Java approved these changes Aug 9, 2022

View reviewed changes

openjdk bot added the ready label Aug 9, 2022

openjdk bot added the sponsor label Aug 9, 2022

openjdk bot added the integrated label Aug 9, 2022

openjdk bot closed this Aug 9, 2022

openjdk bot removed ready rfr sponsor labels Aug 9, 2022

bwhuang-us deleted the JDK-8290836 branch March 1, 2023 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bwhuang-us commented Aug 4, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 4, 2022

bwhuang-us commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

mlbridge bot commented Aug 4, 2022 •

edited

Loading

JoeWang-Java left a comment

JoeWang-Java Aug 4, 2022

JoeWang-Java Aug 4, 2022

JoeWang-Java Aug 4, 2022

JoeWang-Java Aug 4, 2022

JoeWang-Java Aug 4, 2022

JoeWang-Java left a comment

openjdk bot commented Aug 9, 2022 •

edited

Loading

bwhuang-us commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

JoeWang-Java commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

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

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

Conversation

bwhuang-us commented Aug 4, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Aug 4, 2022

bwhuang-us commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

mlbridge bot commented Aug 4, 2022 • edited Loading

Webrevs

JoeWang-Java left a comment

Choose a reason for hiding this comment

JoeWang-Java Aug 4, 2022

Choose a reason for hiding this comment

JoeWang-Java Aug 4, 2022

Choose a reason for hiding this comment

JoeWang-Java Aug 4, 2022

Choose a reason for hiding this comment

JoeWang-Java Aug 4, 2022

Choose a reason for hiding this comment

JoeWang-Java Aug 4, 2022

Choose a reason for hiding this comment

JoeWang-Java left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 9, 2022 • edited Loading

bwhuang-us commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

JoeWang-Java commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

openjdk bot commented Aug 9, 2022

bwhuang-us commented Aug 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 4, 2022 •

edited

Loading

openjdk bot commented Aug 9, 2022 •

edited

Loading