Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uses TZ = UTC in buildout #909

Open
wants to merge 1 commit into
base: 6.1
Choose a base branch
from
Open

Uses TZ = UTC in buildout #909

wants to merge 1 commit into from

Conversation

wesleybl
Copy link
Member

This ensures that tests using datetime will work locally on machines using TZ = GMT

It's the same thing that is in Plone 5.2 but was lost in Plone 6:

https://github.com/plone/buildout.coredev/blob/5.2/tests.cfg#L150-L158

@wesleybl
Copy link
Member Author

@jenkins-plone-org please run jobs

This ensures that tests using datetime will work locally on machines
using TZ = GMT
@davisagli
Copy link
Member

@wesleybl Which tests are failing due to the different timezone?

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should fix the test (which one?) to be more timezone robust, i.e by using freezegun, instead of setting all to UTC.

I think it was "lost" for a reason.

Also, to inject it globally creates and unexpected environment, not transparent to the test-author. If really needed, better do it in the test-fixture, where an author of a test would expect it

@wesleybl
Copy link
Member Author

@davisagli @jensens see the original discussion about this: #693

@davisagli
Copy link
Member

@wesleybl When I run the buildout.coredev tests currently on my machine in UTC-8, I don't get any timezone-related errors. I think we improved the tests that were previously timezone-dependent, and don't currently need this.

@davisagli
Copy link
Member

(Side note: on my M1 Mac, bin/test -j 4 completes in less than 6 minutes. Wow.)

@mauritsvanrees
Copy link
Member

The pure Python test script that I wrote in this comment on 5.2 is still correct: setting TZ in the environment has no effect on Python 3.8 and higher; you need to call time.tzset() afterwards.

plone.restapi master does the same in base.cfg and in testing.py.

Since plone.restapi has this, I doubt this is still needed to do in coredev. But @wesleybl you have not yet said which tests fail for you.

I dug up some history from the beginning of 2021 to explain the difference between this part of tests.cfg in Plone 5.2 and 6:

This all happened within a few days, but no one put enough dots together to use the same solution on both branches. :-/ Oh well, that has been three years ago, so let's laugh at it. :-)

As said, my guess is that this is not needed anymore because plone.restapi handles this for its tests. Jenkins is happy either way. So the question is still what test failure would be solved by this.

@wesleybl
Copy link
Member Author

@davisagli @mauritsvanrees when I run the ./bin/test -m plone.restapi command, I still have some errors. For example:

Failure in test test_transition_with_effective_date (plone.restapi.tests.test_workflow.TestWorkflowTransition.test_transition_with_effective_date)
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
  File "/home/user/git/buildout.coredev/src/plone.restapi/src/plone/restapi/tests/test_workflow.py", line 197, in test_transition_with_effective_date
    self.assertEqual(
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 1253, in assertMultiLineEqual
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python3.11/unittest/case.py", line 703, in fail
    raise self.failureException(msg)
AssertionError: '2018-06-24T09:17:00+00:00' != '2018-06-24T09:17:00-03:00'
- 2018-06-24T09:17:00+00:00
?                    ^ ^
+ 2018-06-24T09:17:00-03:00
?                    ^ ^



Failure in test test_statictime_full_history (plone.restapi.tests.test_statictime.TestStaticTime.test_statictime_full_history)
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
  File "/home/user/git/buildout.coredev/src/plone.restapi/src/plone/restapi/tests/test_statictime.py", line 242, in test_statictime_full_history
    self.assertEqual(
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 1079, in assertListEqual
    self.assertSequenceEqual(list1, list2, msg, seq_type=list)
  File "/usr/lib/python3.11/unittest/case.py", line 1061, in assertSequenceEqual
    self.fail(msg)
  File "/usr/lib/python3.11/unittest/case.py", line 703, in fail
    raise self.failureException(msg)
AssertionError: Lists differ: [Date[21 chars]0:00 GMT-3'), -612844200.0, DateTime('1950/07/[15 chars]-3')] != [Date[21 chars]0:00 UTC'), -612855000.0, DateTime('1950/07/31 19:30:00 UTC')]

First differing element 0:
DateTime('1950/07/31 17:30:00 GMT-3')
DateTime('1950/07/31 17:30:00 UTC')

- [DateTime('1950/07/31 17:30:00 GMT-3'),
?                                ^^ ^^

+ [DateTime('1950/07/31 17:30:00 UTC'),
?                                ^ ^

-  -612844200.0,
?       ^^^

+  -612855000.0,
?       ^^^

-  DateTime('1950/07/31 19:30:00 GMT-3')]
?                                ^^ ^^

+  DateTime('1950/07/31 19:30:00 UTC')]
?              


Failure in test test_statictime_lockinfo (plone.restapi.tests.test_statictime.TestStaticTime.test_statictime_lockinfo)
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
  File "/home/user/git/buildout.coredev/src/plone.restapi/src/plone/restapi/tests/test_statictime.py", line 278, in test_statictime_lockinfo
    self.assertEqual(-612858600.0, lock_infos[0]["time"])
  File "/usr/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.11/unittest/case.py", line 866, in _baseAssertEqual
    raise self.failureException(msg)
AssertionError: -612858600.0 != -612847800.0

Note that the use of the TZ environment variable in testing.py predates my first PR here. I commented out testing.py's setUp and tearDown methods and only got one more error:

Failure in test test_brain_summary_includes_all_metadata_fields (plone.restapi.tests.test_serializer_summary.TestSummarySerializers.test_brain_summary_includes_all_metadata_fields)
Traceback (most recent call last):
  File "/usr/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/usr/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
  File "/usr/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
  File "/home/user/git/buildout.coredev/src/plone.restapi/src/plone/restapi/tests/test_serializer_summary.py", line 144, in test_brain_summary_includes_all_metadata_fields
    self.assertLessEqual(
  File "/usr/lib/python3.11/unittest/case.py", line 1265, in assertLessEqual
    self.fail(self._formatMessage(msg, standardMsg))
  File "/usr/lib/python3.11/unittest/case.py", line 703, in fail
    raise self.failureException(msg)
AssertionError: dict_items([('@id', 'http://nohost/plone/doc1'), ('@type', 'DXTestDocument'), ('CreationDate', '2016-01-21T01:14:48+00:00'), ('Creator', 'test_user_1_'), ('Date', '2017-01-21T01:14:48+00:00'), ('Description', 'Description'), ('EffectiveDate', 'None'), ('ExpirationDate', 'None'), ('ModificationDate', '2017-01-21T01:14:48+00:00'), ('Subject', []), ('Title', 'Lorem Ipsum'), ('Type', 'DX Test Document'), ('UID', 'c6dcbd55ab2746e199cd4ed458000001'), ('author_name', None), ('cmf_uid', None), ('commentators', []), ('created', '2016-01-21T01:14:48+00:00'), ('description', 'Description'), ('effective', '1969-12-31T00:00:00+00:00'), ('end', None), ('exclude_from_nav', False), ('expires', '2499-12-31T00:00:00+00:00'), ('getIcon', None), ('getId', 'doc1'), ('getObjSize', '0 KB'), ('getPath', '/plone/doc1'), ('getRemoteUrl', None), ('getURL', 'http://nohost/plone/doc1'), ('id', 'doc1'), ('in_response_to', None), ('is_folderish', False), ('last_comment_date', None), ('listCreators', ['test_user_1_']), ('location', None), ('mime_type', 'text/plain'), ('modified', '2017-01-21T01:14:48+00:00'), ('portal_type', 'DXTestDocument'), ('review_state', 'private'), ('start', None), ('sync_uid', None), ('title', 'Lorem Ipsum'), ('total_comments', 0)]) not less than or equal to dict_items([('mime_type', 'text/plain'), ('@id', 'http://nohost/plone/doc1'), ('getId', 'doc1'), ('EffectiveDate', 'None'), ('description', 'Description'), ('portal_type', 'DXTestDocument'), ('exclude_from_nav', False), ('@type', 'DXTestDocument'), ('is_folderish', False), ('sync_uid', None), ('created', '2016-01-21T01:14:48+00:00'), ('commentators', []), ('in_response_to', None), ('title', 'Lorem Ipsum'), ('Creator', 'test_user_1_'), ('expires', '2499-12-31T03:00:00+00:00'), ('author_name', None), ('last_comment_date', None), ('total_comments', 0), ('getIcon', None), ('UID', 'c6dcbd55ab2746e199cd4ed458000001'), ('effective', '1969-12-31T03:00:00+00:00'), ('cmf_uid', None), ('Title', 'Lorem Ipsum'), ('getPath', '/plone/doc1'), ('listCreators', ['test_user_1_']), ('getObjSize', '0 KB'), ('start', None), ('Date', '2017-01-20T22:14:48-03:00'), ('Type', 'DX Test Document'), ('ModificationDate', '2017-01-20T22:14:48-03:00'), ('id', 'doc1'), ('type_title', 'DX Test Document'), ('end', None), ('getRemoteUrl', None), ('image_scales', None), ('getURL', 'http://nohost/plone/doc1'), ('Description', 'Description'), ('ExpirationDate', 'None'), ('location', None), ('nav_title', None), ('CreationDate', '2016-01-20T22:14:48-03:00'), ('modified', '2017-01-21T01:14:48+00:00'), ('Subject', []), ('review_state', 'private')])

So it seems like he's not solving all the problems.

Anyway, I think using TZ in buildout is more practical, because it will solve the problem in any package.

@davisagli
Copy link
Member

@wesleybl Interesting, I can reproduce your errors if I set my system timezone to America/Sao_Paulo, but not when it is set to America/Los_Angeles.

I think we should fix this in plone.restapi's tests. The goal is for them to be consistent regardless of what TZ environment variable is set. I guess there is some bug in https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/tests/statictime.py which tries to handle this already.

@jensens
Copy link
Member

jensens commented Mar 4, 2024

I guess there is some bug in https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/tests/statictime.py which tries to handle this already.

Looking at statictime.py's complexity: Why don't we use freezegun here?

@wesleybl
Copy link
Member Author

wesleybl commented Mar 4, 2024

I guess there is some bug in https://github.com/plone/plone.restapi/blob/main/src/plone/restapi/tests/statictime.py which tries to handle this already.

@davisagli I don't quite understand what StaticTime is but it seems to be very specific for documentation tests. I don't know how to use it for other tests.

Looking at statictime.py's complexity: Why don't we use freezegun here?

@jensens It appears that the freezegun was used in the past, but problems occurred. See:

https://github.com/plone/plone.restapi/blob/24f25d7632ed23ffd0087ba810657250242ddccb/src/plone/restapi/tests/statictime.py#L31C5-L35C44

I still think that using the environment variable is much simpler and would solve the problem in all packages, not just restapi.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants