On Tue, 30 Aug 2022 20:17:04 GMT, Raffaello Giulietti <d...@openjdk.org> wrote:
>> test/jdk/java/util/regex/NamedGroupsTests.java line 45: >> >>> 43: >>> 44: testMatchResultStartEndGroup(); >>> 45: } >> >> I haven't gone through all the tests in great detail (yet), but it occurs to >> me that we potentially have THREE implementations of some of the logic, and >> strictly speaking we should test all the code paths. The three >> implementations are in: >> >> 1. Matcher >> 2. Matcher$ImmutableMatchResult >> 3. MatchResult's default methods >> >> I took a quick look and it looks like Matcher and >> Matcher$ImmutableMatchResult override the default methods, so the default >> methods themselves need to be tested. This is essentially testing the >> `@implSpec`. The typical way to do that is to have the test create its own >> MatchResult implementation(s). There might need to be implementations that >> do and do not implement `namedGroups`, in order to test UOE. They might also >> need some state to cover various cases of no-match, has-match with and >> without group names, etc. > > An implementation that overrides `namedGroups` without overriding the other > methods accepting group names is `Matcher$ImmutableMatchResult`, which is > already exercised in the tests. OK, as long as all the code paths are covered. >> test/jdk/java/util/regex/NamedGroupsTests.java line 51: >> >>> 49: testMatchResultStartEndGroup2(); >>> 50: testMatchResultStartEndGroup3(); >>> 51: } >> >> The three numbered tests here are a little hard to follow. Looks like these >> tests are >> >> 1. test existing group names, with no match >> 2. test existing group names, with a successful match >> 3. test nonexistent group names, with a successful match >> >> On test names, sometimes people provide extremely verbose test names such as >> `testThatExistingGroupNameWithMatchReturnsNegativeOrNull` which I think is >> overkill, but having a name that's somewhat descriptive would be helpful. >> >> It looks like a case is missing, which is a test for a nonexistent group >> name on a MatchResult that doesn't have a successful match. I'm not sure >> which is checked first; I think the implementation would throw IAE, because >> of the nonexistent name, regardless of whether or not the MatchResult has a >> match. >> >> However, I don't think we've specified this, and in fact I don't think we >> want to. In general though, if multiple error conditions can arise in the >> same operation, the general style is not to constrain implementations to >> check for things in a particular order. Thus either IAE or ISE would be >> acceptable. Perhaps a test should be added for that. (Hm, might want to take >> another look at the specs regarding this issue.) > >> (Hm, might want to take another look at the specs regarding this issue.) > > Not sure who wants to take another look. If that it's you, then I'll wait > with the CSR. > I'll change the method names to something a bit more speaking. Sorry I should have been more specific. "Somebody" should take another look. :-) Well, anyway, I did, and the specification as written does not indicate which error condition is checked first. I think this is OK, so I don't think any changes are necessary. You might mention this in the text of the CSR; I know that Joe and I have discussed this issue previously, and he might have a recommendation. ------------- PR: https://git.openjdk.org/jdk/pull/10000
