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

Wed Mar 20 09:20:36 UTC 2019

Hi,

if it is only about parsing floats, why not use
NumberFormat.getInstance().parse(value)?

I did 
8205108: [testbug] Fix pattern matching in jstatd tests.
http://hg.openjdk.java.net/jdk-updates/jdk11u/rev/1637a4e50fc9?revcount=80
some while ago, because we had wrong float parsing on mac in our tests.

... just a hint to think about, you don't need to redo the change ...

Best regards,
  Goetz.

> -----Original Message-----
> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf Of
> Langer, Christoph
> Sent: Mittwoch, 20. März 2019 10:10
> To: Stuart Marks <stuart.marks at oracle.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>
> Subject: [CAUTION] RE: RFR(S): 8172695: (scanner)
> java/util/Scanner/ScanTest.java fails
> 
> Hi Stuart,
> 
> thanks for consulting with Naoto and providing the update.
> 
> So I'll run the fix through jdk-submit and our test system at SAP with Locale.US.
> Provided I don't see issues, I'll push it thereafter...
> 
> Best regards
> Christoph
> 
> > -----Original Message-----
> > From: Stuart Marks <stuart.marks at oracle.com>
> > Sent: Dienstag, 19. März 2019 23:22
> > To: Langer, Christoph <christoph.langer at sap.com>
> > Cc: Brian Burkhalter <brian.burkhalter at oracle.com>; core-libs-dev <core-
> > libs-dev at openjdk.java.net>
> > Subject: Re: RFR(S): 8172695: (scanner) java/util/Scanner/ScanTest.java fails
> >
> > 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.marks at oracle.com>
> > >> Sent: Dienstag, 19. März 2019 00:44
> > >> To: Langer, Christoph <christoph.langer at sap.com>
> > >> Cc: Brian Burkhalter <brian.burkhalter at oracle.com>; core-libs-dev <core-
> > >> libs-dev at 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.langer at 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/>