RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()

[Volker Simonis]

On Tue, Jul 12, 2016 at 4:27 AM, Mandy Chung <mandy.chung at oracle.com> wrote:
>
>> On Jul 12, 2016, at 12:18 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>
>> Hi,
>>
>> here comes a new version of this change. I've tried to address most of
>> the concerns/suggestions:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/
>>
>
> This version looks okay in general.

Please find the new webrev at:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v2/

>I suggest to combine the check and Integer.parseInt in a single method (e.g. “numberAt” or some other better method name).
>

Done.

>   71         if (index - prevIndex > 1 &&
>
> may read better as "index > prevIndex”
>

That's actually not the same. I would have to write "index > prevIndex
+ 1" which I think isn't much better.

> The new comment can simply say “This method is used by regression tests” that should do it.  I would not mention the test name in the source since refactoring may make it outdated.
>

Done.

> In the new test,
>
>   92             if (error) {
>   93                 throw new Exception(invalidVersions[i] +
>   94                         " not recognized as invalid by VersionProps.parseVersionNumbers()");
>   95             }
>
> An alternative to the above check, it can throw an exception immediate after line 82 when  parseVersionNumbers.invoke succeeds.
>

Yes, that's much clearer. Thanks.

Any other  comments?

Regards,
Volker