The default bug view has changed. See this FAQ.

Sunspider trace-test 'check-date-format-tofte.js' fails on computers not set to Pacific time

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Honza, Assigned: sid0)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86
Windows Vista
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
Attachment #479362 - Flags: review?(sayrer)
Attachment #479362 - Attachment is patch: true
Attachment #479362 - Attachment mime type: application/octet-stream → text/plain
Blocks: 514067
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

6 years ago
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.
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

6 years ago
Yes, that's what I meant. I believe that's what the patch should do (and is also the appropriate solution to bug 530974).
I stupidly attached a proposed patch to bug 530974 instead... I can attach it here again, but that might be pointless?

Comment 6

6 years ago
Probably best to move it here, and obsolete it there. Thanks for doing this. You can set r? to me.
Created attachment 532668 [details] [diff] [review]
Run jit tests with US/Pacific timezone set
Attachment #479362 - Attachment is obsolete: true
Attachment #532668 - Flags: review?(pbiggar)
Attachment #479362 - Flags: review?(sayrer)
Sorry, not sure what ate the filenames in the diff.
Created attachment 532669 [details] [diff] [review]
Same patch, correct filenames
Attachment #532668 - Attachment is obsolete: true
Attachment #532669 - Flags: review?(pbiggar)
Attachment #532668 - Flags: review?(pbiggar)

Comment 10

6 years ago
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).
Attachment #532668 - Flags: review+
Created attachment 532704 [details] [diff] [review]
hg exported patch, tested against tracemonkey tip
Attachment #532669 - Attachment is obsolete: true
Attachment #532669 - Flags: review?(pbiggar)
Note: the appropriate keyword appears to be checkin-needed, not commit-needed.
Keywords: checkin-needed
Created attachment 532901 [details]
Unmangled version of the patch
Attachment #532704 - Attachment is obsolete: true
Attachment #532901 - Flags: review+

Comment 14

6 years ago
http://hg.mozilla.org/tracemonkey/rev/e0288c977846
Keywords: checkin-needed
Whiteboard: [fixed-in-tracemonkey]

Comment 15

6 years ago
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 ...
Whiteboard: [fixed-in-tracemonkey]
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

6 years ago
Pushed to try:

http://tbpl.mozilla.org/?tree=Try&rev=6e4f37f16794
No, that didn't work either:

http://tinderbox.mozilla.org/showlog.cgi?log=Try/1305713824.1305718660.25009.gz
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.
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
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)
(Assignee)

Updated

5 years ago
Summary: Sunspider trace-test 'check-date-format-tofte.js' is failing (false negative) → Sunspider trace-test 'check-date-format-tofte.js' fails on computers not set to Pacific time
If this test fails, we need to deploy the hotfix in 
http://support.microsoft.com/kb/932590 to all Windows tinderbox machines.
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.
Attachment #638313 - Flags: review?(dmandelin)
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.
Attachment #638313 - Flags: review?(dmandelin) → review-
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.
Attachment #532901 - Attachment is obsolete: true
Attachment #638313 - Attachment is obsolete: true
Attachment #638446 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #638446 - Flags: review? → review?(dmandelin)
(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.
It's actually Windows 7 even though it says WINNT 5.2. Man. :(
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.
Attachment #638446 - Flags: review?(dmandelin) → review+
Thanks!  I have my desktop machine set to California time in order to avoid this kind of annoyance...
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
Assignee: general → sagarwal
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c332505ca51f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.