RFR:JDK-8163330:HijrahDate aligned day of week incorrect

[Roger Riggs]

Hi,

Looks ok.

Typically, there is a space after the '//' comment characters, it makes 
it easier to read.
Also, in this case, I don't think the comments help.  "Monday" isn't 
important to the test
and the 'Any Other day' is still 1, not something else.

I would remove those comments.
Otherwise fine, no need for another webrev.

Thanks, Roger


On 10/12/2016 2:16 PM, Anubhav Meena wrote:
> Hi Roger,
>
> Thanks for the suggestion, have made the suggested changes and shifted 
> the tests to 
> /java/time/test/java/time/chrono/TestUmmAlQuraChronology.java.
>
> Updated webrev: 
> http://cr.openjdk.java.net/~rchamyal/anmeena/8163330/webrev.04/ 
> <http://cr.openjdk.java.net/%7Erchamyal/anmeena/8163330/webrev.04/>
>
> Thanks,
> Anubhav
>
>> On Oct 12, 2016, at 8:45 PM, Roger Riggs <Roger.Riggs at Oracle.com 
>> <mailto:Roger.Riggs at oracle.com>> wrote:
>>
>> Hi Anubhav,
>>
>> In general, I agree with Stephen that the tests should be testing an 
>> algorithm against facts.
>> Embedding an algorithm in the test increases the risk that the test 
>> will just replicate the
>> implementation code and therefor not be much of a test.
>>
>> Though in this case, the specification of aligned day of week is of a 
>> computation.
>> If the test were to independently compute the correct answer, it 
>> would be valid as a 'tck' test.
>>
>> Since the Hijrah calendar is data driven, the tests should be in 
>> /java/time/*test*/java/time/chrono/TestUmmAlQuraChronology.java.
>> Tests in java/time/tck/... should correspond directly to specified 
>> behaviors.
>> In this case, the algorithm is specified but the test is data 
>> dependent. (Perhaps a gray area).
>>
>> Thanks, Roger
>>
>>
>> On 10/12/2016 11:03 AM, Anubhav Meena wrote:
>>> Hi Stephen,
>>>
>>> Have incorporated the changes you suggested. Updated webrev is 
>>> available here
>>> http://cr.openjdk.java.net/~rchamyal/anmeena/8163330/webrev.03/ 
>>> <http://cr.openjdk.java.net/%7Erchamyal/anmeena/8163330/webrev.03/>
>>>
>>> Please review and suggest if anymore changes are required.
>>>
>>> Thanks,
>>> Anubhav
>>>
>>>> On Oct 12, 2016, at 3:21 PM, Anubhav Meena 
>>>> <anubhav.meena at oracle.com <mailto:anubhav.meena at oracle.com>> wrote:
>>>>
>>>> Gentle reminder.
>>>>
>>>>> On Oct 7, 2016, at 2:12 PM, Anubhav Meena 
>>>>> <anubhav.meena at oracle.com <mailto:anubhav.meena at oracle.com>> wrote:
>>>>>
>>>>> Hi all,
>>>>>
>>>>> Please review.
>>>>> Bug Id :https://bugs.openjdk.java.net/browse/JDK-8163330
>>>>>
>>>>> Issue:The HijrahDate class incorrectly calculates the 
>>>>> aligned-day-of-week field. It based the calculation on the 
>>>>> day-of-week, when it should be based on the day-of-month.
>>>>>
>>>>> Webrev:http://cr.openjdk.java.net/~rchamyal/anmeena/8163330/webrev.02/ 
>>>>> <http://cr.openjdk.java.net/%7Erchamyal/anmeena/8163330/webrev.02/>
>>>>> -- 
>>>>> Thanks and Regards,
>>>>> Anubhav
>>>>
>>>
>>
>