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

[Claes Redestad]

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.

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
>