When running test-webkitpy locally (GMT +1), it fails on metered_stream_unittest. Setting the timezone to US West Coast makes the test pass. This behaviour is a regression, it was introduced in r112140: <http://trac.webkit.org/changeset/112140>
Created attachment 136488 [details] Patch
I can't believe I didn't think of this when I wrote these tests. Sorry! Note that there are regions of the world whose tz offset isn't an integral number of hours (e.g., India is +0530). You might be better off adjusting the local timezone to something consistent (GMT, US/Pacific, whatever), and using that. See http://stackoverflow.com/questions/1301493/python-timezones for an example).
Comment on attachment 136488 [details] Patch Let's remove it from the queue and check those exceptions.
Created attachment 136526 [details] Patch I'm now setting a fixed TZ besides calculating the offset. If you feel the offset is redundant now and we could add some hardcoded value for a given time zone, I'll change it
Comment on attachment 136526 [details] Patch I would probably stop using self.timeoffset and just use a hard-coded value -- since it's just extra complexity now -- but it's up to you.
(In reply to comment #5) > (From update of attachment 136526 [details]) > I would probably stop using self.timeoffset and just use a hard-coded value > -- since it's just extra complexity now -- but it's up to you. Weird. I removed the timezone offset calculations and restored the previous hardcoded values. After that, I realized that the tests seem to be ignoring the environment variable. I tried to debug it with pdb.set_trace(), and although I can confirm that os.environ['TZ'] is the one I set, time.tzname still returns my system's configuration. I'll continue checking it tomorrow: if that doesn't work, we would still fail in the cases you commented in comment #2. Do you recall anything in this tests configuration (or in the test runner) that could prevent time.tzset() from working?
(In reply to comment #6) > Do you recall anything in this tests configuration (or in the test runner) that > could prevent time.tzset() from working? Nope, we generally don't do anything with timezones. I can take a look at it if I get a chance.
(In reply to comment #7) > (In reply to comment #6) > > Do you recall anything in this tests configuration (or in the test runner) that > > could prevent time.tzset() from working? > > Nope, we generally don't do anything with timezones. I can take a look at > it if I get a chance. I took a look at this, and found that - if we update TZ envvar in test-webkitpy or the Tester, the unit test runs in the timezone we expect and works fine. - if we update the TZ envvar in the unit test, time.tzset() ignores it, and stays in my system's timezone. I checked what's going on with the environment, and it looks that in webkitpy/layout_tests/port/webkit.py, we run the tests with a copy of the environment (check _run_script method). Am I getting it right? Is there a way to modify the environment for a given test so time.tzset() gets it? Updating the TZ envvar on the Tester or the test-webkitpy script is trivial, but I'm afraid it could be forgotten there and cause issues in other tests later.
Just FTR, while this doesn't work: os.environ['TZ'] = 'UTC+8' time.tzset() if you do this: os.putenv('TZ', 'UTC+8') in the metered_stream_unittest, it works.
Created attachment 136747 [details] Patch
Sigh. Timezones bugs appear doomed to haunt me no matter what I do in my career ... I think one problem with your patch is that the format for the TZ variable is wrong; possibly "CMT" is unrecognized. When I changed that to something like "US/Eastern" it worked fine. However, there's a bigger problem, which is that this whole approach won't work on Windows, which doesn't have tzset() and won't look at a TZ environment variable. This leaves us with either trying to calculate what the tz offset is, as you did in your first patch, or to ignore the fields. The problem with calculating the tzoffset is that although you may know what the tz offset is *now*, it's difficult if not impossible to know what rule is in effect for a given time != now (in this case, 1/1/1970). So, while we could make this patch various levels of "sort of correct", making it 100% bulletproof would be really hard. I've uploaded a patch that just ignores the actual values for HH:MM in the timestamps; I expect that that should be good enough to catch gross mistakes, and we'll catch bugs like transposing HH and MM by just seeing the logs, should that ever come up, and this seems good enough to me.
Comment on attachment 136747 [details] Patch Alright, looks good. Thanks!
(In reply to comment #11) > Sigh. Timezones bugs appear doomed to haunt me no matter what I do in my career ... > > I think one problem with your patch is that the format for the TZ variable is wrong; possibly "CMT" is unrecognized. When I changed that to something like "US/Eastern" it worked fine. > Weird: I used the same format with os.environ and os.putenv, and one worked, the other one didn't. (Besides, when doing it from the interpreter, the format was also accepted). > However, there's a bigger problem, which is that this whole approach won't work on Windows, which doesn't have tzset() and won't look at a TZ environment variable. I agree with you, I haven't though about this before. > This leaves us with either trying to calculate what the tz offset is, as you did in your first patch, or to ignore the fields. The problem with calculating the tzoffset is that although you may know what the tz offset is *now*, it's difficult if not impossible to know what rule is in effect for a given time != now (in this case, 1/1/1970). > > So, while we could make this patch various levels of "sort of correct", making it 100% bulletproof would be really hard. > > I've uploaded a patch that just ignores the actual values for HH:MM in the timestamps; I expect that that should be good enough to catch gross mistakes, and we'll catch bugs like transposing HH and MM by just seeing the logs, should that ever come up, and this seems good enough to me. I totally agree with you, and I think the patch you wrote is good enough for what we need. Thanks!
Committed r114014: <http://trac.webkit.org/changeset/114014>