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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: 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)]

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]

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)]

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]

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)]

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]

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)]

Attachment: Run jit tests with US/Pacific timezone set

Comment 8

[Dirkjan Ochtman (:djc)]

Sorry, not sure what ate the filenames in the diff.

Comment 9

[Dirkjan Ochtman (:djc)]

Attachment: Same patch, correct filenames

Comment 10

[Paul Biggar]

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)]

Attachment: hg exported patch, tested against tracemonkey tip

Comment 12

[Dirkjan Ochtman (:djc)]

Note: the appropriate keyword appears to be checkin-needed, not commit-needed.

Comment 13

[Dirkjan Ochtman (:djc)]

Attachment: Unmangled version of the patch

Comment 14

[Paul Biggar]

http://hg.mozilla.org/tracemonkey/rev/e0288c977846

Comment 15

[Paul Biggar]

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)]

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]

Pushed to try:

http://tbpl.mozilla.org/?tree=Try&rev=6e4f37f16794

Comment 18

[Dirkjan Ochtman (:djc)]

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)]

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

[Rain [:rain1] (inactive)]

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

[Rain [:rain1] (inactive)]

Attachment: 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

[Rain [:rain1] (inactive)]

If this test fails, we need to deploy the hotfix in 
http://support.microsoft.com/kb/932590 to all Windows tinderbox machines.

Comment 23

[Rain [:rain1] (inactive)]

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]]

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

[Rain [:rain1] (inactive)]

Attachment: 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

[Rain [:rain1] (inactive)]

(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

[Rain [:rain1] (inactive)]

It's actually Windows 7 even though it says WINNT 5.2. Man. :(

Comment 28

[David Mandelin [:dmandelin]]

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 [inactive]]

Thanks!  I have my desktop machine set to California time in order to avoid this kind of annoyance...

Comment 30

[Rain [:rain1] (inactive)]

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

Comment 31

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c332505ca51f