RFR: 8065554: MatchResult should provide values of named-capturing groups [v4]

[Stuart Marks]

On Tue, 30 Aug 2022 20:17:04 GMT, Raffaello Giulietti <duke at 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