[webkit-reviews] review denied: [Bug 130123] Incorrect Date returned between 3.1.2034 and 31.12.2099 : [Attachment 226583] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 13 10:25:34 PDT 2014


Mark Lam <mark.lam at apple.com> has denied Byungseon Shin <sun.shin at lge.com>'s
request for review:
Bug 130123: Incorrect Date returned between 3.1.2034 and 31.12.2099
https://bugs.webkit.org/show_bug.cgi?id=130123

Attachment 226583: Patch
https://bugs.webkit.org/attachment.cgi?id=226583&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
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.


More information about the webkit-reviews mailing list