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

[Volker Simonis]

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/

1. Added a private 'check()' method to the VersionProps class which
ensures that every single part of a version number starts with a digit
greater zero (i.e. no leading zeros). If this condition isn't
fulfilled, an IllegalArgumentException will be thrown.
2. Check that the final list of version numbers returned by
VersionProps.parseVersionNumbers() has no leading or trailing zeros.
If not, throw an IllegalArgumentException.
3. With the changes 1. and 2. in place we can now check for bad input
in the regression test as well. The test now checks that
VersionProps.parseVersionNumbers() accepts and rejects the same
version strings as  Runtime.Version.parse(). So I think there's no
need for runtime assertions or configure checks any more.
4. Put a comment on VersionProps.parseVersionNumbers() to remind
others that the method is reflectively tests.

- The test copyright is OK. We already have a bunch of tests/files
with similar copyright.
- Wrapped the long lines.
- Moved test from test/java/lang/VersionProps.java to
test/java/lang/Runtime/Version/VersionProps.java
- I would prefer not to convert this test to TestNG for simplicity. I
like to be able to execute a test stand-alone in case of failure,
without any test framework. Moreover the test already prints what it's
doing anyway (see below) and in the case of failure it prints the
initial/expected version strings.

[1]
[1, 2]
[1, 2, 3]
[1, 2, 3, 4]
[1, 0, 0, 1]
[1, 10000, 1]
[1, 0, 2, 0, 0, 3, 0, 0, 0, 4, 5, 0, 0, 6]
[1000001]
[1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 9, 8, 7, 6, 5, 4, 3, 2, 1]
OK - caught bad version string 01
OK - caught bad version string 0.1.2
OK - caught bad version string 1.02.3
OK - caught bad version string 1.2.03.4
OK - caught bad version string 1.0.0.1.0
OK - caught bad version string 1.0.1.0.0
OK - caught bad version string 1.00.1
OK - caught bad version string 1.0.1.00
OK - caught bad version string 1.10000.

- Finally, I didn't understood the comment about 'List.of might be
preferred over Arrays.asList' so I didn't change that.

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
>