Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271302: Regex Test Refresh #5092

Closed
wants to merge 5 commits into from
Closed

Conversation

@igraves
Copy link
Member

@igraves igraves commented Aug 11, 2021


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5092/head:pull/5092
$ git checkout pull/5092

Update a local copy of the PR:
$ git checkout pull/5092
$ git pull https://git.openjdk.java.net/jdk pull/5092/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5092

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5092.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 11, 2021

👋 Welcome back igraves! 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.

@igraves
Copy link
Member Author

@igraves igraves commented Aug 11, 2021

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.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 11, 2021

@igraves The following labels will be automatically applied to this pull request:

  • core-libs
  • i18n

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

@igraves igraves changed the title 8271302: Migrating regular expression tests to TestNG 8271302: Regex Test Refresh Aug 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 11, 2021

@bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Aug 13, 2021

In the JBS issue, it looks like the Description was put in the Environment. :)

test/jdk/java/util/regex/RegExTest.java Outdated Show resolved Hide resolved
test/jdk/java/util/regex/RegExTest.java Show resolved Hide resolved
test/jdk/java/util/regex/RegExTest.java Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Aug 18, 2021

@igraves 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:

8271302: Regex Test Refresh

Reviewed-by: bchristi, smarks

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 321 new commits pushed to the master branch:

  • 98b9d98: 8272797: Mutex with rank safepoint_check_never imply allow_vm_block
  • f11e099: 8272651: G1 heap region info print order changed by JDK-8269914
  • fbffa54: 8270438: "Cores to use" output in configure is misleading
  • 5185dbd: 8273098: Unnecessary Vector usage in java.naming
  • 276b07b: 8271490: [ppc] [s390]: Crash in JavaThread::pd_get_top_frame_for_profiling
  • bb7aa1c: 8272161: Make evacuation failure data structures local to collection
  • 9ede41b: 8229031: Exporting CLASSPATH from shell can result in build failures
  • 16e8305: 8273059: Redundant Math.min call in Http2ClientImpl#getConnectionWindowSize
  • f55d5ab: 8272838: Move CriticalJNI tests out of tier1
  • a9188f2: 8268894: forged ASTs can provoke an AIOOBE at com.sun.tools.javac.jvm.ClassWriter::writePosition
  • ... and 311 more: https://git.openjdk.java.net/jdk/compare/034788a02cbe1f80fc1581ec307a3d54bef380b4...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 18, 2021
@@ -24,7 +24,6 @@
/**
* @test
* @summary tests RegExp framework (use -Dseed=X to set PRNG seed)
* @author Mike McCloskey
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happened to Mike here? :-)

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2021 Oracle and/or its affiliates. All rights reserved.
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comma after 2021, or the copyright headers check won't pass.

@@ -26,17 +26,17 @@
* @bug 8223174
* @summary Pattern.compile() can throw confusing NegativeArraySizeException
* @requires os.maxMemory >= 5g
* @run main/othervm NegativeArraySize -Xms5G -Xmx5G
* @run testng/othervm -Xms5G -Xmx5G NegativeArraySize
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that the order of the arguments has changed. Will that work as expected? Had it worked as expected before?

Copy link
Member Author

@igraves igraves Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
public static void testNegativeArraySize() {
assertThrows(OutOfMemoryError.class, () -> Pattern.compile("\\Q" + "a".repeat(42 + Integer.MAX_VALUE / 3)));
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

}

private static void check(String p, String s, boolean expected) {
Matcher matcher = Pattern.compile(p).matcher(s);
if (matcher.find() != expected)
failCount++;
assertSame(matcher.find(), expected);
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use assertSame(Object, Object)? I would expect assertEquals(boolean, boolean).

Copy link
Member Author

@igraves igraves Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Artifacting because of the use of == I'll make it more readable.

@@ -325,154 +203,138 @@ private static String toSupplementaries(String s) {
return sb.toString();
}

// Regular expression tests
// Regular expression test// Most of the tests are in a file
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistakenly joined comment lines?

if (r != Pattern.compile(p).matcher(s).matches()) {
failCount++;
}
assertSame(r, Pattern.compile(p).matcher(s).matches());
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertEquals(boolean, boolean) might be better.

if (r != Pattern.compile(p).matcher(s).matches()) {
failCount++;
}
assertSame(r, Pattern.compile(p).matcher(s).matches());
Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@pavelrappo pavelrappo Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter comment referred to hand-rolled try-fail constructs like this:

try {
Pattern.compile(pat);
fail();
} catch (PatternSyntaxException e) {
if (!e.getMessage().startsWith(
"capturing group name does not start with a"
+ " Latin letter")) {
fail();
}

Not sure why my IDE plugin messed up the comment position.

@stuart-marks
Copy link
Member

@stuart-marks stuart-marks commented Aug 27, 2021

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.

import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;
import static org.testng.Assert.expectThrows; //Replaced by assertThrows in JUnit5

Copy link
Member

@stuart-marks stuart-marks Aug 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly odd to mention JUnit 5 here, since this is being converted to a TestNG test. Are these notes for yourself for future work?

Copy link
Member Author

@igraves igraves Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes. I'll pull that.

@igraves
Copy link
Member Author

@igraves igraves commented Aug 30, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2021

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

  • f18c0fa: 8271560: sun/security/ssl/DHKeyExchange/LegacyDHEKeyExchange.java still fails due to "An established connection was aborted by the software in your host machine"
  • 5aaa20f: 8272861: Add a micro benchmark for vector api
  • 7a01ba6: 8272093: Extract evacuation failure injection from G1CollectedHeap
  • 98b9d98: 8272797: Mutex with rank safepoint_check_never imply allow_vm_block
  • f11e099: 8272651: G1 heap region info print order changed by JDK-8269914
  • fbffa54: 8270438: "Cores to use" output in configure is misleading
  • 5185dbd: 8273098: Unnecessary Vector usage in java.naming
  • 276b07b: 8271490: [ppc] [s390]: Crash in JavaThread::pd_get_top_frame_for_profiling
  • bb7aa1c: 8272161: Make evacuation failure data structures local to collection
  • 9ede41b: 8229031: Exporting CLASSPATH from shell can result in build failures
  • ... and 314 more: https://git.openjdk.java.net/jdk/compare/034788a02cbe1f80fc1581ec307a3d54bef380b4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2021

@igraves Pushed as commit fecefb8.

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

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 31, 2021

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.

@xpbob
Copy link

@xpbob xpbob commented Aug 31, 2021

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
  */

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants