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

[Volker Simonis]

On Thu, Jul 7, 2016 at 5:33 PM, Claes Redestad
<claes.redestad at oracle.com> wrote:
> Hi Volker,
>
> On 2016-07-07 15:59, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have a review for the following change which makes
>> VersionProps.versionNumbers() testable and the corresponding
>> regression test:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564/
>
>
> thanks for doing this!
>
> Changes to VersionProps looks good.
>
> Adding tests to the root of jdk/test/java/lang might be frowned upon. While
> VersionProps is a class of it's own, it is also tightly coupled with
> Runtime.java; perhaps the test could find a home under
> java/lang/Runtime/Version?
>
> For readability List.of might be preferred over Arrays.asList
>
>> https://bugs.openjdk.java.net/browse/JDK-8160564
>>
>> During the review for "8160457: VersionProps.versionNumbers() is
>> broken" it was suggested to refactor VersionProps.versionNumbers() in
>> order to make it testable by a regression test. This change does
>> exactly that. It extracts the implementation of
>> VersionProps.versionNumbers() into a new method
>> parseVersionNumbers(String versionNumber) which can be tested from the
>> associated regression test.
>>
>> There are still two points to notice:
>>
>> - VersionProps.versionNumbers() deliberately doesn't check for badly
>> formatted version strings because it is assumed it will only get valid
>> input (see discussion for "JDK-8160000: Runtime.version() cause
>> startup regressions" at [2]). So we can currently only check that
>> VersionProps.versionNumbers() accepts all kind of valid version
>> strings.
>
>
> Not sure how error checking could or should be improved:
> VersionProps.parseVersionNumbers(String) will throw a NFE on most malformed
> data, technically an IllegalArgumentException. Same thing would happen if
> you ran an invalid string through Runtime.Version.parse(String) (having
> NumberFormatException specified to be thrown there is perhaps redundant).
> So, wouldn't pre-8160000 behavior be more or less the same if someone
> specified an un-parseable version string during setup?
>
> Perhaps the test could verify that both parseVersionNumber(String) and
> Runtime.Version.parse(String) throws exceptions on the same input.
>

The problem is that they actually don't do that.
parseVersionNumber(String) happily accepts leading zeros or negative
numbers (actually everything that's parsed by Integer.parseInt()).
Handling all these cases in parseVersionNumber(String) would either
make the implementation quite complicated or bring us back to using a
regexp.

> Thanks!
>
> /Claes
>
>
>>
>> - I was a little bit surprised that I could reflectively access and
>> execute java.lang.VersionProps.parseVersionNumbers() where both the
>> class and the method are package private. Maybe this is related to
>> Jigsaw issue #ReflectiveAccessToNonExportedTypes [3]? As I'm not a
>> Jigsaw expert, I'd be graceful to anybody explaining me why this is
>> still so easily possible with Jigsaw?
>>
>> Thank you and best regards,
>> Volker
>>
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
>> [2] https://bugs.openjdk.java.net/browse/JDK-8160000
>> [3]
>> http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes
>>
>