Re: RFR(S): 8172695: (scanner) java/util/Scanner/ScanTest.java fails

Tue, 19 Mar 2019 15:22:40 -0700 

Hi Christoph,
I spoke with Naoto Sato (internationalization) and he recommends using Locale.US instead of Locale.ROOT. (Sorry for having misdirected you.) 


The test case in question, parsing float strings, relies on the decimal 
separator, which isn't defined in the ROOT locale. Of course it's "." in the US 
locale. Also, the US locale is the only one that's guaranteed to be available; 
see Locale::getAvailableLocales.



So I think your current changeset will be fine if Locale.ROOT is replaced with 
Locale.US. Assuming it passes test runs. :-)


s'marks


On 3/19/19 1:46 AM, Langer, Christoph wrote:

Hi Stuart,

thanks for your analysis of this.

So you are suggesting to use Locale.ROOT for this test unconditionally? I agree 
😊

The test coding as it is right now is not ready to support arbitrary locales, as the 
comment already suggests. Generally, the test probably needs some overhaul. The scanned 
Strings should for instance use the Locale's decimal separators instead of hard coding 
".". Maybe there are other shortcomings. But as long as this is not done, using 
Locale ROOT seems to be safe for all possible scenarios.

I've updated the webrev: http://cr.openjdk.java.net/~clanger/webrevs/8172695.1/

Are you ok with that? Brian also?

Thanks
Christoph



-----Original Message-----
From: Stuart Marks <stuart.ma...@oracle.com>
Sent: Dienstag, 19. März 2019 00:44
To: Langer, Christoph <christoph.lan...@sap.com>
Cc: Brian Burkhalter <brian.burkhal...@oracle.com>; core-libs-dev <core-
libs-...@openjdk.java.net>
Subject: Re: RFR(S): 8172695: (scanner) java/util/Scanner/ScanTest.java fails

Hi Christoph,

After looking at this for a little bit I realized that it's not necessary to
have a system particular default locale in order to get the test case to fail.
It's possible create a locale that causes test case to fail, like so:

      Locale.setDefault(new Locale("en", "DE"))
      Scanner sc = new Scanner("\u0f23.\u0f27");
      sc.nextFloat();
          ^ throws InputMismatchException

(This is because using "DE" as the locale's country causes texpects , instead of
. as the decimal separator.)

The test already saves the current default locale and attempts to find a
known
good locale for running the tests. The difficulty seems to be finding a "known
good" locale. The current check for the language of English doesn't work if
the
default is en_DE.

Instead, I'd suggest we use the root locale Locale.ROOT as the known good
locale
for running the tests. We could then do something like this:

      Locale reservedLocale = Locale.getDefault();
      try {
          Locale.setDefault(Locale.ROOT);
          // run all the tests
      } finally {
          // restore the default locale
          Locale.setDefault(reservedLocale);
      }

and dispense with the logic of searching the existing locales for one that we
think is suitable.

(Actually, since the test is being run in /othervm mode, it's not clear to me
that restoring the previous default locale is necessary. But that's kind of a
separate discussion. I'm ok with leaving the save/restore in place.)

s'marks

On 3/18/19 10:57 AM, Brian Burkhalter wrote:

Hi Christoph,

Not a locale expert here so you probably need to hear from someone else,

but this looks like a reasonable approach to me. One minor comment is to
perhaps use format() at lines 67-68 instead of println().


Thanks,

Brian


On Mar 18, 2019, at 7:51 AM, Langer, Christoph

<christoph.lan...@sap.com> wrote:


please review a fix for 8172695, where java/util/Scanner/ScanTest.java

fails. It reproducibly fails on one of our Mac servers, where the locale is set 
to
en_DE.


The test in its current form checks if the test VM is run with a supported

default Locale by just checking the language to be one of “en”, “zh”, “ko”,
“ja”. But it turns out, that also the country part seems to be important. I
suggest to change the test case to test whether the VM’s default locale is
one of a restricted set of locales and if it isn’t, it shall try to run with
“ENGLISH”.


I tested the set of US, UK, CHINA, KOREA, JAPAN and GERMANY to work

on my machine. Before pushing, I’d run it through the submit repo.


bug: https://bugs.openjdk.java.net/browse/JDK-8172695

<https://bugs.openjdk.java.net/browse/JDK-8172695>

webrev: http://cr.openjdk.java.net/~clanger/webrevs/8172695.0/

<http://cr.openjdk.java.net/~clanger/webrevs/8172695.0/>






















					Reply via email to