Bug 83583 – test-webkitpy fails on metered_stream_unittest for non-US West Coast Time Zones

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Simon Pena]

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>

[Simon Pena]

Created attachment 136488 [details]
Patch

[Dirk Pranke]

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

[Simon Pena]

Comment on attachment 136488 [details]
Patch

Let's remove it from the queue and check those exceptions.

[Simon Pena]

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

[Dirk Pranke]

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.

[Simon Pena]

(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?

[Dirk Pranke]

(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.

[Simon Pena]

(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.

[Simon Pena]

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.

[Dirk Pranke]

Created attachment 136747 [details]
Patch

[Dirk Pranke]

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.

[Philippe Normand]

Comment on attachment 136747 [details]
Patch

Alright, looks good. Thanks!

[Simon Pena]

(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!

[Dirk Pranke]

Committed r114014: <http://trac.webkit.org/changeset/114014>