RFR(S): 8160564: TEST: Add a test to check the implementation of VersionProps.versionNumbers()
claes.redestad at oracle.com
Tue Jul 12 12:50:18 UTC 2016
On 2016-07-11 18:18, Volker Simonis wrote:
> here comes a new version of this change. I've tried to address most of
> the concerns/suggestions:
Looks good. As I'm currently obsessing about startup performance, I did
wish we could rely on (and fix, as a separate issue) the validation
done in build scripts, but there's no real harm in doing proper
validation in runtime as long as we avoid using regex here.
> - Finally, I didn't understood the comment about 'List.of might be
> preferred over Arrays.asList' so I didn't change that.
Oh, I meant that new List.of(...) can be used as a shorthand for
Arrays.asList(...) in the test. No real difference, just one less
import and a bit cleaner code in my opinion, so please ignore it at
> OK to push now (before it gets really over-engineered :)
> Thank you and best regards,
> On Thu, Jul 7, 2016 at 9:54 PM, Mandy Chung <mandy.chung at oracle.com> wrote:
>> 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.
>>> On Jul 7, 2016, at 6:59 AM, Volker Simonis <volker.simonis at gmail.com> wrote:
>>> can I please have a review for the following change which makes
>>> VersionProps.versionNumbers() testable and the corresponding
>>> regression test:
>>> 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 ). So we can currently only check that
>>> VersionProps.versionNumbers() accepts all kind of valid version
>>> - 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 ? 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,
>>>  http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/042058.html
>>>  https://bugs.openjdk.java.net/browse/JDK-8160000
>>>  http://openjdk.java.net/projects/jigsaw/spec/issues/#ReflectiveAccessToNonExportedTypes
More information about the core-libs-dev