Last Comment Bug 600522 - Sunspider trace-test 'check-date-format-tofte.js' fails on computers not set to Pacific time
: Sunspider trace-test 'check-date-format-tofte.js' fails on computers not set ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla16
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 514067
  Show dependency treegraph
 
Reported: 2010-09-29 06:21 PDT by Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08
Modified: 2012-07-06 07:48 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First patch for the test (45.30 KB, patch)
2010-09-29 06:21 PDT, Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08
no flags Details | Diff | Splinter Review
Run jit tests with US/Pacific timezone set (294 bytes, patch)
2011-05-16 09:10 PDT, Dirkjan Ochtman (:djc)
paul.biggar: review+
Details | Diff | Splinter Review
Same patch, correct filenames (371 bytes, patch)
2011-05-16 09:17 PDT, Dirkjan Ochtman (:djc)
no flags Details | Diff | Splinter Review
hg exported patch, tested against tracemonkey tip (624 bytes, patch)
2011-05-16 12:14 PDT, Dirkjan Ochtman (:djc)
no flags Details | Diff | Splinter Review
Unmangled version of the patch (624 bytes, text/plain)
2011-05-17 01:54 PDT, Dirkjan Ochtman (:djc)
dirkjan: review+
Details
updated version of djc's patch (816 bytes, patch)
2012-07-02 05:02 PDT, Siddharth Agarwal [:sid0] (inactive)
dmandelin: review-
Details | Diff | Splinter Review
add a tz-pacific flag (4.64 KB, patch)
2012-07-02 12:04 PDT, Siddharth Agarwal [:sid0] (inactive)
dmandelin: review+
Details | Diff | Splinter Review

Description Jan Honza Odvarko [:Honza] PTO 07/23 - 08/08 2010-09-29 06:21:25 PDT
Created attachment 479362 [details] [diff] [review]
First patch for the test

Following Javascript trace-test is always failing for me:
\mozilla-trunk\js\src\trace-test\tests\sunspider\check-date-format-tofte.js

The test is checking date format.

It depends on the current timezone and failing.

I am attaching a patch that uses fixed GMT time zone.

Honza
Comment 1 Dirkjan Ochtman (:djc) 2011-05-16 04:36:07 PDT
Can this get into the tree, please? It applies cleanly to xulrunner-2.0.1 and fixes the one test failure I get with that tree (on amd64 Linux).
Comment 2 Paul Biggar 2011-05-16 05:36:50 PDT
I'm not sure this is the right fix, though it's a marginal point. Can you try to use LC_TIME like in bug 530974 comment 5. If that's not possible, then this patch is appropriate with a minor change, which is to use PDT (or whatever time zone was originally used) instead of GMT, so that the assertEq line isn't changed.
Comment 3 Dirkjan Ochtman (:djc) 2011-05-16 07:44:50 PDT
If LC_TIME is recommended per bug 530974 comment 5, could we just have the test script set that up? It would sure be nice if make check just did the right thing.
Comment 4 Paul Biggar 2011-05-16 07:53:34 PDT
Yes, that's what I meant. I believe that's what the patch should do (and is also the appropriate solution to bug 530974).
Comment 5 Dirkjan Ochtman (:djc) 2011-05-16 08:34:53 PDT
I stupidly attached a proposed patch to bug 530974 instead... I can attach it here again, but that might be pointless?
Comment 6 Paul Biggar 2011-05-16 08:58:14 PDT
Probably best to move it here, and obsolete it there. Thanks for doing this. You can set r? to me.
Comment 7 Dirkjan Ochtman (:djc) 2011-05-16 09:10:29 PDT
Created attachment 532668 [details] [diff] [review]
Run jit tests with US/Pacific timezone set
Comment 8 Dirkjan Ochtman (:djc) 2011-05-16 09:11:15 PDT
Sorry, not sure what ate the filenames in the diff.
Comment 9 Dirkjan Ochtman (:djc) 2011-05-16 09:17:37 PDT
Created attachment 532669 [details] [diff] [review]
Same patch, correct filenames
Comment 10 Paul Biggar 2011-05-16 09:28:12 PDT
Comment on attachment 532668 [details] [diff] [review]
Run jit tests with US/Pacific timezone set

Review of attachment 532668 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming that this works for you.

To get this committed, you should commit it locally (or using mercurial queues) and then attach the `hg export` as the patch. That will ensure your username etc stays with the commit. Then set the commit-needed keyword (or I can push it for you tomorrow morning GMT).
Comment 11 Dirkjan Ochtman (:djc) 2011-05-16 12:14:15 PDT
Created attachment 532704 [details] [diff] [review]
hg exported patch, tested against tracemonkey tip
Comment 12 Dirkjan Ochtman (:djc) 2011-05-16 12:16:45 PDT
Note: the appropriate keyword appears to be checkin-needed, not commit-needed.
Comment 13 Dirkjan Ochtman (:djc) 2011-05-17 01:54:01 PDT
Created attachment 532901 [details]
Unmangled version of the patch
Comment 15 Paul Biggar 2011-05-17 05:29:59 PDT
Backed out:

http://hg.mozilla.org/tracemonkey/rev/365061f013dd
http://hg.mozilla.org/tracemonkey/rev/8f29678aa3ac


Windows didn't like it.

TEST-UNEXPECTED-FAIL | jit_test.py                | e:\builds\moz2_slave\tm-w32-dbg-spidermonkey-nomethodjit\src\js\src\jit-test\tests\sunspider\check-date-format-tofte.js: e:\builds\moz2_slave\tm-w32-dbg-spidermonkey-nomethodjit\src\js\src\jit-test\tests\sunspider\check-date-format-tofte.js:302: Error: Assertion failed: got "2007-01-01Monday, January 01, 2007 1:11:11 ...
Comment 16 Dirkjan Ochtman (:djc) 2011-05-18 01:15:21 PDT
Okay, I think I have an alternate fix that might work on Windows:

diff --git a/js/src/jit-test/jit_test.py b/js/src/jit-test/jit_test.py
--- a/js/src/jit-test/jit_test.py
+++ b/js/src/jit-test/jit_test.py
@@ -163,6 +163,7 @@

 def run_test(test, lib_dir):
     env = os.environ.copy()
+    env['TZ'] = 'PST8PDT'
     if test.tmflags:
         env['TMFLAGS'] = test.tmflags
     cmd = get_test_cmd(test.path, test.jitflags, lib_dir)

It looks like Windows doesn't have the "friendly" names for timezones that are used for the Olson timezone database, but the PST8PDT value should actually refer to the same timezone, and is references in Windows documentation as a possible value for the TZ environment variable.

Can we use the try server to test this, or is there someone on the JS team who has a Windows setup to test with? This fix works on my Linux box just like the US/Pacific one, and a similar fix for jstests.py could solve the issues seen in bug 530974 comment 18.
Comment 17 Paul Biggar 2011-05-18 03:23:40 PDT
Pushed to try:

http://tbpl.mozilla.org/?tree=Try&rev=6e4f37f16794
Comment 18 Dirkjan Ochtman (:djc) 2011-05-18 04:57:16 PDT
No, that didn't work either:

http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305713824.1305718660.25009.gz
Comment 19 Chris Leary [:cdleary] (not checking bugmail) 2011-05-23 14:15:48 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/e0288c977846
http://hg.mozilla.org/mozilla-central/rev/365061f013dd (backout)
http://hg.mozilla.org/mozilla-central/rev/8f29678aa3ac (backout)
Note: not marking as fixed because last changeset is a backout.
Comment 20 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 04:26:46 PDT
Haha, oh man. Looks like djc's patch was correct (it works locally). The bug was in Windows itself.

http://support.microsoft.com/kb/932590
Comment 21 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 05:02:40 PDT
Created attachment 638313 [details] [diff] [review]
updated version of djc's patch

I pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=f8460be0df56 (the previous patches aren't relevant)
Comment 22 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 05:04:39 PDT
If this test fails, we need to deploy the hotfix in 
http://support.microsoft.com/kb/932590 to all Windows tinderbox machines.
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 07:25:31 PDT
Comment on attachment 638313 [details] [diff] [review]
updated version of djc's patch

Our Windows builders are now Win7 x64, which doesn't have this bug. Let's get this into the tree.
Comment 24 David Mandelin [:dmandelin] 2012-07-02 10:58:45 PDT
Comment on attachment 638313 [details] [diff] [review]
updated version of djc's patch

Review of attachment 638313 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we should force the timezone to PT for all tests. It would be better to add a metadata flag that does it for that test only (see js/src/jit-test/README). It could be named 'force-tz-pacific' or something like that.
Comment 25 Siddharth Agarwal [:sid0] (inactive) 2012-07-02 12:04:27 PDT
Created attachment 638446 [details] [diff] [review]
add a tz-pacific flag

OK (though I'm not sure adding a flag for something that shouldn't really break anything is that necessary). force-tz-pacific would break the indentation, so I went with tz-pacific instead.
Comment 26 Siddharth Agarwal [:sid0] (inactive) 2012-07-03 00:49:30 PDT
(In reply to Siddharth Agarwal [:sid0] from comment #23)
> Comment on attachment 638313 [details] [diff] [review]
> updated version of djc's patch
> 
> Our Windows builders are now Win7 x64, which doesn't have this bug.

Actually no, they're still Win2k3 -- but the test didn't fail, so I guess it made its way into an update somehow.
Comment 27 Siddharth Agarwal [:sid0] (inactive) 2012-07-03 03:38:04 PDT
It's actually Windows 7 even though it says WINNT 5.2. Man. :(
Comment 28 David Mandelin [:dmandelin] 2012-07-03 10:49:47 PDT
Comment on attachment 638446 [details] [diff] [review]
add a tz-pacific flag

Review of attachment 638446 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for bearing with my fussiness!

::: js/src/jit-test/jit_test.py
@@ +228,5 @@
>          run = run_cmd
> +
> +    env = os.environ.copy()
> +    if test.tz_pacific:
> +        env['TZ'] = 'PST8PDT'

You can change this block to 

    if test.tz_pacific:
       env = env.copy().update({ 'TZ': 'PST8PDT' })

for hot path perf bonus points, not that it's likely to matter.
Comment 29 Nicholas Nethercote [:njn] 2012-07-04 21:50:39 PDT
Thanks!  I have my desktop machine set to California time in order to avoid this kind of annoyance...
Comment 30 Siddharth Agarwal [:sid0] (inactive) 2012-07-05 22:12:08 PDT
I didn't bother doing that because the way it is right now makes it much easier to add more env flags later if we need to.

http://hg.mozilla.org/integration/mozilla-inbound/rev/c332505ca51f

Note You need to log in before you can comment on or make changes to this bug.