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

[Sergei Kovalev]

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
>>

-- 
With best regards,
Sergei