[Webkit-unassigned] [Bug 130123] Incorrect Date returned between 3.1.2034 and 31.12.2099

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=130123


Mark Lam <mark.lam at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #226583|review?                     |review-
               Flag|                            |




--- Comment #5 from Mark Lam <mark.lam at apple.com>  2014-03-13 10:25:57 PST ---
(From update of attachment 226583)
View in context: https://bugs.webkit.org/attachment.cgi?id=226583&action=review

Much better, but I think this still need some changes.  First of all, out of curiosity, how did you determine that the erroneous date range is between 3.1.2034 and 31.12.2099?  The reason I ask is because I would like to know the added regression test to test the appropriate range to ensure that things are correct.  The test that you added only tests one value and may not necessarily catch future regressions.

> LayoutTests/js/script-tests/date-2034-03-01.js:7
> +description(
> +"This tests new Date('03/01/2034') equals to 'Wed Mar 01 2034'."
> +);
> +
> +shouldBe("(new Date('03/01/2034')).getMonth()", "2");
> +shouldBe("(new Date('03/01/2034')).getDate()", "1");
> +shouldBe("(new Date('03/01/2034')).getFullYear()", "2034");

There is already a js/date-constructor.html test that test conversion values.  If you look in date-constructor.js, you’ll see mention of a regression test for another bug.  Instead of adding new test files, I suggest you add your test there as a regression test for this bug.

Also, the purpose of adding the regression test is to catch possible regressions in date parsing.  This bug was file because the date parsing actually worked for some date ranges but not for others.  Hence, I think the regression test that we add needs to test a range of dates instead of just doing one specific date.  Why don’t you do something like this to get more coverage:

=== BEGIN ===
// Regression test for ...
function () {
    var monthNames = [ “January”, “February”, “March”, ... “December” ];

    function testDate(year, month, date) {
        var success = true;
        var dateString = monthNames[month] + “ “ + date + “, “ + year;
        var dateObj = new Date(dateString);
        if (dateObj.getFullYear() != year) {
            shouldBe("(new Date(‘“ + dateString + “‘).)getFullYear()”, year);
            success = false;
        }
        if (dateObj.getMonth() != month) {
            shouldBe("(new Date(‘“ + dateString + “‘).)getMonth()”, month);
            success = false;
        }
        if (dateObj.getDate() != date) {
            shouldBe("(new Date(‘“ + dateString + “‘).)getDate()”, date);
            success = false;
        }
        return success;
    }

    var success = false;
    var year = 1000;
    var month = 0;
    var date = 1;
    while (year < 9999) {
        success ||= testDate(year, month, date);
        year += 37;
        month = (month + 7) % 12;
        date = (date + 13) % 28 + 1;
    }
    success ||= testDate(9999, month, date);

    if (success)
        debug(“PASS Dates from year 1000 to 9999 parsed correctly”);
} ();
=== END ===

Some details about why I suggest the above:
1. According to http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getFullYear, the full year ranges from 1000 to 9999.  I figured we could just test some dates scattered over that range of years.
2. To keep the dates varying, I incremented the year, month, day by primes.  For simplicity, I ignored dates >28 so that I don’t have to deal with leap years, and varying last date of the months.
3. If we specify a date as “03/01/2034”, I wonder if there are localization implications resulting in this sometimes meaning mm/dd/yyyy and sometimes meaning dd//mm/yyyy.  So, I suggested using a text string for the month so that it is not ambiguous.  However, I’m not sure if that would end up exercising the code path that we want to test in the fix.  Please check if this is appropriate.  If not, please address the localization issue if needed.
4. I came up with this test without knowing how you determine that the erroneous date range is between 3.1.2034 and 31.12.2099.  Is there anything special about that date range that we need to add additional tests?

Feel free to implement an alternative if you have a better suggestion.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.