harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Yu <junjie0...@gmail.com>
Subject Re: [jira] Created: (HARMONY-6207) [classlib][text] SimpleDateFormatTest.test_parseLjava_lang_StringLjava_text_ParsePosition would fail
Date Sat, 16 May 2009 15:36:32 GMT
I see. Thanks, Nathan.

2009/5/16 Nathan Beyer <ndbeyer@apache.org>

> On Fri, May 15, 2009 at 12:11 PM, Jim Yu <junjie0122@gmail.com> wrote:
> > 2009/5/15 Oliver Deakin <oliver.deakin@googlemail.com>
> >
> >> Jim Yu wrote:
> >>
> >>> Hi Oli,
> >>>
> >>> Thanks for your attention on this issue. My explanation as below. I
> hope
> >>> it
> >>> could help explain the reason:-)
> >>>
> >>> 2009/5/15 Oliver Deakin <oliver.deakin@googlemail.com>
> >>>
> >>>
> >>>
> >>>> Hi Jim,
> >>>>
> >>>> I just had a look at this JIRA and had a question about it. It sounds
> >>>> from
> >>>> your description that we are seeing a difference between ICU's and
> >>>> Harmony's
> >>>> implementation of some Calendar related classes. This seems to me like
> a
> >>>> bug
> >>>> in either our code or ICU's and I wasn't sure that fixing the tests
to
> >>>> pass
> >>>> was the right thing to do
> >>>>
> >>>>
> >>>
> >>>
> >>> The failing testcase is for SimpleDateFormat class but there is no bug
> of
> >>> this class for this testcase. The reason of why the testcase failed is
> >>> that
> >>> we used getTime method of GregorianCalendar instance to create a Date
> >>> instance as the expected value. However, there is a non-bug behavior
> >>> difference between GregorianCalendar instances of Harmony and ICU.(ICU
> >>> complies with newer version of CLDR while Harmony complies with older
> one
> >>> I
> >>> guess. Please correct me if there is something incorrect) So when the
> >>> testcase want to assert that the expected Date instance created by
> Harmony
> >>> equals the result Date instance created by ICU is true, it failed. In a
> >>> word, the testcase discovered a non-bug difference of GregorianCalendar
> >>> between Harmony and ICU other than a bug of SimpleDateFormat class.
> >>>
> >>>
> >>
> >> Ahh ok, that makes more sense now.
> >>
> >>
> >>>
> >>>> here. My thoughts were that we need to do one of:
> >>>> - raise a bug with ICU and fix the tests for now.
> >>>> - fix a bug in our GregorianCalendar class.
> >>>> - delegate from our GregorianCalendar class to ICU's version (why
> don't
> >>>> we
> >>>> do this already?).
> >>>>
> >>>> I may be missing something, so I thought I'd ask - it just seems that
> >>>> there
> >>>> is a bug/discrepancy here somewhere that needs to be fixed. I guess
my
> >>>> question is: why is this a fix to the tests and not the code? :)
> >>>>
> >>>>
> >>>
> >>>
> >>> Our implementation of SimpleDateFormat class is correct for this
> testcase,
> >>> so we don't need to make a fix to the code. BTW, to delegate from our
> >>> GregorianCalendar class to ICU's version is a work we need to do.
> (Maybe
> >>> there is a lot work to do as we need to delegate Calendar class as well
> >>> and
> >>> might need to update the testcase for them) But in this case, it is not
> >>> necessary as we can use Date instance directly to create expected value
> >>> other than via creating a GregorianCalendar instance first and calling
> >>> getTime after that.
> >>>
> >>>
> >>>
> >>
> >> Ok, so we can work around this difference for now. What do you think is
> the
> >> right thing to do for the future? Should we delegate Calendar
> >> responsibilities to icu?  I can see upsides and downsides - upsides are
> that
> >> we no longer need to maintain the code for the Calendar functions,
> downsides
> >> are that there may be some overheads of delegating through to icu which
> may
> >> cause performance degradation, and we will also need to test which
> >> implementation gives us the better performance overall.
> >>
> >
> > Currently, we have several classes which have already delegated the
> > implementation to ICU. E.g.  format relevant classes in text module and
> > timezone relevant classes in luni module. However, there are some classes
> > which can be delegated to icu but not done yet. E.g. Calendar relevant
> > classes. I'm not sure what the real reason is. Maybe we have investigated
> > the downsides for them and decided to leave them as they are. Or we have
> not
> > investigated the issue yet. If the fact is the later one, I will
> investigate
> > this issue after moving the remaining tests of text module out of the
> > exclude list(have moved some out, will keep working).
>
> It's the later - it hasn't been investigated.
>
> >
> >>
> >> Perhaps for now it is easier to leave the code as it is and address the
> >> issues above when we find a user/app that is affected by them.
> >>
> >
> > Agree.  I think there might be other classes which can be considered to
> > delegate to ICU. We can address them first and do the actual work when we
> > find something are blocked by them.
> >
> >>
> >> Regards,
> >> Oliver
> >>
> >>
> >>
> >>>
> >>>> Regards,
> >>>> Oliver
> >>>>
> >>>>
> >>>> Jim Yu (JIRA) wrote:
> >>>>
> >>>>
> >>>>
> >>>>> [classlib][text]
> >>>>>
> SimpleDateFormatTest.test_parseLjava_lang_StringLjava_text_ParsePosition
> >>>>> would fail
> >>>>>
> >>>>>
> >>>>>
> ----------------------------------------------------------------------------------------------------
> >>>>>
> >>>>>                Key: HARMONY-6207
> >>>>>                URL:
> https://issues.apache.org/jira/browse/HARMONY-6207
> >>>>>            Project: Harmony
> >>>>>         Issue Type: Test
> >>>>>         Components: Classlib
> >>>>>   Affects Versions: 5.0M9
> >>>>>           Reporter: Jim Yu
> >>>>>            Fix For: 5.0M10
> >>>>>
> >>>>>
> >>>>> Currently, the testcase
> >>>>>
> SimpleDateFormatTest.test_parseLjava_lang_StringLjava_text_ParsePosition
> >>>>> would fail. I've investigated the root cause of this failure and
> found
> >>>>> the
> >>>>> main reason is that the GregorianCalendar class used in the testcase
> is
> >>>>> implemented by Harmony itself not delegating to ICU. So when we
call
> >>>>> getTime
> >>>>> of GregorianCalendar to get an Date instance as the expected value,
> it
> >>>>> would
> >>>>> not equal to the one created by ICU as the result. E.g. the following
> >>>>> testcase [1] would fail while [2] can pass. So I use Date instances
> >>>>> directly
> >>>>> for these failing ones in my patch. [1] test.parse("yyy", "99",
new
> >>>>> GregorianCalendar(99, Calendar.JANUARY, 1)
> >>>>>               .getTime(), 0, 2);
> >>>>> [2] test.parse("yyy", "99", new
> com.ibm.icu.util.GregorianCalendar(99,
> >>>>> Calendar.JANUARY, 1)
> >>>>>               .getTime(), 0, 2);
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>> --
> >>>> Oliver Deakin
> >>>> Unless stated otherwise above:
> >>>> IBM United Kingdom Limited - Registered in England and Wales with
> number
> >>>> 741598. Registered office: PO Box 41, North Harbour, Portsmouth,
> >>>> Hampshire
> >>>> PO6 3AU
> >>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>>
> >>
> >> --
> >> Oliver Deakin
> >> Unless stated otherwise above:
> >> IBM United Kingdom Limited - Registered in England and Wales with number
> >> 741598. Registered office: PO Box 41, North Harbour, Portsmouth,
> Hampshire
> >> PO6 3AU
> >>
> >>
> >
> >
> > --
> > Best Regards,
> > Jim, Jun Jie Yu
> >
>



-- 
Best Regards,
Jim, Jun Jie Yu

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message