Hi Volker, Thanks for adding a new test for it.
Nit: can you wrap the long lines. As for the bad version, one possible change is to add assert Runtime.Version.parse(versionNumber) in parseVersionNumbers method and add -esa in @run tag. It’d be good to convert this to testng test where the dataprovider can show the input version and expected returned list. Mandy > On Jul 7, 2016, at 6:59 AM, Volker Simonis <volker.simo...@gmail.com> 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/ > 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. > > - 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