Closed Bug 1089745 Opened 8 years ago Closed 8 years ago

Fix jstest failures in non-PST timezones

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Outside the Pacific time zone, the following jstests have been perma-fail since forever:

    ecma_3/Date/15.9.5.5.js
    ecma_3/Date/15.9.5.6.js
    ecma_3/Date/15.9.5.7.js
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/Regress/regress-58116.js

This patch fixes them as follows:

(*) It adds a tz-pacific flag to the jsreftest harness. This is similar to the jit-test flag with the same name. Like the jit-test harness, it sets the TZ=PST8PDT environment variable.

(*) For the first two tests, I also had to change toLocaleDateString() to toLocaleDateString("en-US") to get a string that Date.parse can parse correctly.

I only changed tasks_unix.py. tasks_win.py seems to use worker threads so it's more complicated: updating os.environ could race with other threads.
Attachment #8512098 - Flags: review?(terrence)
Comment on attachment 8512098 [details] [diff] [review]
Patch

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

Looks good, but be sure to try-server first. We basically just write the test header verbatim into the jstests.list that reftests parse and I'm not sure how it will deal with random, unknown keywords. If it errors out on the new keyword, I'd also be fine just overriding TZ unconditionally.
Attachment #8512098 - Flags: review?(terrence) → review+
Attached patch Patch v2Splinter Review
You were right, the jsreftest harness wasn't happy. This patch just sets the environment variable in the main() function. A bit simpler and should hopefully also work on Windows/Cygwin this way.
Attachment #8512098 - Attachment is obsolete: true
Attachment #8512571 - Flags: review?(terrence)
Attachment #8512571 - Flags: review?(terrence) → review+
Any chance this might fix bug 610183?  If so, that would be great!

(I guess we'll find out this weekend.)
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Any chance this might fix bug 610183?  If so, that would be great!
> 
> (I guess we'll find out this weekend.)

I replied on IRC, but for others following along: this patch only affects the shell harness and jsreftests run in the browser on tinderbox.
https://hg.mozilla.org/mozilla-central/rev/99072302a3f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
/me wonders if he can now safely set the timezone on his Linux desktop machine to something other than Pacific time...
(In reply to Nicholas Nethercote [:njn] from comment #8)
> /me wonders if he can now safely set the timezone on his Linux desktop
> machine to something other than Pacific time...

Were you able to? In particular, is bug 515254 still relevant?
Flags: needinfo?(n.nethercote)
> > /me wonders if he can now safely set the timezone on his Linux desktop
> > machine to something other than Pacific time...
> 
> Were you able to? In particular, is bug 515254 still relevant?

I tried and got no failures, so I closed bug 515254. Thanks for the reminder.
Flags: needinfo?(n.nethercote)
Duplicate of this bug: 934238
You need to log in before you can comment on or make changes to this bug.