Bug 17820 – fast/js/date-DST-time-cusps.html and fast/js/date-big-setdate.html fail when not in PST/PDT time zone

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Robert Blaut]

Summary: 
fast/js/date-DST-time-cusps.html and fast/js/date-big-setdate.html test DST but I suspect that these test are only valid for US time zones. I'm not 100% sure. For me these both test fail (CET time zone). 

First diff:

--- /tmp/layout-test-results/fast/js/date-big-setdate-expected.txt	2008-03-13 07:49:47.000000000 +0100
+++ /tmp/layout-test-results/fast/js/date-big-setdate-actual.txt	2008-03-13 07:49:47.000000000 +0100
@@ -13,11 +13,11 @@
 PASS d.valueOf() - curValue is millisecondsPerDay
 PASS d.valueOf() - curValue is millisecondsPerDay
 PASS d.valueOf() - curValue is millisecondsPerDay
-PASS d.valueOf() - c.valueOf() is millisecondsPerDay - millisecondsPerHour
-PASS d.valueOf() - c.valueOf() is millisecondsPerDay - millisecondsPerHour
-PASS d.valueOf() - c.valueOf() is millisecondsPerDay - millisecondsPerHour
-PASS d.valueOf() - c.valueOf() is millisecondsPerDay - millisecondsPerHour
-PASS d.valueOf() - c.valueOf() is millisecondsPerDay - millisecondsPerHour
+FAIL d.valueOf() - c.valueOf() should be 82800000. Was 86400000.
+FAIL d.valueOf() - c.valueOf() should be 82800000. Was 86400000.
+FAIL d.valueOf() - c.valueOf() should be 82800000. Was 86400000.
+FAIL d.valueOf() - c.valueOf() should be 82800000. Was 86400000.
+FAIL d.valueOf() - c.valueOf() should be 82800000. Was 86400000.
 PASS successfullyParsed is true
 
 TEST COMPLETE

Second diff:

--- /tmp/layout-test-results/fast/js/date-DST-time-cusps-expected.txt	2008-03-13 07:49:47.000000000 +0100
+++ /tmp/layout-test-results/fast/js/date-DST-time-cusps-actual.txt	2008-03-13 07:49:47.000000000 +0100
@@ -3,10 +3,10 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS (new Date(1982, 2, 14, 2, 10)).getHours() is 1
-PASS (new Date(1982, 2, 14, 2)).getHours() is 1
-PASS (new Date(1982, 11, 7, 1, 10)).getTimezoneOffset() is 480
-PASS (new Date(1982, 11, 7, 1)).getTimezoneOffset() is 480
+FAIL (new Date(1982, 2, 14, 2, 10)).getHours() should be 1. Was 2.
+FAIL (new Date(1982, 2, 14, 2)).getHours() should be 1. Was 2.
+FAIL (new Date(1982, 11, 7, 1, 10)).getTimezoneOffset() should be 480. Was -60.
+FAIL (new Date(1982, 11, 7, 1)).getTimezoneOffset() should be 480. Was -60.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Alexey Proskuryakov]

I believe these fail for all time zones but PST/PDT.

[Adam Roben (:aroben)]

(In reply to comment #1)
> I believe these fail for all time zones but PST/PDT.

So true!

-Adam from EDT

[Mark Rowe (bdash)]

See also bug 4930, bug 6547, and bug 11515.

[Shinichiro Hamaji]

Created attachment 31083 [details]
Patch v1

 WebCore/ChangeLog                                |    9 +++++++++
 WebCore/platform/qt/QWebPopup.cpp                |    4 +++-
 WebCore/platform/text/qt/TextBreakIteratorQt.cpp |    1 +
 WebKit/qt/Api/qcookiejar.cpp                     |    2 +-
 WebKit/qt/Api/qwebnetworkinterface.cpp           |    1 +
 WebKit/qt/ChangeLog                              |   12 ++++++++++++
 WebKit/qt/Plugins/ICOHandler.cpp                 |    1 +
 WebKit/qt/WebCoreSupport/DragClientQt.cpp        |    1 +
 WebKit/qt/WebCoreSupport/EditCommandQt.cpp       |    1 +
 WebKitTools/ChangeLog                            |   13 +++++++++++++
 WebKitTools/Scripts/run-webkit-tests             |    5 +++++
 11 files changed, 48 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31083 [details]
Patch v1

Sorry, I created the patch in wrong way... Will upload again.

[Shinichiro Hamaji]

Created attachment 31084 [details]
Patch v2

 WebKitTools/ChangeLog                |   13 +++++++++++++
 WebKitTools/Scripts/run-webkit-tests |    5 +++++
 2 files changed, 18 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31084 [details]
Patch v2

Uploaded again. This fixes run-webkit-tests on both Mac and Linux.

[Mark Rowe (bdash)]

Comment on attachment 31084 [details]
Patch v2

Why're we changing the testing tool rather than fixing the tests?

[Shinichiro Hamaji]

> Why're we changing the testing tool rather than fixing the tests?
I'm not sure, but I think there are no standard way to set timezone from javascript. Another option would be adding way to set timezone to DumpRenderTree, say layoutTestController.setTimezone. But it will introduce discrepancies between DumpRenderTree and Safari which runs after layout tests run.

[Shinichiro Hamaji]

Hmm... I found that https://bugs.webkit.org/show_bug.cgi?id=4930 has similar patch but they say it fails for someone in Eastern Time. I will look into the issue if I can fix it...

[Mark Rowe (bdash)]

(In reply to comment #9)
> > Why're we changing the testing tool rather than fixing the tests?
> I'm not sure, but I think there are no standard way to set timezone from
> javascript. Another option would be adding way to set timezone to
> DumpRenderTree, say layoutTestController.setTimezone. But it will introduce
> discrepancies between DumpRenderTree and Safari which runs after layout tests
> run.

I meant fixing the tests so that they're not dependent on the time zone in which they're run, not explicitly set the timezone from within the test.

[Shinichiro Hamaji]

> I meant fixing the tests so that they're not dependent on the time zone in
> which they're run, not explicitly set the timezone from within the test.
Ah, I see. These failing tests exercise the behavior of DST and it would be difficult to make them portable.

[Shinichiro Hamaji]

> Hmm... I found that https://bugs.webkit.org/show_bug.cgi?id=4930 has similar
> patch but they say it fails for someone in Eastern Time. I will look into the
> issue if I can fix it...
Hmm... I couldn't reproduce the failures Adam met (I changed timezone of my mac by system preference). Adam, could you tell me what kind of failures you met? Thanks!

[Eric Seidel (no email)]

Comment on attachment 31084 [details]
Patch v2

seems like a nice simple solution to me! ;) r=me.

[Eric Seidel (no email)]

ChangeLogs should have a link back to the original bug.

[Eric Seidel (no email)]

Success!

[Shinichiro Hamaji]

Created attachment 31191 [details]
Patch v3

 WebKitTools/ChangeLog                |   15 +++++++++++++++
 WebKitTools/Scripts/run-webkit-tests |    5 +++++
 2 files changed, 20 insertions(+), 0 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31191 [details]
Patch v3

Thanks for the review! I added the URL of this bug in ChangeLog. Sorry for forgetting about this.

[Eric Seidel (no email)]

Comment on attachment 31191 [details]
Patch v3

No problem at all!  Sorry I've been slow about committing this.  I've actually been using this bug as a test bug while I work on fixing bug 26283, hence the accidental "Success!" comment which was added by my script during testing (it was supposed to go on bug 26301, but I used this bug number by mistake.) :)

[Alexey Proskuryakov]

Hrm, so this means that running tests while in a different time zone won't check that date/time machinery works correctly there? I think it's a bad change.

I want to revert this change - any objections?

[Shinichiro Hamaji]

Created attachment 31287 [details]
Another patch v1

 JavaScriptCore/ChangeLog                           |   16 ++++++++++
 JavaScriptCore/wtf/DateMath.cpp                    |   13 ++++++++-
 LayoutTests/ChangeLog                              |   13 ++++++++
 .../fast/js/resources/date-DST-time-cusps.js       |    7 ++++-
 LayoutTests/fast/js/resources/date-big-setdate.js  |    6 ++++
 WebKitTools/ChangeLog                              |   19 ++++++++++++
 .../DumpRenderTree/LayoutTestController.cpp        |   30 ++++++++++++++++++++
 WebKitTools/DumpRenderTree/LayoutTestController.h  |    3 ++
 8 files changed, 105 insertions(+), 2 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31287 [details]
Another patch v1

Alexey, thanks for your opinion. I think the patch wasn't submitted and we don't need to revert it.

I uploaded another patch, which is more complicated than the previous patch. This patch adds setTimezoneToPDT and setTimezoneToOriginal into layoutTestController and uses these functions in layout tests which exercise DST. This may make DateMath a bit slower to check if environment value TZ is changed. I'm not sure if it's better solution for this bug.

Anyway, I'm happy if webkit accepts either of my patches. As the DST related layout tests always fail in my timezone, I need to check the failures of date-* manually, which is annoying...

Thanks,

[Alexey Proskuryakov]

Interesting! Looks like this bug got marked RESOLVED/FIXED by mistake indeed. Reopening.

Did you measure SunSpider effect of this patch? It may affect performance, as you noted.

We have adjustments for time zone in other tests, see e.g. bug 4322, bug 5061. Could something like this be done here, as well?

Finally, I think that just disabling these two tests when running in other time zones would be acceptable, if nothing else works.

> As the DST related layout tests always fail in my timezone, 
> I need to check the failures of date-* manually, which is annoying...

I wholeheartedly agree - they always fail in my time zone, too, so I'll be really happy to see this fixed.

[Shinichiro Hamaji]

Created attachment 31315 [details]
Yet another patch v1

 LayoutTests/ChangeLog                              |   13 ++++
 .../fast/js/date-DST-time-cusps-expected.txt       |    5 +-
 LayoutTests/fast/js/date-big-setdate-expected.txt  |    6 +--
 .../fast/js/resources/date-DST-time-cusps.js       |   27 ++++++--
 LayoutTests/fast/js/resources/date-big-setdate.js  |   72 ++++++++++----------
 WebKitTools/ChangeLog                              |   12 +++
 .../DumpRenderTree/LayoutTestController.cpp        |   11 +++
 7 files changed, 95 insertions(+), 51 deletions(-)

[Shinichiro Hamaji]

> Did you measure SunSpider effect of this patch? It may affect performance, as
> you noted.
> 
> We have adjustments for time zone in other tests, see e.g. bug 4322, bug 5061.
> Could something like this be done here, as well?
> 
> Finally, I think that just disabling these two tests when running in other time
> zones would be acceptable, if nothing else works.

Thanks for the suggestions! I didn't measure the performance. I will do if webkit guys decide to accept the "Another patch". However, I think the your idea just disabling DST related tests for people whose timezone isn't PDT would be better. I uploaded yet another patch to do so. Could you take a look again?

[Oliver Hunt]

Can you please remove the review? flag from patches that are being obsoleted? cheers

[Shinichiro Hamaji]

Comment on attachment 31287 [details]
Another patch v1

Done.

[Shinichiro Hamaji]

Created attachment 31503 [details]
SunSpider results for "Another patch"

Putting SunSpider results for "Another patch". It seems that the performance becomes worse significantly.

[Alexey Proskuryakov]

Comment on attachment 31315 [details]
Yet another patch v1

> +        Skip layout tests for DST if the test runs not in PDT.

If my understanding is correct, PDT is daylight time, so these tests won't run anywhere for 6 months each year. What does tzname return in California in winter - should we perhaps permit PST, as well?

+        { "inTimezonePDT", inTimezonePDTCallback, kJSPropertyAttributeReadOnly | kJSPropertyAttributeDontDelete },

I think that you could get time zone name from Javascript - e.g. (new Date).toString() returns "Thu Jun 18 2009 23:08:56 GMT+0400 (MSD)" for me. The code for formatting dates is in formatTime() in DateConversion.cpp. Even if it's necessary to add a method to DumpRenderTree, I'd make it return the time zone name, not a boolean.

[Shinichiro Hamaji]

Created attachment 31529 [details]
Yet another patch v2

 LayoutTests/ChangeLog                              |   13 ++++
 .../fast/js/date-DST-time-cusps-expected.txt       |    5 +-
 LayoutTests/fast/js/date-big-setdate-expected.txt  |    6 +--
 .../fast/js/resources/date-DST-time-cusps.js       |   27 ++++++--
 LayoutTests/fast/js/resources/date-big-setdate.js  |   72 ++++++++++----------
 5 files changed, 72 insertions(+), 51 deletions(-)

[Shinichiro Hamaji]

Created attachment 31530 [details]
Yet another patch v3

 LayoutTests/ChangeLog                              |   13 ++++
 .../fast/js/date-DST-time-cusps-expected.txt       |    5 +-
 LayoutTests/fast/js/date-big-setdate-expected.txt  |    6 +--
 .../fast/js/resources/date-DST-time-cusps.js       |   27 ++++++--
 LayoutTests/fast/js/resources/date-big-setdate.js  |   72 ++++++++++----------
 5 files changed, 72 insertions(+), 51 deletions(-)

[Shinichiro Hamaji]

(In reply to comment #29)
> (From update of attachment 31315 [details] [review])
> > +        Skip layout tests for DST if the test runs not in PDT.
> 
> If my understanding is correct, PDT is daylight time, so these tests won't run
> anywhere for 6 months each year. What does tzname return in California in
> winter - should we perhaps permit PST, as well?

Sorry, my change description was wrong. I was using tzname[1] which is the name of timezone for DST. Anyway, I agree that we can check this from JavaScript, and I rewrote my patch not to change DumpRenderTree. Thanks for the review!

[Alexey Proskuryakov]

Comment on attachment 31530 [details]
Yet another patch v3

r=me

Maybe the tests could also be tweaked where they say "or skipped the tests if your timezone isn't PDT" to avoid similar confusion.

[Shinichiro Hamaji]

Created attachment 31534 [details]
Yet another patch v4

 LayoutTests/ChangeLog                              |   13 ++++
 .../fast/js/date-DST-time-cusps-expected.txt       |    5 +-
 LayoutTests/fast/js/date-big-setdate-expected.txt  |    6 +--
 .../fast/js/resources/date-DST-time-cusps.js       |   27 ++++++--
 LayoutTests/fast/js/resources/date-big-setdate.js  |   72 ++++++++++----------
 5 files changed, 72 insertions(+), 51 deletions(-)

[Shinichiro Hamaji]

Comment on attachment 31534 [details]
Yet another patch v4

I modified the output messages. Thanks for the comments!

[Alexey Proskuryakov]

Comment on attachment 31534 [details]
Yet another patch v4

r=me

[Alexey Proskuryakov]

Committed revision 44975.