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

+1 Stephen On 12 October 2016 at 19:16, 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/8163

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

Thanks Roger, appreciate that. > On Oct 13, 2016, at 3:09 AM, Roger Riggs wrote: > > 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 te

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

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

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

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/ Thanks, Anubhav > On Oct 12, 2016, at 8:45 PM, Roger Rig

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

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

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

Hi Stephen, Have incorporated the changes you suggested. Updated webrev is available here http://cr.openjdk.java.net/~rchamyal/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 wrote: > > Gentle

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

The indentation in TCKHijrahChronology is not correct. In TCKHijrahChronology, it would be better to add the expected values to the provider ie two additional value for aligned week and aligned day: {1437, 9, 1, 1, 1} {1437, 9, 2, 1, 2} ... That way, the test relies on facts, not on a formula. S

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

Gentle reminder. > On Oct 7, 2016, at 2:12 PM, Anubhav Meena 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

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

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 d