Bug 186612 - Ensure the tests are run with US/Pacific time zone
Summary: Ensure the tests are run with US/Pacific time zone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-13 17:37 PDT by Carlos Alberto Lopez Perez
Modified: 2020-10-16 05:34 PDT (History)
16 users (show)

See Also:


Attachments
Patch (1.34 KB, patch)
2018-06-13 17:41 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (13.04 MB, application/zip)
2018-06-14 14:33 PDT, EWS Watchlist
no flags Details
Patch (1.67 KB, patch)
2020-10-15 19:03 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2018-06-13 17:37:14 PDT
It seems that when you run layout tests on a machine not configured with US/Pacific timezone some tests fail, like:

Regressions: Unexpected text-only failures (6)
  storage/indexeddb/modern/date-basic-private.html [ Failure ]
  storage/indexeddb/modern/date-basic.html [ Failure ]
  storage/indexeddb/modern/get-keyrange-private.html [ Failure ]
  storage/indexeddb/modern/get-keyrange.html [ Failure ]
  storage/indexeddb/modern/idbobjectstore-delete-1-private.html [ Failure ]
  storage/indexeddb/modern/idbobjectstore-delete-1.html [ Failure ]


In order to fix this and avoid potential new issues related to this I propose to unconditionally ensure the tests are always run with an US/Pacific timezone.

The JavaScriptCore tests already do this after https://trac.webkit.org/changeset/173116
Comment 1 Carlos Alberto Lopez Perez 2018-06-13 17:41:29 PDT
Created attachment 342713 [details]
Patch
Comment 2 Alexey Proskuryakov 2018-06-13 22:15:45 PDT
Comment on attachment 342713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342713&action=review

> Tools/ChangeLog:8
> +        Unconditionally set TZ environment variable to US/Pacific for the test environment.

I don't think that we should do this. Tests need to run inside the browser too, so making them depend on a specific time zone would result in not making them resilient. Also, bug 137473 alludes to us getting some valuable testing from not forcing the time zone.
Comment 3 Carlos Alberto Lopez Perez 2018-06-14 02:41:14 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Comment on attachment 342713 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342713&action=review
> 
> > Tools/ChangeLog:8
> > +        Unconditionally set TZ environment variable to US/Pacific for the test environment.
> 
> I don't think that we should do this. Tests need to run inside the browser
> too, so making them depend on a specific time zone would result in not
> making them resilient.

Well... the status quo is that all the bots (at least the ones that Igalia maintain) run with an US/Pacific timezone by default to avoid this and other issues.

Also a big part of the WebKit contributors live on that timezone.

So it will happen tests will be created with expectations of running on that timezone (not on purpose, but it will happen).

And the only way of noticing this will be that a contributor not living in that timezone get bitten by this and report a bug (like I'm doing)


> Also, bug 137473 alludes to us getting some valuable
> testing from not forcing the time zone.

That is already 4 years old.

In any case this patch doesn't force the timezone.

It just changes the default from whatever the system has to US/Pacific.
Comment 4 EWS Watchlist 2018-06-14 14:33:18 PDT
Comment on attachment 342713 [details]
Patch

Attachment 342713 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8184693

New failing tests:
http/tests/misc/last-modified-parsing.html
Comment 5 EWS Watchlist 2018-06-14 14:33:29 PDT
Created attachment 342762 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 6 Alexey Proskuryakov 2018-06-14 23:28:45 PDT
> Also a big part of the WebKit contributors live on that timezone.

That's... weird. I certainly didn't on PST before moving to Apple, so what changed? How many tests are failing?

Even bots switch between PST and DST as appropriate.
Comment 7 Adrian Perez 2018-06-17 16:00:00 PDT
(In reply to Alexey Proskuryakov from comment #6)
> > Also a big part of the WebKit contributors live on that timezone.
> 
> That's... weird. I certainly didn't on PST before moving to Apple, so what
> changed? How many tests are failing?
> 
> Even bots switch between PST and DST as appropriate.

The issue is not PST vs. PST, but US/Pacific vs. any other timezone.

I think ideally it would be indeed desirable that tests would run
independently of the configured timezone, but certainly we're not
quite there yet, so the suggested patch looks like a good stopgap
measure to me.

(And even if tests in general ran independently of the timezone,
it looks to me for some of them it would be still needed to set
the a known timezone for a certain test, because it still seems
like a good idea that reference test results are fixed.)
Comment 8 Alexey Proskuryakov 2018-06-17 23:19:47 PDT
> The issue is not PST vs. PST, but US/Pacific vs. any other timezone.

DST and PST are different time zones too though.

> I think ideally it would be indeed desirable that tests would run
> independently of the configured timezone, but certainly we're not
> quite there yet, so the suggested patch looks like a good stopgap
> measure to me.

You are saying that moving in the opposite direction from ideal to the ideal is a stopgap measure. I don't see how that would help improve things.
Comment 9 Alexey Proskuryakov 2018-06-17 23:20:02 PDT
s/to the ideal//
Comment 10 Adrian Perez 2018-06-18 02:51:38 PDT
(In reply to Alexey Proskuryakov from comment #8)
> > The issue is not PST vs. PST, but US/Pacific vs. any other timezone.
> 
> DST and PST are different time zones too though.

Ouch, it seems I was very sleepy when I wrote this yesterday... In
my head I was thinking “DST vs. non-DST” (Daylight Savings Time).
Sorry, please forget about what I wrote there.
Comment 11 Alexey Proskuryakov 2018-06-18 03:04:51 PDT
Sorry, I did mean PDT vs. PST indeed.
Comment 12 Sam Sneddon [:gsnedders] 2020-10-08 14:42:00 PDT
These tests are _still_ failing for non-Pacific timezones. Clearly, things _aren't_ improving and those of us outside of Pacific-time have to deal with extra noise in our test results on a daily basis. By not even working around this, it's hurting almost all non-Apple contributors to WebKit, which really isn't great.

We should do one of two things here:

 * follow the approach already taken by JavaScriptCore to make these deterministic and leave fixing the tests for all timezones as future work (and preferably set up a bot to test things in a non-California-centric configuration!), or

 * mark these tests as flaky, given that's what they are.
Comment 13 Carlos Alberto Lopez Perez 2020-10-08 14:52:39 PDT
I still think forcing a US/Pacific timezone before starting the tests is the most pragmatic approach. The patch attached here should do that.
Comment 14 Michael Catanzaro 2020-10-08 15:02:34 PDT
Comment on attachment 342713 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=342713&action=review

> Tools/Scripts/webkitpy/port/base.py:948
> +        # Some tests fail if the time zone is not set to US/Pacific (<https://webkit.org/b/186612>)
> +        clean_env['TZ'] = 'US/Pacific'

Alexey's concern is reasonable. I would add a FIXME: to your comment to make it clear that this is a concession rather than desired behavior.

Of course it would be nicer if tests didn't depend on timezone, but fixing every test doesn't seem like a priority. I've never considered running tests locally to be a reasonable option due to very high number of local failures that don't occur on bots, and anything to reduce that seems like a good step towards making it practical to run tests locally.
Comment 15 Alexey Proskuryakov 2020-10-08 17:28:12 PDT
I'm no longer strongly opposed to this change. Although I still think that tests need to work properly in any time zone, they shouldn't expect PST/DST.
Comment 16 Carlos Alberto Lopez Perez 2020-10-15 18:59:43 PDT
(In reply to Alexey Proskuryakov from comment #15)
> I'm no longer strongly opposed to this change. Although I still think that
> tests need to work properly in any time zone, they shouldn't expect PST/DST.

I agree that tests should work in any timezone.

This is not a solution to fix those tests. Its only a workaround to make the issue less annoying to those not living in that timezone meanwhile the tests are not fixed.

I will add a FIXME comment pointing to this bug so eventually this can be removed if those tests are fixed.
Comment 17 Carlos Alberto Lopez Perez 2020-10-15 19:03:04 PDT
Created attachment 411521 [details]
Patch

patch for landing, test EWS first
Comment 18 EWS 2020-10-16 05:33:58 PDT
Committed r268581: <https://trac.webkit.org/changeset/268581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411521 [details].
Comment 19 Radar WebKit Bug Importer 2020-10-16 05:34:24 PDT
<rdar://problem/70374961>