Consider testing `zoneinfo` with `tzdata` PyPI package installed

[sobolevn]

Feature or enhancement

Right now none of our build bots test zoneinfo with tzdata pypi package installed.
Proof: https://github.com/python/cpython/pull/99602/files#diff-7908b1930bd1135d6f8077ef5ff578b02577f5f7042b54be88b9b86b6c7af45dR27-R35

Link to the package: https://pypi.org/project/tzdata/

However, it works locally:

(.venv) ~/Desktop/cpython  issue-99508 ✔                                             
» pip install tzdata     
Collecting tzdata
  Using cached tzdata-2022.6-py2.py3-none-any.whl (338 kB)
Installing collected packages: tzdata
Successfully installed tzdata-2022.6

(.venv) ~/Desktop/cpython  issue-99508 ✔                                             
» python
Python 3.12.0a2+ (heads/main:a3360facba, Nov 19 2022, 14:10:54) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import importlib.metadata
>>> importlib.metadata.metadata("tzdata")
<importlib.metadata._adapters.Message object at 0x107d5df50>

But, presence / absence of this 3rd party package change the behavior of zoneinfo module: both C and Python versions of it.

So, we need to be sure that tests can check all combinations.

Risks

Right now we don't have 3rd party packages installed for tests.
This might cause quite a lot of problems: where / how / when to install it.
It also can affect test / buildbot stability, because pip sometimes just fails to install some packages and you have to retry.

Pitch

I think we should test with tzdata PyPI package installed on at least one buildbot / CI job.
The question is: how can we do it without compromising the CI setup and reliability?

Previous discussion

• PEP 615: Add zoneinfo module #84683 (comment)
• gh-96463: fix load_tzdata raising OSError, not `ZoneInfoNotFound… #99602
• ZoneInfo object raises OSError instead of ZoneInfoNotFoundError for timezone input larger than 255 characters #96463

CC @pganssle

[vstinner]

In general, I dislike that Python test suite depends on an external resources. It makes tests more error prone :-( Is it possible to mock the 3rd party tzdata package in tests?

[sobolevn]

In general, I dislike that Python test suite depends on an external resources.

Yes, me too. I guess this can also set a bad precedent: other 3rd-party libs can be added after that as well.

Is it possible to mock the 3rd party tzdata package in tests?

I was thinking about something similar. Maybe storing some old version of it in test_zoneinfo/ test package and run tests twice: with and without it.

No idea how hard it would be, because I've never seen what's inside tzdata package 🤔

[vstinner]

I'm not talking about an hypothetical issue. For 3 weeks, I'm getting tons of email because some buildbots fail to download data from unicode.org, and so test_unicodedata fails randomly. I qualify these issues as "network issue". It happens a lot.

Browse https://mail.python.org/archives/list/buildbot-status@python.org/ and search for "test_unicodedata".

Example: https://buildbot.python.org/all/#/builders/662/builds/960

test_normalization (test.test_unicodedata.NormalizationTest) ... 	fetching http://www.pythontest.net/unicode/13.0.0/NormalizationTest.txt ...
FAIL

======================================================================
FAIL: test_normalization (test.test_unicodedata.NormalizationTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/http/client.py", line 587, in _read_chunked
    value.append(self._safe_read(amt))
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/http/client.py", line 632, in _safe_read
    raise IncompleteRead(data, amt-len(data))
http.client.IncompleteRead: IncompleteRead(7631 bytes read, 561 more expected)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/test/test_unicodedata.py", line 343, in test_normalization
    testdata = open_urlresource(TESTDATAURL, encoding="utf-8",
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/test/support/__init__.py", line 596, in open_urlresource
    s = f.read()
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/gzip.py", line 301, in read
    return self._buffer.read(size)
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/_compression.py", line 118, in readall
    while data := self.read(sys.maxsize):
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/gzip.py", line 494, in read
    buf = self._fp.read(io.DEFAULT_BUFFER_SIZE)
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/gzip.py", line 88, in read
    return self.file.read(size)
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/http/client.py", line 459, in read
    return self._read_chunked(amt)
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/http/client.py", line 597, in _read_chunked
    raise IncompleteRead(b''.join(value))
http.client.IncompleteRead: IncompleteRead(0 bytes read)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.10.cstratak-fedora-rawhide-ppc64le.clang/build/Lib/test/test_unicodedata.py", line 349, in test_normalization
    self.fail(f"Could not retrieve {TESTDATAURL}")
AssertionError: Could not retrieve http://www.pythontest.net/unicode/13.0.0/NormalizationTest.txt

[FFY00]

As per my reply in #99602 (comment), we don't need the tzdata package. It exists as a way to support data lookup when system provided tzdata is not available. We don't actually need it to test the code that loads the tzdata that way, it can be mocked. The only reason to add it would be for an integration test, which I am under the impression it's not that common here, and I don't think it's really needed, especially considering the tzdata PyPI package just repackages the tzdata from IANA, the upstream, (https://github.com/python/tzdata/blob/master/update.py) and we already test with the system provided tzdata.

[hugovk]

PR #19909 added Misc/requirements-test.txt and installed it as part of .github/workflows/coverage.yml and .travis.yml.

However, PRs #25679 and #30257 removed those CI files, so Misc/requirements-test.txt is unused.

Should we install it as part of another GitHub Actions workflow? (Replies on this issue suggest not.) Should we remove it?

[hugovk]

Yes, let's remove Misc/requirements-test.txt. Please see PR #110240.

[FFY00]

@sobolevn do you think adding a test for zonoinfo._common.load_tzdata using mocks would be enough?

I think we should add a test for zonoinfo._common.load_tzdata with mocks regardless, so that it can still be tested when the tzdata package is missing. We can then also add a test using the tzdata package, if anyone thinks it is necessary — for me, it'd be a nice to have, but probably not worth the effort on the CI maintenance.