From 3a7a1cd51310cb148a38b845dfd879793bcea07e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 17 Oct 2026 10:22:21 -0400 Subject: [PATCH] ci(util): fetch with requests and poll memory with psutil in the scripts (#1894) - util/ci_timing.py: fetch through a requests session; an HTTPError's status decides the retry and the 404 note, the Link header is parsed as requests parses it, and the token staying off a cross-host redirect is tested against loopback servers. ci-timing.yml installs requests and lxml. - util/update_python_versions.py: fetch() uses requests and parse_images() reads the runner-images table with markdown-it-py, so an escaped pipe stays in its cell; both import lazily, so `publish` still runs on a bare python3. The detect job installs both. - examples/generators/pcapng.py: download with requests, keeping the retry, Retry-After and offline fallback; a timeout mid-body is still retried. - util/vendor_test_gate.py: sample a run's process group with psutil, and with ps where psutil is not installed. - util/ci_timing.py parse_junit() and util/pyshark_encap_map.py's PDML read: lxml with a hardened parser instead of ElementTree, no fallback; the tests that parse are gated on HAS_LXML. - pyproject.toml: the test extra carries psutil (CPython only), markdown-it-py and lxml (not on GraalPy). - Pipfile [dev-packages]: the same, with the test extra's markers, pinned by a test. Closes #1894 --- .github/workflows/ci-timing.yml | 6 +- .github/workflows/cron-python-versions.yml | 8 +- .github/workflows/cron-vendor.yml | 5 +- .github/workflows/unit-tests.yml | 4 +- Pipfile | 4 + examples/generators/pcapng.py | 57 ++-- pyproject.toml | 16 ++ tests/_dependency_gates.py | 9 + tests/project/test_ci_timing.py | 245 ++++++++++++++-- .../test_pcapng_download_retry_1658_unit.py | 272 +++++++++++++----- tests/project/test_pyshark_encap_map.py | 107 +++++++ .../test_python_versions_update_1660.py | 132 ++++++++- .../project/test_script_dependencies_1894.py | 88 ++++++ tests/project/test_vendor_test_gate_1853.py | 117 +++++++- .../test_vendor_reg_apptype_generator_unit.py | 2 +- util/ci_timing.py | 91 ++++-- util/pyshark_encap_map.py | 34 ++- util/update_python_versions.py | 74 +++-- util/vendor_test_gate.py | 57 +++- 19 files changed, 1153 insertions(+), 175 deletions(-) create mode 100644 tests/project/test_script_dependencies_1894.py diff --git a/.github/workflows/ci-timing.yml b/.github/workflows/ci-timing.yml index 19f4a0aa10..a8e2a6bf35 100644 --- a/.github/workflows/ci-timing.yml +++ b/.github/workflows/ci-timing.yml @@ -54,11 +54,15 @@ jobs: id: python-versions run: python3 util/python_versions.py --github-output - # Nothing is installed: the script is standard library only. - uses: actions/setup-python@v7 with: python-version: ${{ steps.python-versions.outputs.latest }} + # The script fetches with requests and parses the JUnit artifacts with lxml + # (#1894), and needs nothing else outside the standard library. + - name: Install requests and lxml + run: python -m pip install requests lxml + # The newest earlier run that uploaded a report, so a week whose run failed # on a HARD threshold is still next week's baseline. A first run, or one # after 90 days without a report, has none and says so. diff --git a/.github/workflows/cron-python-versions.yml b/.github/workflows/cron-python-versions.yml index 9ed06b926b..ec2880046e 100644 --- a/.github/workflows/cron-python-versions.yml +++ b/.github/workflows/cron-python-versions.yml @@ -57,9 +57,11 @@ jobs: python-version: ${{ steps.python-versions.outputs.latest }} # PyYAML, from the `test` extra, for the workflow readers and the audit; - # neither imports pcapkit. - - name: Install PyYAML - run: python -m pip install PyYAML + # neither imports pcapkit. requests fetches the sources and markdown-it-py + # reads the runner-images table (#1894); only `update` imports either, so + # `pr`'s `publish` runs without them. + - name: Install PyYAML, requests and markdown-it-py + run: python -m pip install PyYAML requests markdown-it-py # Fetches the sources, rewrites the files, and writes the proposal to # $RUNNER_TEMP/proposal. `publish` is false when nothing changed, or when diff --git a/.github/workflows/cron-vendor.yml b/.github/workflows/cron-vendor.yml index c2b1141202..67b6ed764e 100644 --- a/.github/workflows/cron-vendor.yml +++ b/.github/workflows/cron-vendor.yml @@ -264,8 +264,9 @@ jobs: # zero once it has decided; if this step fails or times out instead, the # tree may hold modules no run passed with, and the next step discards # every regenerated module. The test extra is installed here because only - # this step needs it. The script's budget bounds its runs together, and - # `timeout-minutes` sits above it as the backstop. Run serially, pytest + # this step needs it; it carries psutil too, which the script samples a + # run's memory with (#1894). The script's budget bounds its runs together, + # and `timeout-minutes` sits above it as the backstop. Run serially, pytest # peaked at 0.7 GB resident over tests/const, measured locally, so the # memory cap leaves headroom on this runner; files are capped at 2 GiB, as # in experimental.yml. diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index fbcfe482ad..1b578deef5 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -472,8 +472,8 @@ jobs: # The Scapy extra is there for examples/generators/pcap.py and legacy.py, # which build their captures with scapy; examples/generators/pcapng.py needs - # nothing beyond the standard library and pcapkit itself, which it uses to - # parse each fixture back. DPKT joins it so the dpkt-gated methods + # requests (#1894), from the test extra, and pcapkit itself, which it uses + # to parse each fixture back. DPKT joins it so the dpkt-gated methods # living under tests/integration/ (test_engine_parity.py, # test_engine_runtime.py) actually run here instead of skipping -- see # #729; same reasoning as the `test` job above. diff --git a/Pipfile b/Pipfile index 952ad17747..e4a690f787 100644 --- a/Pipfile +++ b/Pipfile @@ -44,6 +44,10 @@ libpcap = {version = ">=1.11.0b29",markers = "python_version >= '3.10'"} pypcapfile = "*" beautifulsoup4 = {extras = ["html5lib"],version = "*"} requests = {extras = ["socks"],version = "*"} +# The markers match the test extra in pyproject.toml (#1894). +psutil = {version = "*",markers = "platform_python_implementation == 'CPython'"} +lxml = {version = "*",markers = "platform_python_implementation != 'GraalVM'"} +markdown-it-py = "*" autopep8 = "*" bandit = "*" isort = "*" diff --git a/examples/generators/pcapng.py b/examples/generators/pcapng.py index dd206f19be..5e003954af 100644 --- a/examples/generators/pcapng.py +++ b/examples/generators/pcapng.py @@ -6,13 +6,14 @@ out of the repository. This module puts them back on any machine, without a checkout of anything private, by one of these routes: -* **Downloaded** -- fetched from a public source, with the source URL and the - SHA-256 of the exact bytes pinned below. The digest is checked after every - download and a mismatch is a hard error, never a warning. An HTTP 429 or - 5xx, or a timeout, is retried a few times with backoff (see - :func:`_download`). If the network is still unavailable the download - degrades to a synthesised stand-in, and both the console output and the - return value say so rather than pretending otherwise. +* **Downloaded** -- fetched from a public source with :mod:`requests`, with the + source URL and the SHA-256 of the exact bytes pinned below. The digest is + checked after every download and a mismatch is a hard error, never a warning. + An HTTP 429 or 5xx, or a timeout, is retried a few times with backoff (see + :func:`_download`). If the network is still unavailable, or :mod:`requests` + is not installed, the download degrades to a synthesised stand-in, and both + the console output and the return value say so rather than pretending + otherwise. * **Synthesised** -- written byte by byte by :class:`_Blocks` below, with no network access whatsoever, from packet payloads carried in the committed ``examples/captures/dhcp.pcapng`` fixture. Generation is deterministic: the same input @@ -162,8 +163,6 @@ import struct import sys import time -import urllib.error -import urllib.request from typing import TYPE_CHECKING, NamedTuple if TYPE_CHECKING: @@ -836,11 +835,22 @@ def _transient(exc: 'OSError') -> 'bool': server's final answer, and an unreachable host fails at once rather than after a wait, which keeps an offline run as quick as it was. + :mod:`requests` raises a :exc:`~requests.Timeout` for a timeout while it + connects or waits for the headers, but a timeout while the body is read + reaches here as a :exc:`~requests.ConnectionError` wrapping urllib3's + :exc:`~urllib3.exceptions.ReadTimeoutError`. + """ - if isinstance(exc, urllib.error.HTTPError): - return exc.code == 429 or 500 <= exc.code <= 599 - reason = exc.reason if isinstance(exc, urllib.error.URLError) else exc - return isinstance(reason, (socket.timeout, TimeoutError)) + import requests # pylint: disable=import-outside-toplevel + import urllib3 # pylint: disable=import-outside-toplevel + + if isinstance(exc, requests.HTTPError): + code = getattr(exc.response, 'status_code', None) + return code is not None and (code == 429 or 500 <= code <= 599) + if isinstance(exc, (requests.Timeout, socket.timeout, TimeoutError)): + return True + reason = exc.args[0] if isinstance(exc, requests.ConnectionError) and exc.args else None + return isinstance(reason, urllib3.exceptions.ReadTimeoutError) def _retry_after(exc: 'OSError', now: 'float') -> 'Optional[float]': @@ -857,8 +867,8 @@ def _retry_after(exc: 'OSError', now: 'float') -> 'Optional[float]': The wait, or :data:`None` if there is no header or it does not parse. """ - headers = getattr(exc, 'headers', None) - value = None if headers is None else headers.get('Retry-After') + response = getattr(exc, 'response', None) + value = None if response is None else response.headers.get('Retry-After') if value is None: return None value = value.strip() @@ -893,14 +903,19 @@ def _download(url: 'str') -> 'bytes': Raises: OSError: The last failure, once the retries are over or for a failure that is not retried -- :func:`_materialise` then falls back to a - stand-in (#1658). + stand-in (#1658). Every :exc:`requests.RequestException` is one. + ImportError: :mod:`requests` is not installed, which + :func:`_materialise` treats as it does an unreachable host. """ + import requests # pylint: disable=import-outside-toplevel + attempt, waited = 1, 0.0 while True: try: - with urllib.request.urlopen(url, timeout=TIMEOUT) as response: # nosec: B310 - return response.read() + response = requests.get(url, timeout=TIMEOUT) + response.raise_for_status() + return response.content except OSError as exc: if attempt >= DOWNLOAD_ATTEMPTS or not _transient(exc): raise @@ -946,8 +961,10 @@ def _materialise(fixture: '_Fixture', path: 'pathlib.Path') -> 'tuple[bytes, str try: data = _download(fixture.url) - except (urllib.error.URLError, OSError, ValueError) as exc: - print(f' ! {fixture.url} is unreachable ({exc});') + except (ImportError, OSError, ValueError) as exc: + why = ('cannot be fetched without requests' if isinstance(exc, ImportError) + else 'is unreachable') + print(f' ! {fixture.url} {why} ({exc});') print(f' ! synthesising a stand-in for {fixture.name} instead -- the ' 'upstream capture was NOT used') return fixture.build(), 'synthesised (download unavailable)' diff --git a/pyproject.toml b/pyproject.toml index 4e8ac0263e..71834efd49 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -363,6 +363,22 @@ test = [ # Test-only, as ``pcapkit`` never imports ``yaml``. Unpinned, since # ``safe_load`` is all they use. "PyYAML", + # What util/vendor_test_gate.py samples a test run's memory with (#1894), + # here because cron-vendor.yml's gate step installs this extra and nothing + # else. CPython only: PyPI has wheels for every CPython leg, free-threaded + # 3.14t included, and none for PyPy or GraalPy, where the gate falls back to + # ``ps``. Unpinned, like the entries above. + "psutil; platform_python_implementation == 'CPython'", + # What util/update_python_versions.py reads the runner-images README's table + # with (#1894), for the tests under ``tests/project/`` that run it on the + # saved README. A pure-Python wheel; unpinned, like the entries above. + "markdown-it-py", + # The strict XML parser util/ci_timing.py reads JUnit with and + # util/pyshark_encap_map.py reads PDML with (#1894). PyPI has wheels for + # CPython and PyPy but none for GraalPy, which reports its implementation as + # ``GraalVM``; the tests that parse skip there, and the scripts do not fall + # back to a laxer parser. Unpinned, like the entries above. + "lxml; platform_python_implementation != 'GraalVM'", ] # setuptools specific diff --git a/tests/_dependency_gates.py b/tests/_dependency_gates.py index d4558df935..f2f5f7308c 100644 --- a/tests/_dependency_gates.py +++ b/tests/_dependency_gates.py @@ -250,6 +250,15 @@ # The theme ``HAS_FURO`` asks for, whose table wrapper the #1861 transform # test runs. Only the ``docs`` extra carries it. 'furo': ('furo',), + # The strict XML parser ``HAS_LXML`` asks for (#1894): util/ci_timing.py + # reads JUnit and util/pyshark_encap_map.py reads PDML with it, and neither + # falls back to a laxer parser. The ``test`` extra carries it with a + # ``platform_python_implementation != 'GraalVM'`` marker, because PyPI has + # no wheel for GraalPy, so the gate skips on experimental.yml's GraalPy leg. + # That is no :data:`DEPENDENCY_GATE_EXCLUSIONS` entry: this module does not + # model markers and does not read experimental.yml, so every job it reads + # satisfies the gate, and an entry would describe a gap that is not there. + 'lxml': ('lxml',), } #: Flags whose condition is not a distribution, with what it is instead. A flag diff --git a/tests/project/test_ci_timing.py b/tests/project/test_ci_timing.py index a980f427d5..609755c5e8 100644 --- a/tests/project/test_ci_timing.py +++ b/tests/project/test_ci_timing.py @@ -8,7 +8,10 @@ projection to a cap, and the indentation reader the caps come from, on the real workflow files as well as on the excerpt below. * :class:`TestClient`, :class:`TestEndToEnd` and the classes after it -- the script - against a fake API serving the fixtures in :file:`tests/project/ci_timing_fixtures/`. + against a fake API serving the fixtures in :file:`tests/project/ci_timing_fixtures/`, + and its :mod:`requests` transport (#1894) against servers on ``127.0.0.1``. + :class:`TestStrictJUnit` checks the hardened lxml parser the JUnit artifacts + are read with (#1894); it and what reads an artifact skip without lxml. The run listing and the job listings are real responses from 2026-10-05 and 2026-10-09, trimmed to the fields the script reads; run 37323913186 is the push run that reads ``cancelled`` because its ``protocols`` ordering job hit the 45-minute @@ -32,6 +35,7 @@ import argparse import contextlib import datetime +import http.server import importlib.util import io import json @@ -40,12 +44,14 @@ import re import statistics import tempfile +import threading import unittest -import urllib.error import urllib.parse import zipfile from unittest import mock +import requests + from tests.project.test_python_versions_1660 import NO_TOMLLIB from tests.project.test_python_versions_1660 import legs as matrix_legs from tests.project.test_python_versions_1660 import matrix, tomllib @@ -55,6 +61,12 @@ if tomllib is None: raise unittest.SkipTest(NO_TOMLLIB) +#: Whether lxml, which the script parses JUnit with (#1894), is installed. The +#: test extra installs it everywhere but GraalPy, which has no wheel; with no +#: laxer parser to fall back to, what reads a JUnit artifact skips there. +HAS_LXML = importlib.util.find_spec('lxml') is not None +NO_LXML = 'lxml, from the test extra, is not installed' + ROOT = pathlib.Path(__file__).resolve().parents[2] FIXTURES = pathlib.Path(__file__).resolve().parent / 'ci_timing_fixtures' WORKFLOWS = ROOT / '.github' / 'workflows' @@ -112,6 +124,13 @@ def span(start: 'str', end: 'str') -> 'float': return (stamp(end) - stamp(start)).total_seconds() / 60 +def http_error(url: 'str', status: 'int') -> 'requests.HTTPError': + """The error the script's transport raises for an error status.""" + response = requests.Response() + response.status_code, response.url = status, url + return requests.HTTPError(f'{status} Error for url: {url}', response=response) + + class FakeAPI: """Serves the fixtures by URL, recording every request. @@ -149,7 +168,7 @@ def __call__(self, url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': if '/contents/.github/workflows/unit-tests.yml' in path: assert accept == 'application/vnd.github.raw+json', accept return fixture('workflow-unit-tests.yml'), {} - raise urllib.error.HTTPError(url, 404, 'Not Found', None, None) # type: ignore[arg-type] + raise http_error(url, 404) def listing(self, runs: 'list[dict]', query: 'dict[str, list[str]]') -> 'tuple[bytes, dict[str, str]]': """The runs created inside the query's inclusive ``A..B`` range, a page at a time.""" @@ -299,7 +318,8 @@ def test_the_budget_stops_the_run(self) -> 'None': list(client.paginate(f'/repos/{REPO}/actions/workflows/unit-tests.yml/runs', 'workflow_runs')) def test_a_server_error_is_retried_and_a_client_error_is_not(self) -> 'None': - replies = [urllib.error.HTTPError('u', 502, 'Bad Gateway', None, None), # type: ignore[arg-type] + """A requests ``HTTPError`` is an ``OSError`` too, so its status decides, not its type.""" + replies = [http_error('u', 502), requests.ConnectionError('connection reset'), (b'{"ok": 1}', {})] def flaky(url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': @@ -311,29 +331,184 @@ def flaky(url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': waits = [] # type: list[float] client = ci_timing.Client('t', transport=flaky, sleep=waits.append) self.assertEqual(client.get('/x'), {'ok': 1}) - self.assertEqual((client.calls, waits), (2, [ci_timing.RETRY_WAITS[0]])) + self.assertEqual((client.calls, waits), (3, list(ci_timing.RETRY_WAITS))) missing = ci_timing.Client('t', transport=FakeAPI(), sleep=waits.append) - with self.assertRaises(urllib.error.HTTPError): + with self.assertRaises(requests.HTTPError): missing.get('/nowhere') self.assertEqual(missing.calls, 1) - def test_the_token_is_not_forwarded_on_a_redirect(self) -> 'None': - """An artifact download redirects to blob storage, which must not see it.""" - seen = [] + def test_the_link_header_is_read_as_requests_reads_it(self) -> 'None': + """GitHub's own shape: every relation in one header, ``next`` among them.""" + first = 'https://api.github.com/repositories/1/actions/runs?page=1' + header = (f'<{first.replace("1", "2", 1)}>; rel="prev", <{LINK_ROOT}?page=3>; rel="next", ' + f'<{LINK_ROOT}?page=9>; rel="last", <{first}>; rel="first"') + self.assertEqual(ci_timing.next_link(header), f'{LINK_ROOT}?page=3') + self.assertIsNone(ci_timing.next_link(f'<{first}>; rel="first"')) + self.assertIsNone(ci_timing.next_link('')) + + +class _Recorder(http.server.BaseHTTPRequestHandler): + """Records each request's headers on its server and answers from ``server.reply``.""" + + def do_GET(self) -> 'None': # noqa: N802 -- the name http.server calls + self.server.seen.append((self.path, dict(self.headers.items()))) # type: ignore[attr-defined] + status, headers, body = self.server.reply(self.path) # type: ignore[attr-defined] + self.send_response(status) + for name, value in headers.items(): + self.send_header(name, value) + self.send_header('Content-Length', str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format: 'str', *args: 'object') -> 'None': # pylint: disable=redefined-builtin + """Quiet: the test reads what it needs from ``server.seen``.""" + + +def local_server(case: 'unittest.TestCase', reply: 'Any') -> 'Any': + """A :class:`_Recorder` server on ``127.0.0.1`` answering ``reply(path)``, shut down after ``case``.""" + server = http.server.ThreadingHTTPServer(('127.0.0.1', 0), _Recorder) + server.seen = [] # type: ignore[attr-defined] + server.reply = reply # type: ignore[attr-defined] + server.root = f'http://127.0.0.1:{server.server_address[1]}' # type: ignore[attr-defined] + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + case.addCleanup(thread.join, 30) + case.addCleanup(server.server_close) + case.addCleanup(server.shutdown) + return server + + +class TestTransport(unittest.TestCase): + """The default transport, :mod:`requests`, against servers on ``127.0.0.1`` (#1894). + + :meth:`Client.fetch` refuses anything but https, so these call the transport + itself, which is what ``fetch`` calls. - class Response(io.BytesIO): - headers = {'X-RateLimit-Remaining': '7'} + """ + + def serve(self, reply: 'Any') -> 'str': + """A server answering with ``reply(path)``; its root URL.""" + server = local_server(self, reply) + self.servers.append(server) + return server.root + + def setUp(self) -> 'None': + self.servers = [] # type: list[Any] + self.client = ci_timing.Client('secret') + # No proxy between the test and its own servers, whatever the host has set. + proxies = mock.patch.dict(os.environ, {'NO_PROXY': '127.0.0.1', 'no_proxy': '127.0.0.1'}) + proxies.start() + self.addCleanup(proxies.stop) + + def test_a_reply_is_the_body_and_the_lower_cased_headers(self) -> 'None': + root = self.serve(lambda path: (200, {'Link': '; rel="next"', + 'X-RateLimit-Remaining': '7'}, b'{"ok": 1}')) + body, headers = self.client.transport(f'{root}/x', 'application/vnd.github+json') + self.assertEqual(body, b'{"ok": 1}') + self.assertEqual((headers['link'], headers['x-ratelimit-remaining']), ('; rel="next"', '7')) + _, sent = self.servers[0].seen[0] + self.assertEqual((sent['Accept'], sent['Authorization'], sent['User-Agent']), + ('application/vnd.github+json', 'Bearer secret', 'PyPCAPKit-ci-timing')) + + def test_an_error_status_raises_requests_httperror_with_the_status(self) -> 'None': + """What :meth:`Client.fetch` retries on, and :func:`collect` reads a 404 from.""" + root = self.serve(lambda path: (404, {}, b'{"message": "Not Found"}')) + with self.assertRaises(requests.HTTPError) as caught: + self.client.transport(f'{root}/nowhere', 'application/vnd.github+json') + self.assertEqual(ci_timing.status_of(caught.exception), 404) - def urlopen(request, timeout): # noqa: ARG001 - seen.append(request) - return Response(b'{}') + def test_the_token_is_not_forwarded_on_a_redirect(self) -> 'None': + """An artifact download redirects to blob storage, which must not see it. + + The second server is another origin, as blob storage is another host; + requests compares the port as well as the host. + + """ + blob = self.serve(lambda path: (200, {}, b'PK zip bytes')) + api = self.serve(lambda path: (302, {'Location': f'{blob}/artifact.zip'}, b'')) + body, _ = self.client.transport(f'{api}/actions/artifacts/1/zip', 'application/vnd.github+json') + self.assertEqual(body, b'PK zip bytes') + (_, to_api), = self.servers[1].seen + (path, to_blob), = self.servers[0].seen + self.assertEqual(path, '/artifact.zip') + self.assertEqual(to_api.get('Authorization'), 'Bearer secret') + self.assertNotIn('Authorization', to_blob) + + def test_one_session_serves_every_call(self) -> 'None': + root = self.serve(lambda path: (200, {}, b'{}')) + self.client.transport(f'{root}/a', 'application/vnd.github+json') + session = self.client.session + self.client.transport(f'{root}/b', 'application/vnd.github+json') + self.assertIsInstance(session, requests.Session) + self.assertIs(self.client.session, session) + + +@unittest.skipUnless(HAS_LXML, NO_LXML) +class TestStrictJUnit(unittest.TestCase): + """#1894: JUnit is parsed by lxml with a hardened parser, where ElementTree read it before.""" + + CASE = b'' + PARSED = [('tests.a.TestA', 'test_a', 0.5)] - client = ci_timing.Client('secret') - with mock.patch.object(ci_timing.urllib.request, 'urlopen', urlopen): - client.get('/x') - self.assertEqual(seen[0].unredirected_hdrs.get('Authorization'), 'Bearer secret') - self.assertNotIn('Authorization', seen[0].headers) - self.assertEqual(client.rate_remaining, 7) + def setUp(self) -> 'None': + from lxml import etree # pylint: disable=import-outside-toplevel + self.etree = etree + + def test_malformed_xml_is_refused(self) -> 'None': + with self.assertRaises(self.etree.XMLSyntaxError): + ci_timing.parse_junit(b'') + + def test_nesting_past_the_depth_limit_is_refused(self) -> 'None': + """``huge_tree=False`` keeps libxml2's limit on depth; ElementTree has none.""" + deep = b'' + b'' * 300 + b'' * 300 + self.CASE + b'' + with self.assertRaisesRegex(self.etree.XMLSyntaxError, 'depth'): + ci_timing.parse_junit(deep) + + def test_an_entity_is_neither_expanded_nor_read(self) -> 'None': + """An internal one stays an entity node, and one naming a file or a URL is never opened. + + ElementTree expanded the internal one, and refused the document over the + external ones; neither is what this asks of the parser. + + """ + server = local_server(self, lambda path: (200, {}, b'FETCHED')) + with tempfile.TemporaryDirectory() as tmp: + canary = pathlib.Path(tmp) / 'canary.txt' + canary.write_text('READ', encoding='utf-8') + document = (f'' + f'' + f']>' + '' + '&inner;&outer;&remote;').encode('utf-8') + root = ci_timing.parse_xml(document) + self.assertEqual(ci_timing.parse_junit(document), {'cases': self.PARSED, 'suite_tests': 1}) + case = root.find('testcase') + self.assertIsNone(case.text) + self.assertEqual([(child.tag is self.etree.Entity, child.name) for child in case], + [(True, 'inner'), (True, 'outer'), (True, 'remote')]) + text = self.etree.tostring(root) + for marker in (b'INJECTED', b'READ', b'FETCHED'): + with self.subTest(marker=marker): + self.assertNotIn(marker, text) + self.assertEqual(server.seen, []) + + def test_an_attribute_naming_an_external_entity_is_refused(self) -> 'None': + """An attribute cannot hold an entity node, so libxml2 refuses it rather than read the file.""" + document = (b']>' + b'') + with self.assertRaisesRegex(self.etree.XMLSyntaxError, 'external entity'): + ci_timing.parse_junit(document) + + def test_comments_and_instructions_change_nothing(self) -> 'None': + """lxml's ``iter()`` yields them, but ``iter('testcase')`` and ``findall`` do not.""" + plain = b'' + self.CASE + b'' + noisy = (b'' + b'' + self.CASE + b'') + alone = b'' + self.CASE + b'' + expected = {'cases': self.PARSED, 'suite_tests': 2} + for label, document in (('plain', plain), ('noisy', noisy), ('a bare testsuite', alone)): + with self.subTest(label): + self.assertEqual(ci_timing.parse_junit(document), expected) def counted(job: 'dict', cap: 'float') -> 'bool': @@ -345,8 +520,9 @@ def counted(job: 'dict', cap: 'float') -> 'bool': return job['conclusion'] == 'cancelled' and span(job['started_at'], job['completed_at']) >= 0.95 * cap +@unittest.skipUnless(HAS_LXML, NO_LXML) class TestEndToEnd(unittest.TestCase): - """``main`` over the recorded fixtures.""" + """``main`` over the recorded fixtures, the JUnit artifact included.""" @classmethod def setUpClass(cls) -> 'None': @@ -554,6 +730,7 @@ def test_a_smaller_sample_keeps_the_listing_counts(self) -> 'None': self.assertEqual(report['window']['read']['unit-tests.yml']['listed'], 4) self.assertEqual((report['superseded']['runs'], report['superseded']['of']), (1, 2)) + @unittest.skipUnless(HAS_LXML, NO_LXML) def test_a_day_past_the_listing_cap_is_reported(self) -> 'None': _, _, report, _ = run_main(api=FakeAPI(inflate=ci_timing.LISTING_CAP)) self.assertTrue(any('of which the API returned 3' in note for note in report['notes']), report['notes']) @@ -574,6 +751,32 @@ def test_a_crash_is_not_reported_as_a_threshold(self) -> 'None': self.assertEqual((status, report), (ci_timing.EXIT_CRASH, {})) self.assertIn('::error title=CI timing crashed::', stdout) + def test_a_connection_that_never_answers_is_an_api_failure(self) -> 'None': + """requests' errors subclass ``OSError``, which ``main`` reports as an unreadable API.""" + def down(url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': + raise requests.ConnectionError('connection refused') + + with mock.patch.object(ci_timing, 'RETRY_WAITS', (0, 0)): + status, stdout, report, _ = run_main(api=down) # type: ignore[arg-type] + self.assertEqual((status, report), (ci_timing.EXIT_API, {})) + self.assertIn('::error title=CI timing could not read the API::connection refused', stdout) + + @unittest.skipUnless(HAS_LXML, NO_LXML) + def test_a_workflow_missing_at_a_sha_is_a_note(self) -> 'None': + """A 404 for the workflow file leaves its caps unknown and the run going.""" + class Missing(FakeAPI): + def __call__(self, url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': + if '/contents/' in url: + raise http_error(url, 404) + return super().__call__(url, accept) + + with mock.patch.object(ci_timing, 'MIN_SAMPLES', 1): + status, _, report, _ = run_main(api=Missing()) + self.assertNotIn(status, (ci_timing.EXIT_API, ci_timing.EXIT_CRASH)) + missing = [note for note in report['notes'] if 'caps are unknown' in note] + self.assertEqual(len(missing), 2, report['notes']) + self.assertTrue(all(note.startswith('unit-tests.yml is not at ') for note in missing), missing) + class TestRunningJobs(unittest.TestCase): """A job with no conclusion is still running, whatever its timestamps say.""" diff --git a/tests/project/test_pcapng_download_retry_1658_unit.py b/tests/project/test_pcapng_download_retry_1658_unit.py index 2ddb775b80..9d4506991e 100644 --- a/tests/project/test_pcapng_download_retry_1658_unit.py +++ b/tests/project/test_pcapng_download_retry_1658_unit.py @@ -6,9 +6,13 @@ or 5xx, or a timeout, is now retried with backoff, honouring ``Retry-After``, within a small budget; any other failure still falls back at once. -Every test runs against a stubbed ``urllib.request.urlopen`` and a fake clock -bound over the generator's ``time``, so nothing touches the network or sleeps. -This module is unit-tier and reads no capture. +#1894 moved the download from :mod:`urllib.request` to :mod:`requests`. +:class:`DownloadRetryTests` runs against a stubbed ``requests.get`` and a fake +clock bound over the generator's ``time``, so nothing touches the network or +sleeps. :class:`LocalServerTests` sends real requests to a server on the +loopback interface, because what :mod:`requests` raises for a timeout while the +body is read is its own behaviour, not something a stub can show. This module +is unit-tier and reads no capture. """ from __future__ import annotations @@ -16,22 +20,27 @@ import contextlib import email.utils import http.client +import http.server import importlib.util import io import os import pathlib import socket +import subprocess # nosec B404 import sys import tempfile +import threading import time import types import unittest -import urllib.error from unittest import mock -from tests._support import PCAPNG_GENERATOR +import requests +import urllib3 -#: URL the stubbed opener is asked for. +from tests._support import PCAPNG_GENERATOR, scale_timeout + +#: URL the stubbed getter is asked for. URL = 'https://example.invalid/many_interfaces.pcapng.1' #: Fake current time, a whole second so an HTTP-date maps to it exactly. NOW = 1_800_000_000.0 @@ -61,37 +70,56 @@ def _load_generator() -> 'types.ModuleType': return module -def _http_error(code: int, retry_after: 'str | None' = None) -> urllib.error.HTTPError: - """An :class:`~urllib.error.HTTPError` as ``urlopen`` raises it.""" - headers = http.client.HTTPMessage() +def _response(code: int, body: bytes = b'', retry_after: 'str | None' = None) -> requests.Response: + """A :class:`requests.Response` as ``requests.get`` returns it, unread.""" + response = requests.Response() + response.status_code = code + response.reason = http.client.responses.get(code, 'Unknown') # 599 has no registered phrase + response.url = URL if retry_after is not None: - headers['Retry-After'] = retry_after - reason = http.client.responses.get(code, 'Unknown') # 599 has no registered phrase - return urllib.error.HTTPError(URL, code, reason, headers, io.BytesIO()) + response.headers['Retry-After'] = retry_after + response.raw = io.BytesIO(body) + return response + + +def _http_error(code: int, retry_after: 'str | None' = None) -> requests.HTTPError: + """The :exc:`requests.HTTPError` ``raise_for_status`` raises for ``code``.""" + try: + _response(code, retry_after=retry_after).raise_for_status() + except requests.HTTPError as error: + return error + raise AssertionError(f'{code} is not an error status') # pragma: no cover + +def _body_timeout() -> requests.ConnectionError: + """A timeout while the body is read, as :mod:`requests` raises it.""" + return requests.ConnectionError(urllib3.exceptions.ReadTimeoutError(None, URL, 'Read timed out.')) -def _timeout() -> urllib.error.URLError: - """A connect timeout, as ``urlopen`` wraps it.""" - return urllib.error.URLError(socket.timeout('timed out')) +class _Getter: + """A stand-in for ``requests.get`` serving canned outcomes in turn. -class _Opener: - """A stand-in for ``urllib.request.urlopen`` serving canned outcomes in turn.""" + An exception is raised, a :class:`requests.Response` is returned as it is, + and bytes are returned as a 200 response with that body. - def __init__(self, *outcomes: 'BaseException | bytes') -> None: + """ + + def __init__(self, *outcomes: 'BaseException | requests.Response | bytes') -> None: self.outcomes = list(outcomes) self.calls = [] # type: list[str] - def __call__(self, url: str, timeout: float) -> io.BytesIO: + def __call__(self, url: str, timeout: float) -> requests.Response: self.calls.append(url) outcome = self.outcomes.pop(0) if isinstance(outcome, BaseException): raise outcome - return io.BytesIO(outcome) + if isinstance(outcome, bytes): + return _response(200, outcome) + return outcome -class DownloadRetryTests(unittest.TestCase): - """``_download`` retries what is transient, and nothing else.""" +class _Clocked(unittest.TestCase): + """The generator, with a fake clock bound over its ``time``.""" generator: 'types.ModuleType' @@ -109,107 +137,219 @@ def setUp(self) -> None: self.addCleanup(patcher.stop) self.stdout, self.stderr = io.StringIO(), io.StringIO() - def run_download(self, opener: _Opener) -> bytes: - with mock.patch('urllib.request.urlopen', opener), \ + +class DownloadRetryTests(_Clocked): + """``_download`` retries what is transient, and nothing else.""" + + def run_download(self, getter: _Getter) -> bytes: + with mock.patch('requests.get', getter), \ contextlib.redirect_stdout(self.stdout), contextlib.redirect_stderr(self.stderr): return self.generator._download(URL) def test_429_then_success_returns_the_body(self) -> None: - opener = _Opener(_http_error(429), b'upstream') - self.assertEqual(self.run_download(opener), b'upstream') - self.assertEqual(opener.calls, [URL, URL]) + getter = _Getter(_response(429), b'upstream') + self.assertEqual(self.run_download(getter), b'upstream') + self.assertEqual(getter.calls, [URL, URL]) self.assertEqual(self.sleeps, [BACKOFF]) - self.assertIn('HTTP Error 429: Too Many Requests; retry 1 of 2', self.stderr.getvalue()) + self.assertIn(f'429 Client Error: Too Many Requests for url: {URL}; retry 1 of 2', + self.stderr.getvalue()) self.assertEqual(self.stdout.getvalue(), '') def test_retry_after_is_waited_in_seconds_and_as_an_http_date(self) -> None: date = email.utils.formatdate(NOW + 11, usegmt=True) - opener = _Opener(_http_error(429, '7'), _http_error(503, date), b'upstream') - self.assertEqual(self.run_download(opener), b'upstream') + getter = _Getter(_response(429, retry_after='7'), _response(503, retry_after=date), b'upstream') + self.assertEqual(self.run_download(getter), b'upstream') self.assertEqual(self.sleeps, [7.0, 11.0]) - self.assertEqual(len(opener.calls), 3) + self.assertEqual(len(getter.calls), 3) def test_retry_after_past_the_budget_is_not_waited(self) -> None: """A wait that would take the total past ``RETRY_BUDGET`` ends the retries.""" cases = { - 'one long wait': ([_http_error(429, str(int(BUDGET) + 1))], [], 1), - 'two waits summed': ([_http_error(429, '20'), _http_error(429, '20')], [20.0], 2), + 'one long wait': ([_response(429, retry_after=str(int(BUDGET) + 1))], [], 1), + 'two waits summed': ([_response(429, retry_after='20'), _response(429, retry_after='20')], [20.0], 2), } for label, (errors, sleeps, calls) in cases.items(): with self.subTest(label): del self.sleeps[:] self.stderr = io.StringIO() - opener = _Opener(*errors, b'upstream') - with self.assertRaises(urllib.error.HTTPError): - self.run_download(opener) + getter = _Getter(*errors, b'upstream') + with self.assertRaises(requests.HTTPError): + self.run_download(getter) self.assertEqual(self.sleeps, sleeps) - self.assertEqual(len(opener.calls), calls) + self.assertEqual(len(getter.calls), calls) self.assertIn('retry budget, so not retrying', self.stderr.getvalue()) def test_waits_totalling_exactly_the_budget_are_waited(self) -> None: """``RETRY_BUDGET`` is inclusive: a total of exactly 30 s still retries.""" cases = { - 'one wait of the budget': ([_http_error(429, str(int(BUDGET)))], [BUDGET]), - 'two waits summing to it': ([_http_error(429, '10'), _http_error(503, '20')], [10.0, 20.0]), + 'one wait of the budget': ([_response(429, retry_after=str(int(BUDGET)))], [BUDGET]), + 'two waits summing to it': ([_response(429, retry_after='10'), _response(503, retry_after='20')], + [10.0, 20.0]), } for label, (errors, sleeps) in cases.items(): with self.subTest(label): del self.sleeps[:] - opener = _Opener(*errors, b'upstream') - self.assertEqual(self.run_download(opener), b'upstream') + getter = _Getter(*errors, b'upstream') + self.assertEqual(self.run_download(getter), b'upstream') self.assertEqual(self.sleeps, sleeps) - self.assertEqual(len(opener.calls), len(errors) + 1) + self.assertEqual(len(getter.calls), len(errors) + 1) def test_404_and_other_final_failures_are_not_retried(self) -> None: """Only a 429, a 5xx and a timeout get a second attempt. The 429, 5xx and timeout rows are the control: the same stub, a second outcome waiting, and only the error differs. 500 and 599 are the two - ends of the 5xx range, both inclusive. + ends of the 5xx range, both inclusive. A refused connection is a + :exc:`requests.ConnectionError` too, as a timeout while the body is read + is, but wraps no urllib3 ``ReadTimeoutError``. """ + refused = requests.ConnectionError(urllib3.exceptions.MaxRetryError( + None, URL, ConnectionRefusedError(111, 'refused'))) cases = ( - ('HTTP 404', _http_error(404), 1), - ('HTTP 403', _http_error(403), 1), - ('HTTP 400', _http_error(400), 1), - ('connection refused', urllib.error.URLError(ConnectionRefusedError(111, 'refused')), 1), - ('HTTP 429', _http_error(429), 2), - ('HTTP 500', _http_error(500), 2), - ('HTTP 502', _http_error(502), 2), - ('HTTP 599', _http_error(599), 2), - ('connect timeout', _timeout(), 2), - ('read timeout', socket.timeout('timed out'), 2), + ('HTTP 404', _response(404), requests.HTTPError, 1), + ('HTTP 403', _response(403), requests.HTTPError, 1), + ('HTTP 400', _response(400), requests.HTTPError, 1), + ('connection refused', refused, requests.ConnectionError, 1), + ('HTTP 429', _response(429), None, 2), + ('HTTP 500', _response(500), None, 2), + ('HTTP 502', _response(502), None, 2), + ('HTTP 599', _response(599), None, 2), + ('connect timeout', requests.ConnectTimeout('timed out'), None, 2), + ('read timeout', requests.ReadTimeout('timed out'), None, 2), + ('timeout reading the body', _body_timeout(), None, 2), + ('bare socket timeout', socket.timeout('timed out'), None, 2), ) - for label, error, calls in cases: + for label, outcome, raised, calls in cases: with self.subTest(label): del self.sleeps[:] - opener = _Opener(error, b'upstream') - if calls == 1: - with self.assertRaises(type(error)): - self.run_download(opener) + getter = _Getter(outcome, b'upstream') + if raised is not None: + with self.assertRaises(raised): + self.run_download(getter) self.assertEqual(self.sleeps, []) else: - self.assertEqual(self.run_download(opener), b'upstream') - self.assertEqual(len(opener.calls), calls) + self.assertEqual(self.run_download(getter), b'upstream') + self.assertEqual(len(getter.calls), calls) def test_exhausted_retries_fall_back_to_the_stand_in(self) -> None: fixture = self.generator._Fixture(name='many_interfaces.pcapng', feature='probe', build=lambda: b'stand-in', url=URL, sha256='0' * 64) - opener = _Opener(_http_error(429), _http_error(503), _timeout(), b'never served') + timeout = requests.ConnectTimeout('timed out') + getter = _Getter(_response(429), _response(503), timeout, b'never served') with tempfile.TemporaryDirectory(prefix='pcapkit-1658-') as tmp, \ - mock.patch('urllib.request.urlopen', opener), \ + mock.patch('requests.get', getter), \ contextlib.redirect_stdout(self.stdout), contextlib.redirect_stderr(self.stderr): data, origin = self.generator._materialise(fixture, pathlib.Path(tmp) / fixture.name) self.assertEqual((data, origin), (b'stand-in', 'synthesised (download unavailable)')) - self.assertEqual(len(opener.calls), ATTEMPTS) + self.assertEqual(len(getter.calls), ATTEMPTS) self.assertEqual(self.sleeps, [BACKOFF, 2 * BACKOFF]) self.assertEqual(self.stderr.getvalue().count('; retry '), 2) self.assertEqual(self.stdout.getvalue(), ( - f' ! {URL} is unreachable ();\n' + f' ! {URL} is unreachable (timed out);\n' ' ! synthesising a stand-in for many_interfaces.pcapng instead -- the ' 'upstream capture was NOT used\n')) + def test_without_requests_the_stand_in_is_used(self) -> None: + """#1894: :mod:`requests` is imported for the download alone. + + So the generator loads without it, and a fixture with a URL falls back as + it does offline rather than failing the whole run. In a child process, + whose ``sys.modules`` entry of ``None`` makes ``import requests`` raise + :exc:`ImportError` there and nowhere else. + + """ + script = ( + 'import importlib.util, pathlib, sys, tempfile\n' + "sys.modules['requests'] = None\n" + f'spec = importlib.util.spec_from_file_location("generator", {str(PCAPNG_GENERATOR)!r})\n' + 'generator = importlib.util.module_from_spec(spec)\n' + 'spec.loader.exec_module(generator)\n' + "fixture = generator._Fixture(name='many_interfaces.pcapng', feature='probe',\n" + f" build=lambda: b'stand-in', url={URL!r}, sha256='0' * 64)\n" + 'with tempfile.TemporaryDirectory() as tmp:\n' + ' print(generator._materialise(fixture, pathlib.Path(tmp) / fixture.name))\n' + ) + result = subprocess.run([sys.executable, '-c', script], capture_output=True, text=True, + check=False, timeout=scale_timeout(60)) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn(f' ! {URL} cannot be fetched without requests (', result.stdout) + self.assertIn("(b'stand-in', 'synthesised (download unavailable)')", result.stdout) + + +class _Handler(http.server.BaseHTTPRequestHandler): + """Serves the outcomes queued on its server, one per request.""" + + def do_GET(self) -> None: # noqa: N802 -- the name http.server calls + status, headers, body, stall = self.server.outcomes.pop(0) # type: ignore[attr-defined] + self.send_response(status) + for name, value in headers.items(): + self.send_header(name, value) + self.send_header('Content-Length', str(len(body))) + self.end_headers() + if stall: + # Part of the body, then nothing until the test ends. + self.wfile.write(body[:4]) + self.wfile.flush() + self.server.release.wait(30) # type: ignore[attr-defined] + return + self.wfile.write(body) + + def log_message(self, format: str, *args: object) -> None: # pylint: disable=redefined-builtin + """Quiet: the test reads what it needs from the outcomes.""" + + +class LocalServerTests(_Clocked): + """``_download`` against real responses from a server on ``127.0.0.1``.""" + + def setUp(self) -> None: + super().setUp() + self.server = http.server.ThreadingHTTPServer(('127.0.0.1', 0), _Handler) + self.server.outcomes = [] # type: ignore[attr-defined] + self.server.release = threading.Event() # type: ignore[attr-defined] + thread = threading.Thread(target=self.server.serve_forever, daemon=True) + thread.start() + self.addCleanup(thread.join, 30) + self.addCleanup(self.server.server_close) + self.addCleanup(self.server.shutdown) + self.addCleanup(self.server.release.set) # type: ignore[attr-defined] + self.url = f'http://127.0.0.1:{self.server.server_address[1]}/many_interfaces.pcapng.1' + # No proxy between the test and its own server, whatever the host has set. + proxies = mock.patch.dict(os.environ, {'NO_PROXY': '127.0.0.1', 'no_proxy': '127.0.0.1'}) + proxies.start() + self.addCleanup(proxies.stop) + + def serve(self, status: int, body: bytes = b'', stall: bool = False, **headers: str) -> None: + self.server.outcomes.append((status, headers, body, stall)) # type: ignore[attr-defined] + + def download(self) -> bytes: + with contextlib.redirect_stdout(self.stdout), contextlib.redirect_stderr(self.stderr): + return self.generator._download(self.url) + + def test_a_timeout_while_the_body_is_read_is_retried(self) -> None: + self.serve(200, b'upstream capture', stall=True) + self.serve(200, b'upstream capture') + with mock.patch.object(self.generator, 'TIMEOUT', 0.5): + self.assertEqual(self.download(), b'upstream capture') + self.assertEqual(self.sleeps, [BACKOFF]) + self.assertIn('Read timed out', self.stderr.getvalue()) + + def test_retry_after_on_a_real_429_is_waited(self) -> None: + self.serve(429, **{'Retry-After': '3'}) + self.serve(200, b'upstream capture') + self.assertEqual(self.download(), b'upstream capture') + self.assertEqual(self.sleeps, [3.0]) + + def test_a_real_404_is_not_retried(self) -> None: + self.serve(404) + self.serve(200, b'never served') + with self.assertRaises(requests.HTTPError) as caught: + self.download() + self.assertEqual(caught.exception.response.status_code, 404) + self.assertEqual(self.sleeps, []) + self.assertEqual(len(self.server.outcomes), 1) # type: ignore[attr-defined] + class RetryAfterTests(unittest.TestCase): """``_retry_after`` reads both forms RFC 9110 allows, and nothing else.""" @@ -232,7 +372,9 @@ def test_header_values(self) -> None: with self.subTest('no header'): self.assertIsNone(generator._retry_after(_http_error(429), NOW)) with self.subTest('not an HTTP error'): - self.assertIsNone(generator._retry_after(_timeout(), NOW)) + self.assertIsNone(generator._retry_after(requests.ConnectTimeout('timed out'), NOW)) + with self.subTest('not a requests error'): + self.assertIsNone(generator._retry_after(socket.timeout('timed out'), NOW)) def test_a_minus_0000_date_is_utc_whatever_the_host_zone(self) -> None: """An HTTP-date in a ``-0000`` zone names a UTC instant. diff --git a/tests/project/test_pyshark_encap_map.py b/tests/project/test_pyshark_encap_map.py index db7996d79f..7af1b9e6b7 100644 --- a/tests/project/test_pyshark_encap_map.py +++ b/tests/project/test_pyshark_encap_map.py @@ -45,6 +45,10 @@ measure agrees with the committed table rather than merely being present or absent from it. This is what still exercises the real binaries on the CI legs :class:`RealSweepTests` skips. +* :class:`StrictPdmlTests` checks the hardened lxml parser the PDML is read + with (#1894), against stand-ins for editcap and tshark. It and the classes + above are gated on :data:`HAS_LXML` too, since the generator has no laxer + parser to fall back to. """ @@ -98,6 +102,12 @@ def _load_generator(): #: is not something any ``pyproject.toml`` extra could ever install. HAS_WIRESHARK = _EDITCAP is not None and _TSHARK is not None +#: Whether lxml, which the generator parses PDML with (#1894), is installed. The +#: test extra installs it everywhere but GraalPy, which has no wheel; with no +#: laxer parser to fall back to, everything here that parses PDML skips there. +HAS_LXML = importlib.util.find_spec('lxml') is not None +NO_LXML = 'lxml, from the test extra, is not installed' + #: The one Wireshark release ``ENCAP_TYPE_TO_LINKTYPE`` and #: ``FILTER_NAME_TO_LINKTYPE`` were measured against, and the only one #: :class:`RealSweepTests` trusts to reproduce their exact counts. Named here, @@ -287,6 +297,101 @@ def make_tmp_dir(case: unittest.TestCase) -> pathlib.Path: return pathlib.Path(tmp.name) +#: The PDML :program:`tshark` 4.6.9 writes for ``editcap -T null``'s first +#: packet, cut to what :func:`~pyshark_encap_map.measure_encap` reads, with the +#: declaration, stylesheet instruction and comment it opens with. +PDML = ''' + + + + + + + + + + {frame} + + + + +''' + + +@unittest.skipUnless(HAS_LXML, NO_LXML) +class StrictPdmlTests(unittest.TestCase): + """#1894: the PDML is parsed by lxml with a hardened parser, where ElementTree read it before. + + Through :func:`~pyshark_encap_map.measure_encap`, against stand-ins for + :program:`editcap` and :program:`tshark`: the first writes a pcap header + naming DLT 0, the second prints the document the test chose, as bytes. + + """ + + def setUp(self) -> None: + from lxml import etree # pylint: disable=import-outside-toplevel + self.etree = etree + self.tmp = make_tmp_dir(self) + self.editcap = self.tmp / 'fake-editcap' + self.editcap.write_text( + f'#!{sys.executable}\n' + 'import struct, sys\n' + "open(sys.argv[-1], 'wb').write(struct.pack(' 'object': + self.document.write_bytes(document) + return encap_map.measure_encap(str(self.editcap), str(self.tshark), self.tmp / 'in.pcap', 'null', self.tmp) + + def test_tsharks_pdml_is_read_as_bytes(self) -> None: + """It declares its encoding, which lxml will not be handed a decoded string with.""" + self.assertEqual(self.measure(PDML.format(frame='').encode('utf-8')), + encap_map.Measurement(name='null', dlt=0, encap_type=15, root_name='null')) + + def test_malformed_pdml_is_refused(self) -> None: + with self.assertRaises(self.etree.XMLSyntaxError): + self.measure(PDML.format(frame='').encode('utf-8')) + + def test_nesting_past_the_depth_limit_is_refused(self) -> None: + """``huge_tree=False`` keeps libxml2's limit on depth; ElementTree has none.""" + deep = '' * 300 + '' * 300 + with self.assertRaisesRegex(self.etree.XMLSyntaxError, 'depth'): + self.measure(PDML.format(frame=deep).encode('utf-8')) + + def test_an_attribute_naming_an_external_entity_is_refused(self) -> None: + """An attribute cannot hold an entity node, so libxml2 refuses it rather than read the file.""" + document = PDML.format(frame='').replace( + ']>\n None: + canary = self.tmp / 'canary.txt' + canary.write_text('READ', encoding='utf-8') + document = PDML.format(frame='&inner;&outer;').replace( + ']>\n None: @unittest.skipUnless(HAS_WIRESHARK, 'editcap/tshark not found on PATH') +@unittest.skipUnless(HAS_LXML, NO_LXML) @unittest.skipUnless( _TSHARK_VERSION == MEASURED_WIRESHARK_VERSION, f'tshark reports {_TSHARK_VERSION!r}, this class only trusts its exact ' @@ -550,6 +656,7 @@ def test_check_mode_reports_the_committed_file_as_current(self) -> None: @unittest.skipUnless(HAS_WIRESHARK, 'editcap/tshark not found on PATH') +@unittest.skipUnless(HAS_LXML, NO_LXML) class VersionIndependentInvariantTests(unittest.TestCase): """Properties of a real sweep that hold on any Wireshark, not only 4.6.9. diff --git a/tests/project/test_python_versions_update_1660.py b/tests/project/test_python_versions_update_1660.py index 0af06a4e04..42e9e7a675 100644 --- a/tests/project/test_python_versions_update_1660.py +++ b/tests/project/test_python_versions_update_1660.py @@ -13,7 +13,8 @@ Nothing here touches the network. The sources are the snapshots in :file:`tests/project/python_versions_fixtures/`, trimmed from what each URL -served on 2026-10-13, and each scenario edits a copy of them. The scenarios run +served on 2026-10-13, and each scenario edits a copy of them; :class:`TestFetch` +stubs ``requests.get``, which the script fetches them with (#1894). The scenarios run on a scratch copy of the files the script writes, with the tier file as it stood then (the ``python-versions.toml`` there) and the prose regenerated to match it, so a later tier change, this job's own included, leaves them as they are. The @@ -37,10 +38,14 @@ import subprocess # nosec B404 import sys import tempfile +import types import unittest from typing import TYPE_CHECKING from unittest import mock +import requests + +from tests._support import scale_timeout from tests.project.test_release_gates import evaluate_condition try: @@ -245,6 +250,131 @@ def context(sources: 'dict[str, Any]') -> 'Any': module.workflow_labels(module.load_workflows(ROOT))) +############################################################################### +# Fetching +############################################################################### + + +#: Where the stubbed source points: a reserved name, so nothing it reaches answers. +INVALID_URL = 'https://example.invalid/release-cycle.json' + + +def http_response(status: 'int', body: 'bytes' = b'') -> 'requests.Response': + """A :class:`requests.Response` as ``requests.get`` returns it.""" + response = requests.Response() + response.status_code = status + response.url = INVALID_URL + response.raw = io.BytesIO(body) + return response + + +class TestFetch(unittest.TestCase): + """#1894: :func:`fetch` reads each source through ``requests.get``.""" + + def setUp(self) -> None: + self.module = updater() + self.sleeps = [] # type: list[float] + self.calls = [] # type: list[tuple[str, dict[str, Any]]] + self.outcomes = [] # type: list[Any] + source = self.module.Source(INVALID_URL, 'release-cycle.json', 'json') + for patcher in (mock.patch.dict(self.module.SOURCES, {'release-cycle': source}), + mock.patch.object(self.module, 'time', types.SimpleNamespace(sleep=self.sleeps.append)), + mock.patch('requests.get', self.get)): + patcher.start() + self.addCleanup(patcher.stop) + + def get(self, url: 'str', **kwargs: 'Any') -> 'requests.Response': + self.calls.append((url, kwargs)) + outcome = self.outcomes.pop(0) + if isinstance(outcome, BaseException): + raise outcome + return outcome + + def test_a_source_is_read_with_requests(self) -> None: + self.outcomes = [http_response(200, '{"3.15": {"status": "bugfix"}}'.encode('utf-8'))] + self.assertEqual(self.module.fetch('release-cycle'), '{"3.15": {"status": "bugfix"}}') + self.assertEqual(self.calls, [(INVALID_URL, { + 'timeout': 60, 'headers': {'User-Agent': 'PyPCAPKit cron-python-versions (#1660)'}})]) + self.assertEqual(self.sleeps, []) + + def test_an_error_status_and_a_dropped_connection_are_retried(self) -> None: + self.outcomes = [http_response(503), requests.ConnectionError('connection reset'), + http_response(200, b'{}')] + self.assertEqual(self.module.fetch('release-cycle'), '{}') + self.assertEqual(len(self.calls), 3) + self.assertEqual(self.sleeps, [5, 10]) + + def test_the_last_failure_is_an_oserror_naming_the_source(self) -> None: + """``main`` reports an :exc:`OSError` as the run's error, as it did before.""" + self.outcomes = [http_response(404)] * 3 + with self.assertRaises(OSError) as caught: + self.module.fetch('release-cycle') + self.assertTrue(str(caught.exception).startswith(f'{INVALID_URL}: 404 Client Error'), caught.exception) + self.assertIsInstance(caught.exception.__cause__, requests.HTTPError) + self.assertEqual(self.sleeps, [5, 10]) + + def test_loading_the_script_imports_neither_requests_nor_markdown_it(self) -> None: + """``pr`` runs ``publish`` on a bare ``python3``, so loading the script must not need them.""" + imported = "print(sorted({'requests', 'markdown_it'} & set(sys.modules)))\n" + script = ( + 'import importlib.util, pathlib, sys\n' + + imported + + f'spec = importlib.util.spec_from_file_location("update_python_versions", {str(SCRIPT)!r})\n' + 'module = importlib.util.module_from_spec(spec)\n' + 'spec.loader.exec_module(module)\n' + f'module.read_sources(pathlib.Path({str(FIXTURES)!r}))\n' + + imported + ) + result = subprocess.run([sys.executable, '-c', script], capture_output=True, text=True, check=False, + timeout=scale_timeout(60)) + # Not imported at startup either, or the second answer would prove nothing. + self.assertEqual((result.returncode, result.stdout), (0, '[]\n[]\n'), result.stderr) + + +#: The runner-images table's header and delimiter rows, as the README has them. +TABLE_HEAD = ('| Image | Architecture | YAML Label | Included Software |\n' + '| --------------------|--------------|---------------------|------------------|\n') + + +class TestTheImageTable(unittest.TestCase): + """#1894: the runner-images table is read as GitHub renders it, by markdown-it-py.""" + + def test_an_escaped_pipe_stays_in_its_cell(self) -> None: + """In a cell's text or in a code span; splitting the line on ``|`` shifted every column after it.""" + module = updater() + text = TABLE_HEAD + ( + '| Ubuntu 24.04 \\| Noble
![Endpoint Badge](https://example.invalid/badge) | x64 ' + '| `ubuntu-latest` or `ubuntu-24.04` | [ubuntu-24.04] |\n' + '| Ubuntu 22.04 | x64 | `ubuntu-22.04` or `ubuntu\\|22.04` | [ubuntu-22.04] |\n' + '| macOS 15 Arm64 | arm64 | `macos-15`, or `macos-15-xlarge` | [macOS-15-arm64] |\n') + self.assertEqual(module.parse_images(text), [ + module.Image('ubuntu', '24.04', 'x64', ('ubuntu-latest', 'ubuntu-24.04')), + module.Image('ubuntu', '22.04', 'x64', ('ubuntu-22.04', 'ubuntu|22.04')), + module.Image('macos', '15', 'arm64', ('macos-15', 'macos-15-xlarge')), + ]) + + def test_a_line_of_prose_with_a_pipe_is_not_the_table(self) -> None: + """Only a table's header row names its columns, not any line that starts with ``|``.""" + module = updater() + text = ('| The YAML Label column below is what `runs-on` takes.\n\n' + + TABLE_HEAD + '| Ubuntu 24.04 | x64 | `ubuntu-24.04` | [ubuntu-24.04] |\n') + self.assertEqual(module.parse_images(text), [module.Image('ubuntu', '24.04', 'x64', ('ubuntu-24.04',))]) + + def test_what_it_refuses(self) -> None: + """The same errors as before markdown-it-py read the table.""" + module = updater() + cases = ( + ('no table', 'Images: `ubuntu-24.04`.\n', 'has no table with a "YAML Label" column'), + ('no Architecture column', '| Image | YAML Label |\n| --- | --- |\n| Ubuntu 24.04 | `ubuntu-24.04` |\n', + r"has columns \['Image', 'YAML Label'\]"), + ('no Ubuntu image', TABLE_HEAD + '| macOS 15 | arm64 | `macos-15` | [macOS-15] |\n', 'lists no Ubuntu image'), + ) + for label, text, message in cases: + with self.subTest(label): + with self.assertRaisesRegex(ValueError, message): + module.parse_images(text) + + ############################################################################### # The tiers ############################################################################### diff --git a/tests/project/test_script_dependencies_1894.py b/tests/project/test_script_dependencies_1894.py new file mode 100644 index 0000000000..1f6cd25d94 --- /dev/null +++ b/tests/project/test_script_dependencies_1894.py @@ -0,0 +1,88 @@ +# -*- coding: utf-8 -*- +"""#1894: what the scripts import is declared alike for CI and for the dev environment. + +The scripts under :file:`util/` and :file:`examples/generators/` fetch with +``requests``, sample memory with ``psutil``, parse XML with ``lxml`` and read a +Markdown table with ``markdown-it-py``. The ``test`` extra in +:file:`pyproject.toml` carries them for CI; at the owner's request +:file:`Pipfile`'s ``[dev-packages]``, which ``make``'s ``pipenv run`` targets +resolve against, carries them too, with the ``test`` extra's markers. Those keep +each off the interpreters PyPI has no wheel of it for: ``psutil`` is CPython +only, and ``lxml`` is everywhere but GraalPy. The ``dev`` extra is not asked: +it holds what the toolchain linting :mod:`pcapkit` must resolve, not the +scripts' dependencies. + +""" + +from __future__ import annotations + +import pathlib +import re +import unittest +from typing import TYPE_CHECKING + +from tests.project.test_python_versions_1660 import NO_TOMLLIB, tomllib + +if TYPE_CHECKING: + from typing import Any, Optional + +if tomllib is None: + raise unittest.SkipTest(NO_TOMLLIB) + +ROOT = pathlib.Path(__file__).resolve().parents[2] + +#: The distributions the scripts import, by their PyPI names. +SCRIPT_DEPENDENCIES = ('requests', 'psutil', 'lxml', 'markdown-it-py') + +#: A requirement string's distribution name, before any extras, specifier or marker. +NAME = re.compile(r'[A-Za-z0-9][A-Za-z0-9._-]*') + + +def canonical(name: 'str') -> 'str': + """``name`` as PEP 503 normalises it.""" + return re.sub(r'[-_.]+', '-', name).lower() + + +def extra(name: 'str') -> 'dict[str, Optional[str]]': + """``pyproject.toml``'s extra ``name``: each distribution's marker as written, or :data:`None`.""" + project = tomllib.loads((ROOT / 'pyproject.toml').read_text(encoding='utf-8'))['project'] + found = {} # type: dict[str, Optional[str]] + for line in project['optional-dependencies'][name]: + match = NAME.match(line) + assert match is not None, line + _, _, marker = line.partition(';') + found[canonical(match.group(0))] = marker.strip() or None + return found + + +def dev_packages() -> 'dict[str, Optional[str]]': + """:file:`Pipfile`'s ``[dev-packages]``: each distribution's marker as written, or :data:`None`.""" + pipfile = tomllib.loads((ROOT / 'Pipfile').read_text(encoding='utf-8')) + found = {} # type: dict[str, Optional[str]] + for name, spec in pipfile['dev-packages'].items(): # type: str, Any + found[canonical(name)] = spec.get('markers') if isinstance(spec, dict) else None + return found + + +class TestTheScriptDependencies(unittest.TestCase): + """The ``test`` extra's entries, and the same entries in the Pipfile.""" + + def test_the_test_extra_carries_them_with_their_markers(self) -> None: + test = extra('test') + self.assertEqual({name: test.get(name, 'absent') for name in SCRIPT_DEPENDENCIES}, { + 'requests': None, + 'psutil': "platform_python_implementation == 'CPython'", + 'lxml': "platform_python_implementation != 'GraalVM'", + 'markdown-it-py': None, + }) + + def test_the_pipfile_dev_packages_have_the_test_extras_markers(self) -> None: + test, pipfile = extra('test'), dev_packages() + for name in SCRIPT_DEPENDENCIES: + with self.subTest(name=name): + self.assertIn(name, pipfile, f"the Pipfile's [dev-packages] does not carry {name}") + self.assertEqual(pipfile[name], test[name]) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/project/test_vendor_test_gate_1853.py b/tests/project/test_vendor_test_gate_1853.py index fdd802e02d..a6f8663198 100644 --- a/tests/project/test_vendor_test_gate_1853.py +++ b/tests/project/test_vendor_test_gate_1853.py @@ -15,11 +15,16 @@ * :class:`TestTheGate` runs :file:`util/vendor_test_gate.py` in throwaway repositories, with a stand-in test command that fails on a marker, so which modules it keeps is checked against which ones carry the marker. +* :class:`TestTheMemorySampler` checks how the script sums a process group's + memory through psutil (#1894), against a stand-in for psutil listing real + process ids, and that it falls back to ``ps`` where psutil is not installed. """ from __future__ import annotations +import importlib.util +import io import json import os import pathlib @@ -30,8 +35,10 @@ import sys import tempfile import time +import types import unittest from typing import TYPE_CHECKING +from unittest import mock from tests._support import scale_timeout @@ -89,6 +96,15 @@ def vendor_update() -> 'dict[str, Any]': return yaml.safe_load(WORKFLOW.read_text(encoding='utf-8'))['jobs']['vendor-update'] +def load_gate() -> 'types.ModuleType': + """:file:`util/vendor_test_gate.py`, which is a script and not a package.""" + spec = importlib.util.spec_from_file_location('vendor_test_gate_1853', SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + class TestTheWorkflowTestsBeforeItCommits(unittest.TestCase): """The step the ruling on #1853 asks for, as ``vendor-update`` declares it.""" @@ -387,10 +403,10 @@ def test_a_run_over_the_budget_discards_everything(self) -> None: self.assertIn('budget ran out', result.stdout) def test_a_run_over_the_memory_cap_fails(self) -> None: + """Sampled by psutil where this interpreter has it, and by ``ps`` where not (#1894).""" self.write('pcapkit/const/a.py', module('a', 'regenerated')) - if subprocess.run(['ps', '-A', '-o', 'pgid=', '-o', 'rss='], capture_output=True, - check=False).returncode != 0: # pragma: no cover - self.skipTest('ps cannot list processes here, so memory cannot be sampled') + if load_gate().group_rss(os.getpgrp()) is None: # pragma: no cover + self.skipTest('neither psutil nor ps can read memory here, so it cannot be sampled') start = time.monotonic() result = self.run_gate(memory=32, ALLOCATE=str(256 << 20)) self.assertLess(time.monotonic() - start, 40, 'the run was not stopped at the cap') @@ -439,5 +455,100 @@ def test_a_stopped_gate_puts_head_back_and_kills_its_run(self) -> None: os.kill(runs[0]['pid'], 0) +class _NoSuchProcess(Exception): + """Stands in for ``psutil.NoSuchProcess``.""" + + +class _AccessDenied(Exception): + """Stands in for ``psutil.AccessDenied``.""" + + +class _Process: + """A process as ``psutil.process_iter`` yields it: a real id, and a resident size or an error.""" + + def __init__(self, pid: 'int', rss: 'int' = 0, error: 'Optional[Exception]' = None) -> None: + self.pid, self.rss, self.error = pid, rss, error + + def memory_info(self) -> 'types.SimpleNamespace': + if self.error is not None: + raise self.error + return types.SimpleNamespace(rss=self.rss) + + +class TestTheMemorySampler(unittest.TestCase): + """``group_rss`` sums a process group through psutil, or ``ps`` without it (#1894). + + psutil reads no process group, so the script asks :func:`os.getpgid` of each + process it lists. The stand-in lists real processes -- this one, in its own + group, and a child in a session of its own -- so that question gets real + answers, and only the sizes are made up. + + """ + + def setUp(self) -> None: + self.gate = load_gate() + child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)'], + start_new_session=True) + self.addCleanup(child.wait) + self.addCleanup(child.kill) + self.other = child.pid + self.assertNotEqual(os.getpgid(self.other), os.getpgrp()) + # A process id no process holds: one that has been reaped. + gone = subprocess.Popen([sys.executable, '-c', '']) + gone.wait() + self.gone = gone.pid + + def psutil(self, *processes: '_Process', listing: 'Optional[Exception]' = None) -> 'types.SimpleNamespace': + def process_iter() -> 'list[_Process]': + if listing is not None: + raise listing + return list(processes) + + return types.SimpleNamespace(process_iter=process_iter, Error=Exception, + NoSuchProcess=_NoSuchProcess, AccessDenied=_AccessDenied) + + def rss(self, psutil: 'Any') -> 'Optional[int]': + # ``create``: before #1894 the script had no ``psutil`` to bind over, and + # this should fail there on what it returns. + with mock.patch.object(self.gate, 'psutil', psutil, create=True): + return self.gate.group_rss(os.getpgrp()) + + def test_the_group_is_summed_and_nothing_else(self) -> None: + listed = self.psutil(_Process(os.getpid(), rss=3 << 20), _Process(self.other, rss=1 << 30), + _Process(self.gone, rss=1 << 30)) + self.assertEqual(self.rss(listed), 3 << 20) + + def test_a_process_that_exits_while_it_is_read_is_left_out(self) -> None: + listed = self.psutil(_Process(os.getpid(), rss=3 << 20), _Process(os.getpid(), error=_NoSuchProcess())) + self.assertEqual(self.rss(listed), 3 << 20) + + def test_a_member_it_may_not_read_leaves_the_sample_unknown(self) -> None: + """Rather than short: an undercount would let a run past the cap.""" + listed = self.psutil(_Process(os.getpid(), rss=3 << 20), _Process(os.getpid(), error=_AccessDenied())) + self.assertIsNone(self.rss(listed)) + + def test_a_listing_that_fails_leaves_the_sample_unknown(self) -> None: + self.assertIsNone(self.rss(self.psutil(listing=OSError('no /proc')))) + + def test_without_psutil_ps_samples_the_group(self) -> None: + if subprocess.run(['ps', '-A', '-o', 'pgid=', '-o', 'rss='], capture_output=True, + check=False).returncode != 0: # pragma: no cover + self.skipTest('ps cannot list processes here') + rss = self.rss(None) + self.assertIsInstance(rss, int) + # This process alone holds more than a MiB resident. + self.assertGreater(rss, 1 << 20) + + def test_the_warning_names_the_sampler_in_use(self) -> None: + """What ``report`` says when a sample came back unknown.""" + self.assertEqual(self.gate.SAMPLER, 'ps' if self.gate.psutil is None else 'psutil') + gate = self.gate.Gate([], ['true'], 60, 1 << 30, 1 << 30) + gate.unsampled = True + with mock.patch.dict(os.environ, {'GITHUB_STEP_SUMMARY': ''}), \ + mock.patch('sys.stdout', new_callable=io.StringIO) as stdout: + self.gate.report([], frozenset(), '', gate, 'true') + self.assertIn(f'could not sample memory::{self.gate.SAMPLER} could not read', stdout.getvalue()) + + if __name__ == '__main__': unittest.main() diff --git a/tests/vendor/test_vendor_reg_apptype_generator_unit.py b/tests/vendor/test_vendor_reg_apptype_generator_unit.py index 020929fcc9..4ba8c225d4 100644 --- a/tests/vendor/test_vendor_reg_apptype_generator_unit.py +++ b/tests/vendor/test_vendor_reg_apptype_generator_unit.py @@ -107,7 +107,7 @@ which #766 carries for the sibling isort check. What the gate is worth has not changed. mypy is a :file:`Pipfile` -``[dev-packages]`` entry (line 50) and is in no :file:`pyproject.toml` extra, +``[dev-packages]`` entry (line 54) and is in no :file:`pyproject.toml` extra, so no ``pytest`` job in :file:`.github/workflows/unit-tests.yml` -- every one installs ``.[test,...]`` -- ever has it importable, and this test skips on all of them; it runs where mypy is installed, which is a developer checkout and diff --git a/util/ci_timing.py b/util/ci_timing.py index 6fc5dc1bea..c22bffbe59 100644 --- a/util/ci_timing.py +++ b/util/ci_timing.py @@ -49,8 +49,10 @@ measurement, which costs one call per run of every workflow in that window and is therefore off unless asked for. -Standard library only, so the workflow runs it on a bare interpreter. The token is -``GH_TOKEN`` (or ``GITHUB_TOKEN``); locally, ``GH_TOKEN=$(gh auth token)`` works. +It fetches with :mod:`requests` and parses JUnit XML with lxml (#1894), which +the workflow installs, and needs nothing else outside the standard library. +The token is ``GH_TOKEN`` (or ``GITHUB_TOKEN``); locally, +``GH_TOKEN=$(gh auth token)`` works. """ @@ -69,19 +71,17 @@ import time import tomllib import traceback -import urllib.error import urllib.parse -import urllib.request -# Parses JUnit XML that this repository's own CI wrote, never anything untrusted. -import xml.etree.ElementTree as ET # nosec B405 import zipfile from typing import TYPE_CHECKING +import requests + if TYPE_CHECKING: from typing import Any, Callable, Iterable, Iterator, Optional #: Fetches one URL with an ``Accept`` header; returns the body and the headers, - #: names lower-cased. + #: names lower-cased, and raises :exc:`requests.HTTPError` for an error status. Transport = Callable[[str, str], tuple[bytes, dict[str, str]]] API = 'https://api.github.com' @@ -408,9 +408,21 @@ def step_cap(spec: 'Optional[dict[str, Any]]', name: 'str') -> 'Optional[int]': def next_link(header: 'str') -> 'Optional[str]': - """The ``rel="next"`` URL of a ``Link`` header.""" - match = re.search(r'<([^>]+)>;\s*rel="next"', header or '') - return match.group(1) if match else None + """The ``rel="next"`` URL of a ``Link`` header. + + Parsed as :attr:`requests.Response.links` parses it, from the header the + transport returns, so a test's transport needs to send only the header. + + """ + for link in requests.utils.parse_header_links(header or ''): + if link.get('rel') == 'next': + return link.get('url') + return None + + +def status_of(error: 'requests.HTTPError') -> 'int': + """The HTTP status an error carries, or 0 when it has no response.""" + return getattr(error.response, 'status_code', None) or 0 class Client: @@ -428,22 +440,27 @@ def __init__(self, token: 'str', *, transport: 'Optional[Transport]' = None, max_calls: 'int' = DEFAULT_MAX_CALLS, sleep: 'Callable[[float], None]' = time.sleep) -> 'None': self.token = token - self.transport = transport or self._urlopen + self.transport = transport or self._get self.max_calls = max_calls self.sleep = sleep self.calls = 0 self.rate_remaining = None # type: Optional[int] - - def _urlopen(self, url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': - request = urllib.request.Request(url, headers={ + #: The connections :meth:`_get` reuses, opened on its first call. + self.session = None # type: Optional[requests.Session] + + def _get(self, url: 'str', accept: 'str') -> 'tuple[bytes, dict[str, str]]': + """The default transport: one GET, following redirects.""" + if self.session is None: + self.session = requests.Session() + # requests drops the Authorization header on a redirect to another host + # (Session.rebuild_auth), so an artifact download's redirect to blob + # storage does not carry the token to a host that rejects it. Only https + # URLs reach here: the API root, or a Link/download URL it sent. + response = self.session.get(url, timeout=60, headers={ 'Accept': accept, 'X-GitHub-Api-Version': '2022-11-28', - 'User-Agent': 'PyPCAPKit-ci-timing'}) - # Unredirected, so an artifact download's redirect to blob storage does not - # carry the token to a host that rejects it. - request.add_unredirected_header('Authorization', f'Bearer {self.token}') - # Only https URLs reach here: the API root, or a Link/download URL it sent. - with urllib.request.urlopen(request, timeout=60) as response: # nosec B310 - return response.read(), {k.lower(): v for k, v in response.headers.items()} + 'User-Agent': 'PyPCAPKit-ci-timing', 'Authorization': f'Bearer {self.token}'}) + response.raise_for_status() + return response.content, {k.lower(): v for k, v in response.headers.items()} def fetch(self, url: 'str', accept: 'str' = 'application/vnd.github+json') -> 'tuple[bytes, dict[str, str]]': """One request; ``url`` may be a path under the API root.""" @@ -457,10 +474,12 @@ def fetch(self, url: 'str', accept: 'str' = 'application/vnd.github+json') -> 't self.calls += 1 try: body, headers = self.transport(url, accept) - except urllib.error.HTTPError as error: - if wait is None or error.code < 500: + except requests.HTTPError as error: + if wait is None or status_of(error) < 500: raise - except OSError: # URLError, and a timeout or reset mid-read + # requests' ConnectionError and Timeout, a timeout or reset mid-read + # among them, are OSErrors, as its HTTPError is. + except OSError: if wait is None: raise else: @@ -595,8 +614,8 @@ def collect(client: 'Client', repo: 'str', *, now: 'datetime.datetime', days: 'f if text_sha == sha] try: text = known[0] if known else client.text(path, ref=sha) - except urllib.error.HTTPError as error: - if error.code != 404: + except requests.HTTPError as error: + if status_of(error) != 404: raise data['notes'].append(f'{workflow} is not at {sha[:9]}, so its {label} caps are unknown.') continue @@ -637,9 +656,27 @@ def collect(client: 'Client', repo: 'str', *, now: 'datetime.datetime', days: 'f return data +def parse_xml(raw: 'bytes') -> 'Any': + """``raw``'s root element, parsed strictly by lxml (#1894). + + The JUnit files are ones this repository's own CI wrote, but the parser + trusts nothing in them. Malformed XML raises + :exc:`lxml.etree.XMLSyntaxError`, and so does nesting past libxml2's depth + limit, which ``huge_tree=False`` keeps. No DTD is loaded and nothing is + fetched: an entity in text is left unexpanded, and an attribute that + references an external one is an error, so an entity naming a file or a URL + is never read. lxml is imported here, so only a run that reads JUnit needs it. + + """ + from lxml import etree # pylint: disable=import-outside-toplevel + + parser = etree.XMLParser(resolve_entities=False, no_network=True, huge_tree=False, load_dtd=False) + return etree.fromstring(raw, parser) + + def parse_junit(raw: 'bytes') -> 'dict[str, Any]': """The testcases of one pytest JUnit file, and its suite's ``tests=`` total.""" - root = ET.fromstring(raw) # nosec B314 + root = parse_xml(raw) suites = [root] if root.tag == 'testsuite' else root.findall('testsuite') cases = [(case.get('classname') or '', case.get('name') or '', float(case.get('time') or 0)) for suite in suites for case in suite.iter('testcase')] diff --git a/util/pyshark_encap_map.py b/util/pyshark_encap_map.py index d332d9fbcb..03643bb1bf 100644 --- a/util/pyshark_encap_map.py +++ b/util/pyshark_encap_map.py @@ -108,7 +108,8 @@ 4.6.9 is what every measurement here was taken against); their absence is reported on :data:`sys.stderr` and exits cleanly rather than raising, so a host without Wireshark installed does not turn "nothing to regenerate" into a -traceback. +traceback. It reads the PDML with lxml (#1894), which the ``test`` extra +installs except on GraalPy, which has no wheel for it. This cannot be run in CI to verify the committed tables, only locally against a pinned Wireshark @@ -151,14 +152,13 @@ import subprocess import sys import tempfile -import xml.etree.ElementTree as ET from typing import TYPE_CHECKING, NamedTuple if TYPE_CHECKING: from typing import Any, Optional, Sequence __all__ = [ - 'EncapSweepError', 'parse_parenthesised_number', 'list_encap_types', + 'EncapSweepError', 'parse_parenthesised_number', 'parse_pdml', 'list_encap_types', 'measure_encap', 'build_tables', 'render_dict', 'rewrite', 'main', ] @@ -266,6 +266,30 @@ def parse_parenthesised_number(showname: str) -> int: return int(match.group(1)) +def parse_pdml(document: bytes) -> 'Any': + """The root element of :program:`tshark`'s PDML, parsed strictly by lxml (#1894). + + Bytes, not text: the document declares its own encoding, which lxml reads + and refuses to be handed an already-decoded string with. Malformed XML + raises :exc:`lxml.etree.XMLSyntaxError`, and so does nesting past libxml2's + depth limit, which ``huge_tree=False`` keeps. No DTD is loaded and nothing + is fetched: an entity in text is left unexpanded, and an attribute that + references an external one is an error, so an entity naming a file or a URL + is never read. lxml is imported here, so only a sweep needs it. + + Args: + document: ``tshark -T pdml``'s standard output. + + Returns: + The ```` element. + + """ + from lxml import etree # pylint: disable=import-outside-toplevel + + parser = etree.XMLParser(resolve_entities=False, no_network=True, huge_tree=False, load_dtd=False) + return etree.fromstring(document, parser) + + def list_encap_types(editcap: str) -> 'list[str]': """The ``editcap -T`` tokens this binary accepts, in the order it lists them. @@ -333,9 +357,9 @@ def measure_encap(editcap: str, tshark: str, source: pathlib.Path, pdml = subprocess.run( [tshark, '-r', str(out_path), '-c', '1', '-T', 'pdml'], - capture_output=True, text=True, check=False, + capture_output=True, check=False, ) - root = ET.fromstring(pdml.stdout) + root = parse_pdml(pdml.stdout) packet = root.find('packet') if packet is None: return Missing(name, 'tshark produced no packet in its PDML output') diff --git a/util/update_python_versions.py b/util/update_python_versions.py index 64190c9e01..ca8065439e 100644 --- a/util/update_python_versions.py +++ b/util/update_python_versions.py @@ -103,8 +103,6 @@ import tempfile import time import tomllib -import urllib.error -import urllib.request from typing import TYPE_CHECKING, NamedTuple if TYPE_CHECKING: @@ -177,14 +175,23 @@ class Source(NamedTuple): def fetch(name: 'str', attempts: 'int' = 3) -> 'str': - """Source ``name``'s text, retried twice on a network error.""" + """Source ``name``'s text, retried twice on a network error or an HTTP error status. + + Fetched with :mod:`requests` (#1894), imported here rather than at the top: + ``detect`` installs it, but ``pr`` runs ``publish`` on the runner's own + ``python3`` with nothing installed, and ``publish`` never fetches. + + """ + import requests # pylint: disable=import-outside-toplevel + source = SOURCES[name] - request = urllib.request.Request(source.url, headers={'User-Agent': 'PyPCAPKit cron-python-versions (#1660)'}) for attempt in range(attempts): try: - with urllib.request.urlopen(request, timeout=60) as response: # nosec B310 -- fixed https URLs - return response.read().decode('utf-8') - except (urllib.error.URLError, OSError) as error: + response = requests.get(source.url, timeout=60, + headers={'User-Agent': 'PyPCAPKit cron-python-versions (#1660)'}) + response.raise_for_status() + return response.content.decode('utf-8') + except OSError as error: # every requests.RequestException is one if attempt + 1 == attempts: raise OSError(f'{source.url}: {error}') from error time.sleep(5 * (attempt + 1)) @@ -242,27 +249,56 @@ def numeric(version: 'str') -> 'tuple[int, ...]': return tuple(map(int, match.group(0).split('.'))) +def markdown_tables(text: 'str') -> 'list[list[list[Any]]]': + """Each GFM table in ``text``: its rows, header first, each a list of its cells' inline tokens. + + Parsed with markdown-it-py (#1894), imported here for the reason + :func:`fetch` imports :mod:`requests` there. An escaped pipe, ``\\|``, stays + inside its cell, in a code span too, as GitHub renders it; splitting each + line on ``|`` cut such a cell in two and shifted every column after it. + + """ + from markdown_it import MarkdownIt # pylint: disable=import-outside-toplevel + + tables = [] # type: list[list[list[Any]]] + row = None # type: Optional[list[Any]] + for token in MarkdownIt('commonmark').enable('table').parse(text): + if token.type == 'table_open': + tables.append([]) + elif token.type == 'tr_open': + row = [] + tables[-1].append(row) + elif token.type == 'tr_close': + row = None + elif token.type == 'inline' and row is not None: + row.append(token) + return tables + + def parse_images(text: 'str') -> 'list[Image]': - """The Ubuntu and macOS rows of the runner-images README's image table.""" - rows = [line.strip() for line in text.splitlines() if line.strip().startswith('|')] - header = next((index for index, row in enumerate(rows) if 'YAML Label' in row), None) - if header is None: + """The Ubuntu and macOS rows of the runner-images README's image table. + + The labels are the code spans of the YAML Label cell, read from its tokens, + so a span holding an escaped pipe reads as GitHub shows it, without the + backslash a backtick pattern over the cell's source would keep. + + """ + table = next((rows for rows in markdown_tables(text) + if any('YAML Label' in cell.content for cell in rows[0])), None) + if table is None: raise ValueError('the runner-images README has no table with a "YAML Label" column') - columns = [cell.strip() for cell in rows[header].strip('|').split('|')] + columns = [cell.content.strip() for cell in table[0]] try: image, arch, label = columns.index('Image'), columns.index('Architecture'), columns.index('YAML Label') except ValueError as error: raise ValueError(f'the runner-images table has columns {columns}') from error images = [] - for row in rows[header + 1:]: - cells = [cell.strip() for cell in row.strip('|').split('|')] - if len(cells) < len(columns): - continue - match = IMAGE.match(cells[image]) + for cells in table[1:]: + match = IMAGE.match(cells[image].content.strip()) if match is None: continue - images.append(Image(match.group(1).lower(), match.group(2), cells[arch], - tuple(re.findall(r'`([^`]+)`', cells[label])))) + labels = tuple(child.content for child in cells[label].children or () if child.type == 'code_inline') + images.append(Image(match.group(1).lower(), match.group(2), cells[arch].content.strip(), labels)) if not any(found.family == 'ubuntu' for found in images): raise ValueError('the runner-images README lists no Ubuntu image') return images diff --git a/util/vendor_test_gate.py b/util/vendor_test_gate.py index 028e158b87..6e561e1503 100644 --- a/util/vendor_test_gate.py +++ b/util/vendor_test_gate.py @@ -38,8 +38,10 @@ **Limits.** Each run is a child in a session of its own, with ``RLIMIT_FSIZE`` at ``--max-file-size-mib``. It is killed, and counts as failing, once its process group holds more than ``--max-memory-mib`` resident, sampled with -``ps``: ``RLIMIT_AS`` caps address space rather than resident memory, and the -job runs on ``macos-latest``, which has no cgroup to cap it with. +:mod:`psutil` (#1894), or with ``ps`` where psutil is not installed (the test +extra installs it on CPython only): ``RLIMIT_AS`` caps address space rather +than resident memory, and the job runs on ``macos-latest``, which has no cgroup +to cap it with. ``--budget-minutes`` bounds every run together; when it runs out before a decision, every change is discarded. Each run imports through a ``PYTHONPYCACHEPREFIX`` of its own, removed after it, since a module swapped @@ -71,6 +73,11 @@ import time from typing import TYPE_CHECKING, NamedTuple +try: + import psutil +except ImportError: # the test extra installs it on CPython only + psutil = None # type: ignore[assignment] + if TYPE_CHECKING: from typing import FrozenSet, NoReturn, Optional, Sequence @@ -154,8 +161,48 @@ def apply(changes: 'Sequence[Change]', selected: 'FrozenSet[str]') -> None: file.write(content) +#: What samples a run's memory: psutil, or ``ps`` where psutil is not installed. +SAMPLER = 'ps' if psutil is None else 'psutil' + + def group_rss(pgid: 'int') -> 'Optional[int]': - """Bytes resident in the processes of group ``pgid``, or :data:`None` if ``ps`` cannot say.""" + """Bytes resident in the processes of group ``pgid``, or :data:`None` if :data:`SAMPLER` cannot say.""" + if psutil is None: + return _ps_group_rss(pgid) + return _psutil_group_rss(pgid) + + +def _psutil_group_rss(pgid: 'int') -> 'Optional[int]': + """:func:`group_rss` through psutil, which has no figure per process group. + + So it is each process of the group, by :func:`os.getpgid`, summed. A process + that exits between the listing and its reading is left out, as ``ps`` would + not have listed it; one whose memory psutil may not read leaves the sample + unknown rather than short. + + """ + try: + processes = list(psutil.process_iter()) + except (psutil.Error, OSError): + return None + total = 0 + for process in processes: + try: + if os.getpgid(process.pid) != pgid: + continue + except OSError: # exited since; or, where getpgid stops at a session, not in ours + continue + try: + total += process.memory_info().rss + except psutil.NoSuchProcess: # exited since, a zombie included + continue + except psutil.AccessDenied: + return None + return total + + +def _ps_group_rss(pgid: 'int') -> 'Optional[int]': + """:func:`group_rss` through ``ps``, where psutil is not installed.""" try: listing = subprocess.run(['ps', '-A', '-o', 'pgid=', '-o', 'rss='], # nosec B603 B607 capture_output=True, check=False, timeout=30) @@ -343,8 +390,8 @@ def report(changes: 'Sequence[Change]', kept: 'FrozenSet[str]', reason: 'str', g lines.extend(f'| {run.number} | {run.modules} of {len(changes)} | {run.result} | {run.seconds:.0f} |' for run in gate.runs) if gate.unsampled: - print('::warning title=Vendor test gate could not sample memory::ps did not answer, so ' - 'the memory cap was not enforced on every sample') + print(f'::warning title=Vendor test gate could not sample memory::{SAMPLER} could not read ' + "the run's memory, so the memory cap was not enforced on every sample") summary = os.environ.get('GITHUB_STEP_SUMMARY') if summary: with open(summary, 'a', encoding='utf-8') as file: