I see. Thanks, Nathan. 2009/5/16 Nathan Beyer <[email protected]>
> On Fri, May 15, 2009 at 12:11 PM, Jim Yu <[email protected]> wrote: > > 2009/5/15 Oliver Deakin <[email protected]> > > > >> 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 <[email protected]> > >>> > >>> > >>> > >>>> 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
