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

[Volker Simonis]

On Tue, Jul 12, 2016 at 2:50 PM, Claes Redestad
<claes.redestad at oracle.com> wrote:
>
>
> On 2016-07-11 18:18, Volker Simonis wrote:
>>
>> Hi,
>>
>> here comes a new version of this change. I've tried to address most of
>> the concerns/suggestions:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8160564.v1/
>>
>
> 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.
>

Yes, I think the new checks should be quite cheap.

>>
>> - 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
> this point.
>

Ah, I wasn't aware of the new method.

Thanks,
Volker

> Thanks!
>
> /Claes
>
>
>>
>> OK to push now (before it gets really over-engineered :)
>>
>> Thank you and best regards,
>> Volker
>>
>> 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.
>>>
>>> Mandy
>>>
>>>> On Jul 7, 2016, at 6:59 AM, Volker Simonis <volker.simonis at 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
>>>
>>>
>