8271302: Regex Test Refresh ------------- Commit messages: - Migrating regular expression tests to TestNG Changes: https://git.openjdk.java.net/jdk/pull/5092/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271302 Stats: 2226 lines in 3 files changed: 225 ins; 965 del; 1036 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
This PR migrates all regular expression tests in the jdk/java/util/regex directory to use TestNG assertions and annotations. The assertions utilized for this refresh are shared in common with standard ones from JUnit5 should such a migration occur in the future. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Changes requested by bchristi (Reviewer). In the JBS issue, it looks like the Description was put in the Environment. :) test/jdk/java/util/regex/RegExTest.java line 291:
289: 290: int resultStart1 = mr.start(); 291: assertEquals(matcherStart1, resultStart1, "equal matchers have equal start indices");
Should the message be that they *don't* have equal start indices ? test/jdk/java/util/regex/RegExTest.java line 2362:
2360: 2361: { "test\ud834\uddc0", "test\ud834\uddc0", "m", true }, 2362: //{ "test\ud834\uddbc\ud834\udd6f", "test\ud834\uddc0", "m", true }, //problem
Should an issue be filed for these //problems ? test/jdk/java/util/regex/RegExTest.java line 3952:
3950: 3951: m = Pattern.compile("\\H").matcher(matcherSubstring); 3952: assertTrue(m.find() || ng.equals(m.group()));
Should this be: `assertTrue(m.find() && ng.equals(m.group()));` ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 13 Aug 2021 20:16:22 GMT, Brent Christian <bchristi@openjdk.org> wrote:
8271302: Regex Test Refresh
test/jdk/java/util/regex/RegExTest.java line 2362:
2360: 2361: { "test\ud834\uddc0", "test\ud834\uddc0", "m", true }, 2362: //{ "test\ud834\uddbc\ud834\udd6f", "test\ud834\uddc0", "m", true }, //problem
Should an issue be filed for these //problems ?
Yessir! https://bugs.openjdk.java.net/browse/JDK-8271919 ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 13 Aug 2021 20:17:56 GMT, Brent Christian <bchristi@openjdk.org> wrote:
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Couple of fixes
test/jdk/java/util/regex/RegExTest.java line 3952:
3950: 3951: m = Pattern.compile("\\H").matcher(matcherSubstring); 3952: assertTrue(m.find() || ng.equals(m.group()));
Should this be: `assertTrue(m.find() && ng.equals(m.group()));`
Good catch. De Morgan's mistake ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Couple of fixes ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/1426e323..0e9fa209 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Couple of fixes
Marked as reviewed by bchristi (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Wed, 18 Aug 2021 18:35:53 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Couple of fixes
test/jdk/java/util/regex/NegativeArraySize.java line 2:
1: /* 2: * Copyright (c) 2019, 2021 Oracle and/or its affiliates. All rights reserved.
Add comma after 2021, or the copyright headers check won't pass. test/jdk/java/util/regex/NegativeArraySize.java line 29:
27: * @summary Pattern.compile() can throw confusing NegativeArraySizeException 28: * @requires os.maxMemory >= 5g 29: * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
I note that the order of the arguments has changed. Will that work as expected? Had it worked as expected before? test/jdk/java/util/regex/NegativeArraySize.java line 40:
38: @Test 39: public static void testNegativeArraySize() { 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q" + "a".repeat(42 + Integer.MAX_VALUE / 3)));
One observation on this regex. Although the regex looks invalid because `\\Q` misses the pairing `\\E`, it can still be compiled (with a reasonable number of a's, of course). Moreover, the resulting pattern matches strings in a surprising way: jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches() $1 ==> true test/jdk/java/util/regex/RegExTest.java line 27:
25: * @test 26: * @summary tests RegExp framework (use -Dseed=X to set PRNG seed) 27: * @author Mike McCloskey
What happened to Mike here? :-) test/jdk/java/util/regex/RegExTest.java line 121:
119: private static void check(String p, String s, boolean expected) { 120: Matcher matcher = Pattern.compile(p).matcher(s); 121: assertSame(matcher.find(), expected);
Why use `assertSame(Object, Object)`? I would expect `assertEquals(boolean, boolean)`. test/jdk/java/util/regex/RegExTest.java line 206:
204: } 205: 206: // Regular expression test// Most of the tests are in a file
Mistakenly joined comment lines? ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 20 Aug 2021 13:46:39 GMT, Pavel Rappo <prappo@openjdk.org> wrote:
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Couple of fixes
test/jdk/java/util/regex/NegativeArraySize.java line 40:
38: @Test 39: public static void testNegativeArraySize() { 40: assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q" + "a".repeat(42 + Integer.MAX_VALUE / 3)));
One observation on this regex. Although the regex looks invalid because `\\Q` misses the pairing `\\E`, it can still be compiled (with a reasonable number of a's, of course). Moreover, the resulting pattern matches strings in a surprising way:
jshell> Pattern.compile("\\Qaaa").matcher("aaa").matches() $1 ==> true
Maybe that behavior is expected after all. From "Mastering Regular Expressions" by Jeffrey E.F. Friedl, 3rd Edition, p. 136:
Literal-text span: `\Q...\E`
First introduced with Perl, the special sequence `\Q...\E` turns off all regex meta-characters between them, except for `\E` itself. (If the `\E` is omitted, they are turned off until the end of the regex.)
------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 20 Aug 2021 13:32:24 GMT, Pavel Rappo <prappo@openjdk.org> wrote:
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Couple of fixes
test/jdk/java/util/regex/NegativeArraySize.java line 29:
27: * @summary Pattern.compile() can throw confusing NegativeArraySizeException 28: * @requires os.maxMemory >= 5g 29: * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
I note that the order of the arguments has changed. Will that work as expected? Had it worked as expected before?
The new order is consistent with other tests. I had difficulty getting it to run in the original configuration. Perhaps jtreg is more sensitive on order with the testng runner.
test/jdk/java/util/regex/RegExTest.java line 121:
119: private static void check(String p, String s, boolean expected) { 120: Matcher matcher = Pattern.compile(p).matcher(s); 121: assertSame(matcher.find(), expected);
Why use `assertSame(Object, Object)`? I would expect `assertEquals(boolean, boolean)`.
Artifacting because of the use of `==` I'll make it more readable. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision: some quick fixes ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/0e9fa209..3937bf8f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=01-02 Stats: 8 lines in 2 files changed: 1 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 20 Aug 2021 16:27:53 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
some quick fixes
test/jdk/java/util/regex/RegExTest.java line 4270:
4268: String s = (String)pm[1]; 4269: boolean r = (Boolean)pm[2]; 4270: assertSame(r, Pattern.compile(p).matcher(s).matches());
`assertEquals(boolean, boolean)` might be better. test/jdk/java/util/regex/RegExTest.java line 4270:
4268: String s = (String)pm[1]; 4269: boolean r = (Boolean)pm[2]; 4270: assertSame(r, Pattern.compile(p).matcher(s).matches());
`expectThrows(PatternSyntaxException.class, ...)` might suit better for this and similar cases in this file. Not only does `expectThrows` test for expected exception, it also returns an exception which you can further inspect. Now, if we only had `assertThat` or similar functionality for compound assertions, the complete test might've looked nicely. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 20 Aug 2021 16:49:15 GMT, Pavel Rappo <prappo@openjdk.org> wrote:
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
some quick fixes
test/jdk/java/util/regex/RegExTest.java line 4270:
4268: String s = (String)pm[1]; 4269: boolean r = (Boolean)pm[2]; 4270: assertSame(r, Pattern.compile(p).matcher(s).matches());
`expectThrows(PatternSyntaxException.class, ...)` might suit better for this and similar cases in this file. Not only does `expectThrows` test for expected exception, it also returns an exception which you can further inspect. Now, if we only had `assertThat` or similar functionality for compound assertions, the complete test might've looked nicely.
The latter comment referred to hand-rolled try-fail constructs like this: https://github.com/openjdk/jdk/blob/3937bf8f9921395d6fcbdc2bb00a4a98dc2aaf62... Not sure why my IDE plugin messed up the comment position. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Additional cleanup ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/3937bf8f..3f01c172 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=02-03 Stats: 46 lines in 1 file changed: 1 ins; 27 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 20 Aug 2021 21:17:50 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Additional cleanup
Marked as reviewed by smarks (Reviewer). Whew! Changes to GraphemeTest.java and NegativeArraySize.java look fine. I finally figured out how to look at RegExTest.java effectively -- none of the diff views were helpful at all. I just brought up the old and new versions in windows side by side and just scrolled through them. Overall it looks good, mostly a replacement of the ad hoc internal framework (failCount++) with assertX from testng, and some minor reorganizations here and there, such as the splitting of `stringBufferSubstitute` into several tests. Overall it looks like most things here got about 15% shorter, which is pretty good since it reduces the overall bulk. (However, there's so much more to do....) One observation about this technique is that using the "failCount++" approach, if there is a failure, the tests in the same method will continue to run. With the assertX approach, the test will stop at that point, and any assertions later in the same method won't get run at all. Since we expect all the tests to pass all the time, this has no effect in the normal case. If there is a bug, though, the assertX approach might result in a single failure whereas the failCount++ approach might result in several failures. While this bothers some people, it doesn't bother me; it's likely only to occur at development time, and it's like to be only a minor impediment to development. Indeed, it's commonly viewed as a testing antipattern to have a single test method test multiple cases. The solution is to fix that, and not worry about failCount++ vs assertX. I think the long run solution here is to continue cleaning up these tests, possibly breaking down test methods further, eventually driving things into a purely table-driven or data generator/provider approach. test/jdk/java/util/regex/RegExTest.java line 85:
83: import static org.testng.Assert.fail; 84: import static org.testng.Assert.expectThrows; //Replaced by assertThrows in JUnit5 85:
Slightly odd to mention JUnit 5 here, since this is being converted to a TestNG test. Are these notes for yourself for future work? ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Fri, 27 Aug 2021 23:18:34 GMT, Stuart Marks <smarks@openjdk.org> wrote:
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Additional cleanup
test/jdk/java/util/regex/RegExTest.java line 85:
83: import static org.testng.Assert.fail; 84: import static org.testng.Assert.expectThrows; //Replaced by assertThrows in JUnit5 85:
Slightly odd to mention JUnit 5 here, since this is being converted to a TestNG test. Are these notes for yourself for future work?
Ah yes. I'll pull that. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision: Removing some notes re JUnit5 ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/5092/files - new: https://git.openjdk.java.net/jdk/pull/5092/files/3f01c172..0845a56f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5092&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5092.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092 PR: https://git.openjdk.java.net/jdk/pull/5092
On Mon, 30 Aug 2021 15:52:05 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Removing some notes re JUnit5
Looks like it was missed that the test fails oi a github actions: https://github.com/igraves/jdk/runs/3463998089 Run if ! grep --include=test-summary.txt -lqr build/*/test-results -e "TEST SUCCESS" ; then newfailures.txt java/util/regex/NegativeArraySize.java Error: Process completed with exit code 1. Invalid initial heap size: -Xms5G The specified size exceeds the maximum representable size. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Mon, 30 Aug 2021 15:52:05 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
Ian Graves has updated the pull request incrementally with one additional commit since the last revision:
Removing some notes re JUnit5
https://bugs.openjdk.java.net/browse/JDK-8273169 I'd like to fix it. --- a/test/jdk/java/util/regex/NegativeArraySize.java +++ b/test/jdk/java/util/regex/NegativeArraySize.java @@ -25,7 +25,7 @@ * @test * @bug 8223174 * @summary Pattern.compile() can throw confusing NegativeArraySizeException - * @requires os.maxMemory >= 5g + * @requires os.maxMemory >= 5g & vm.bits == 64 * @run testng/othervm -Xms5G -Xmx5G NegativeArraySize */ ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
On Wed, 11 Aug 2021 18:22:42 GMT, Ian Graves <igraves@openjdk.org> wrote:
8271302: Regex Test Refresh
This pull request has now been integrated. Changeset: fecefb85 Author: Ian Graves <igraves@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/fecefb8541d5056b1a8b105126ac9c566875... Stats: 2253 lines in 3 files changed: 220 ins; 989 del; 1044 mod 8271302: Regex Test Refresh Reviewed-by: bchristi, smarks ------------- PR: https://git.openjdk.java.net/jdk/pull/5092
participants (6)
-
Brent Christian
-
Ian Graves
-
Pavel Rappo
-
Sergey Bylokhov
-
Stuart Marks
-
xpbob