On Fri, 20 Aug 2021 21:17:50 GMT, Ian Graves <igra...@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
