RFR(s): 8171958: Several tests from java/time/test/java/time/format requiring jdk.localedata for execution

[Naoto Sato]

Looks good to me.

Naoto

On 1/10/17 2:26 AM, Sergei Kovalev wrote:
> Hi Naoto,
>
> Thank you for review. I looked throw sources mode carefully and found
> some inconsistency you mentioned. I cleaned out all possible
> dependencies on jdk.localedata. Probably previously the cases passed
> because they doing only number formatting. However I agree that it
> meaningful to collect all localedata dependencies in single place.
>
> Could you please take a look at the updated version?
>
> http://cr.openjdk.java.net/~skovalev/8171958/webrev.02/
>
>
> 10.01.17 00:20, Naoto Sato wrote:
>> Hi Sergei,
>>
>> java.base only supports Locale.US locale (and its parents
>> Locale.ENGLISH/Locale.ROOT). I still see tests that use other locales,
>> such as Locale.UK, Locale.FRENCH (eg. in
>> TestDateTimeFormatterBuilder.java). Those test should also need to be
>> separated.
>>
>> Naoto
>>
>> On 1/9/17 1:45 AM, Sergei Kovalev wrote:
>>> Hi All,
>>>
>>> Re-sending request because I'm still looking for reviewers.
>>>
>>> In addition for TEST.properties modification I'd like to put citation
>>> from Jon G. email.
>>>
>>>> FWIW, I note your TEST.properties file looks malformed.  It has 2
>>>> modules entries (line 4, line 7), which by the standard rules of Java
>>>> properties files, means that "last one wins".
>>>>
>>>> line source
>>>> # Threeten test uses TestNG
>>>> TestNG.dirs = .
>>>> othervm.dirs = tck/java/time/chrono test/java/time/chrono
>>>> test/java/time/format
>>>> modules = jdk.localedata
>>>> lib.dirs = ../../lib/testlibrary
>>>> lib.build = jdk.testlibrary.RandomFactory
>>>> modules = java.base/java.time:open java.base/java.time.chrono:open
>>>> java.base/java.time.zone:open
>>> This mean that first modules declaration could be safely removed as I
>>> did.
>>>
>>>
>>> 28.12.16 17:20, Sergei Kovalev wrote:
>>>>
>>>> Hi Rachna,
>>>>
>>>> Thank you for the comment. I've reviewed usage of module declaration
>>>> in TEST.properties file and find that it could be removed without any
>>>> impact. In both cases (with original and modified TEST.properties
>>>> file) I get same result.
>>>>
>>>> Test results: passed: 142; failed: 27
>>>>
>>>> The reason of this behavior is the lack of jtreg header in test
>>>> sources. In case test source has no "@test" tag jtreg ignores all
>>>> properties that exists in TEST.properties file.
>>>>
>>>> I recreated the review by adding TEST.properties modification:
>>>> http://cr.openjdk.java.net/~skovalev/8171958/webrev.01/
>>>>
>>>>
>>>> 27.12.16 13:55, Rachna Goel wrote:
>>>>>
>>>>> Hi Sergei,
>>>>>
>>>>> Though I am not a reviewer, But I have one comment on this fix.
>>>>>
>>>>> test/java/time/TEST.properties declares "modules = jdk.localedata" ,
>>>>> so that all tests for java.time can have access to "jdk.localedata"
>>>>> module.
>>>>>
>>>>> If we are restricting usage of jdk.localedata module for different
>>>>> tests, then TEST.properties need to be updated as well.
>>>>>
>>>>> Thanks,
>>>>> Rachna
>>>>>
>>>>>
>>>>>
>>>>> On 26/12/16 8:27 PM, Sergei Kovalev wrote:
>>>>>> Hello Team,
>>>>>>
>>>>>> Please review below fix for tests.
>>>>>>
>>>>>> Bug ID: https://bugs.openjdk.java.net/browse/JDK-8171958
>>>>>> Web review: http://cr.openjdk.java.net/~skovalev/8171958/webrev.00/
>>>>>>
>>>>>> Issue: some tests fails in case of module limitation by
>>>>>> '--limit-module java.base' command line option.
>>>>>> Root cause: The tests uses locale data that stored in separate
>>>>>> resource file "jdk.localedata".
>>>>>> Solution: Add declaration of required module. In same cases a test
>>>>>> file contains many test items, part of which could be executed with
>>>>>> java.base module only. In this cases we can separate the items and
>>>>>> extract that items which depend on locale data and run them
>>>>>> individually. Therefore this proposal contains additional test files
>>>>>> where added "WithLocale" suffix which demonstrate dependency on
>>>>>> resources. Alternative solution could be add a required module
>>>>>> declaration "jdk.localedata" to all files. However this will lead to
>>>>>> unnecessary test coverage reduction.
>>>>>>
>>>>>
>>>>
>>>> --
>>>> With best regards,
>>>> Sergei
>>>
>