Bug 17820 - fast/js/date-DST-time-cusps.html and fast/js/date-big-setdate.html fail when not in PST/PDT time zone
Summary: fast/js/date-DST-time-cusps.html and fast/js/date-big-setdate.html fail when ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: LayoutTestFailure
Depends on:
Blocks:
 
Reported: 2008-03-13 01:19 PDT by Robert Blaut
Modified: 2009-06-22 22:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 (5.22 KB, patch)
2009-06-08 23:00 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (1.75 KB, patch)
2009-06-08 23:03 PDT, Shinichiro Hamaji
eric: review+
Details | Formatted Diff | Diff
Patch v3 (1.80 KB, patch)
2009-06-11 21:43 PDT, Shinichiro Hamaji
eric: review+
Details | Formatted Diff | Diff
Another patch v1 (10.29 KB, patch)
2009-06-15 05:51 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Yet another patch v1 (10.28 KB, patch)
2009-06-15 15:47 PDT, Shinichiro Hamaji
ap: review-
Details | Formatted Diff | Diff
SunSpider results for "Another patch" (3.57 KB, text/plain)
2009-06-18 11:47 PDT, Shinichiro Hamaji
no flags Details
Yet another patch v2 (7.39 KB, patch)
2009-06-18 19:28 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Yet another patch v3 (7.39 KB, patch)
2009-06-18 19:32 PDT, Shinichiro Hamaji
ap: review+
Details | Formatted Diff | Diff
Yet another patch v4 (7.41 KB, patch)
2009-06-18 23:37 PDT, Shinichiro Hamaji
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Blaut 2008-03-13 01:19:38 PDT
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
Comment 1 Alexey Proskuryakov 2008-03-13 02:47:43 PDT
I believe these fail for all time zones but PST/PDT.
Comment 2 Adam Roben (:aroben) 2008-03-13 09:20:31 PDT
(In reply to comment #1)
> I believe these fail for all time zones but PST/PDT.

So true!

-Adam from EDT
Comment 3 Mark Rowe (bdash) 2008-03-13 11:24:04 PDT
See also bug 4930, bug 6547, and bug 11515.
Comment 4 Shinichiro Hamaji 2009-06-08 23:00:18 PDT
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(-)
Comment 5 Shinichiro Hamaji 2009-06-08 23:01:09 PDT
Comment on attachment 31083 [details]
Patch v1

Sorry, I created the patch in wrong way... Will upload again.
Comment 6 Shinichiro Hamaji 2009-06-08 23:03:34 PDT
Created attachment 31084 [details]
Patch v2

 WebKitTools/ChangeLog                |   13 +++++++++++++
 WebKitTools/Scripts/run-webkit-tests |    5 +++++
 2 files changed, 18 insertions(+), 0 deletions(-)
Comment 7 Shinichiro Hamaji 2009-06-08 23:04:49 PDT
Comment on attachment 31084 [details]
Patch v2

Uploaded again. This fixes run-webkit-tests on both Mac and Linux.
Comment 8 Mark Rowe (bdash) 2009-06-08 23:09:57 PDT
Comment on attachment 31084 [details]
Patch v2

Why're we changing the testing tool rather than fixing the tests?
Comment 9 Shinichiro Hamaji 2009-06-08 23:20:32 PDT
> 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.
Comment 10 Shinichiro Hamaji 2009-06-08 23:22:56 PDT
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...
Comment 11 Mark Rowe (bdash) 2009-06-08 23:26:22 PDT
(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.
Comment 12 Shinichiro Hamaji 2009-06-08 23:56:10 PDT
> 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.
Comment 13 Shinichiro Hamaji 2009-06-09 01:48:50 PDT
> 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!
Comment 14 Eric Seidel (no email) 2009-06-11 01:06:33 PDT
Comment on attachment 31084 [details]
Patch v2

seems like a nice simple solution to me! ;) r=me.
Comment 15 Eric Seidel (no email) 2009-06-11 17:33:10 PDT
ChangeLogs should have a link back to the original bug.
Comment 16 Eric Seidel (no email) 2009-06-11 19:00:37 PDT
Success!
Comment 17 Shinichiro Hamaji 2009-06-11 21:43:34 PDT
Created attachment 31191 [details]
Patch v3

 WebKitTools/ChangeLog                |   15 +++++++++++++++
 WebKitTools/Scripts/run-webkit-tests |    5 +++++
 2 files changed, 20 insertions(+), 0 deletions(-)
Comment 18 Shinichiro Hamaji 2009-06-11 21:44:25 PDT
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.
Comment 19 Eric Seidel (no email) 2009-06-12 02:22:12 PDT
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.) :)
Comment 20 Alexey Proskuryakov 2009-06-13 13:58:31 PDT
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?
Comment 21 Shinichiro Hamaji 2009-06-15 05:51:17 PDT
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(-)
Comment 22 Shinichiro Hamaji 2009-06-15 06:00:55 PDT
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,
Comment 23 Alexey Proskuryakov 2009-06-15 08:25:34 PDT
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.
Comment 24 Shinichiro Hamaji 2009-06-15 15:47:26 PDT
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(-)
Comment 25 Shinichiro Hamaji 2009-06-15 15:59:49 PDT
> 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?
Comment 26 Oliver Hunt 2009-06-18 01:19:00 PDT
Can you please remove the review? flag from patches that are being obsoleted? cheers
Comment 27 Shinichiro Hamaji 2009-06-18 02:09:35 PDT
Comment on attachment 31287 [details]
Another patch v1

Done.
Comment 28 Shinichiro Hamaji 2009-06-18 11:47:43 PDT
Created attachment 31503 [details]
SunSpider results for "Another patch"

Putting SunSpider results for "Another patch". It seems that the performance becomes worse significantly.
Comment 29 Alexey Proskuryakov 2009-06-18 12:10:58 PDT
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.
Comment 30 Shinichiro Hamaji 2009-06-18 19:28:56 PDT
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(-)
Comment 31 Shinichiro Hamaji 2009-06-18 19:32:12 PDT
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(-)
Comment 32 Shinichiro Hamaji 2009-06-18 19:35:52 PDT
(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!
Comment 33 Alexey Proskuryakov 2009-06-18 23:25:31 PDT
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.
Comment 34 Shinichiro Hamaji 2009-06-18 23:37:14 PDT
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(-)
Comment 35 Shinichiro Hamaji 2009-06-18 23:38:29 PDT
Comment on attachment 31534 [details]
Yet another patch v4

I modified the output messages. Thanks for the comments!
Comment 36 Alexey Proskuryakov 2009-06-18 23:44:13 PDT
Comment on attachment 31534 [details]
Yet another patch v4

r=me
Comment 37 Alexey Proskuryakov 2009-06-22 22:53:39 PDT
Committed revision 44975.