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
