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
