[threeten-dev] Hijrah Tests Review Request

[chand basha]

Hi Stephen,

Thanks for your quick feedback. Please see my comments inline.

Thanks,
- Basha

On 6/19/2013 9:48 AM, roger riggs wrote:
>
> On 6/19/2013 5:15 AM, Stephen Colebourne wrote:
>> The TCK test involves a lot of deletion. Was that deliberate?
> Since they had calendar data dependencies, they are not valid as TCK 
> tests.
> I asked that they be moved to the UmmAlQura tests.
>
> Roger
>
>>
>> The TestUmmAlQuraChronology class (non TCK) has date creation
>> indirectly via lines this this:
>> {1318, 01, 01,   1900, 04, 30},
>> but the "01" and "04" are octal numbers, not "normal" constants. Octal
>> numbers may look pretty for dates, but they don't work for "08" and
>> "09" and thus confuse users browsing this code (looking for example
>> API usage).
>>
>> Is it okay if the numbers are changed from "04" to "4" and so on? If 
>> not, I can change the code to have direct date creation.
>>
>> The comment sections (grouping similar tests) are incomplete. For
>> example. you could do with a dashed line or comment header at line 655
>> as the tests below there don't refer to the previous comment section
>> header at line 623. Alternately, remove all the comment sections.
>>
>> Agreed. Even though the names of the data provider and the test 
>> itself are obvious, having a simple comment would help in 
>> understanding the tests. So I will add comments appropriately.
>>
>> The tests themselves look pretty comprehensive, although I haven't
>> checked the actual dates or logic are correct.
>>
>> Stephen
>>
>>
>> On 18 June 2013 21:45, Chand Basha <chand.basha at oracle.com> wrote:
>>> Hi,
>>>
>>> I have added few new tests to TestUmmAlQuraChronology.java and
>>> removed/cleaned up existing TCKHijrahChronology.java files. Roger 
>>> graciously
>>> created a review for me
>>> http://cr.openjdk.java.net/~rriggs/webrev-hijrah-calendar-tests-8016898. 
>>>
>>> Thanks a lot Roger.
>>>
>>> I would appreciate if you could review it and let me know if there 
>>> are any
>>> questions/concerns to commit the files.
>>>
>>> Thanks,
>>> - Basha
>