From 704f734b79057967e54b0060c453931ff3e6fcd2 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Tue, 23 Jun 2026 08:57:21 -0500 Subject: [PATCH] test: unify a shared flaky-rerun marker; fix nwis collection-time call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Ubuntu CI on main (run 27969553989, on the #326 merge) failed: a transient 503 from the live legacy NWIS site service hit the live tests in nwis_test.py. Windows passed by timing. Not a regression from #326 — the live tests and the offending call date to #62. Two things made the transient fatal: - nwis_test.py had no flaky-rerun marker (waterdata/ngwmn got one in #325), and - TestTZ fetched its sites in the class body (`sites, _ = what_sites(...)`), i.e. at collection time, where a 503 aborts the whole module and can never be reran (rerunfailures retries failed tests, not collection errors). Changes: - Add one shared marker, `conftest.flaky_api`, with a unified transient pattern list, and apply it to every live suite: nwis_test.py / waterdata_test.py / ngwmn_test.py (module-level) and utils_test.py::Test_query (class-level — the only other live suite, found by auditing each module behind a dead proxy). Replaces three drifted inline copies. - The status pattern now matches both the OGC path's ":" and the legacy query path's "HTTP " message shapes, so one list serves all four. This closes a latent gap: ngwmn's old marker omitted ServiceUnavailable and the chunked QuotaExhausted/ServiceInterrupted wrappers (#325 fixed waterdata but missed ngwmn), so a 503 in an ngwmn live test would have failed CI too. - Move TestTZ's class-body fetch into a class-scoped `sites` fixture so it runs at test time, where the marker can retry a transient. Everything else is mocked, module-skipped (nadp), or — the chunking ..._on_real_transport test — a deterministic local http.server, not external. Verified: nwis collection is network-free; the markers attach with the unified config; ngwmn now covers ServiceUnavailable/ServiceInterrupted; live Test_query and TestTZ pass. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Sjb14HkwuCydKSKMsaXsgd --- tests/conftest.py | 24 ++++++++++++++++++++++++ tests/ngwmn_test.py | 13 ++++--------- tests/nwis_test.py | 21 ++++++++++++++++----- tests/utils_test.py | 4 ++++ tests/waterdata_test.py | 31 +++++-------------------------- 5 files changed, 53 insertions(+), 40 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7f222011..13a27062 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,6 +14,30 @@ import pytest +#: Trace patterns that ``pytest-rerunfailures`` retries on the live-API test +#: modules: a transient upstream 429/5xx or dropped connection is retried, +#: deterministic failures (assertion errors, 4xx, etc.) are not. The OGC engine +#: renders a status error as ``": ..."`` while the legacy ``query`` path +#: renders ``"HTTP ..."``, so the status pattern allows either shape; +#: the chunked fan-out wraps a transient sub-request as ``QuotaExhausted`` / +#: ``ServiceInterrupted``. +_TRANSIENT_RERUN_PATTERNS = [ + r"(?:RateLimited|ServiceUnavailable|RuntimeError):\s*(?:HTTP\s+)?(?:429|5\d\d)", + r"(?:QuotaExhausted|ServiceInterrupted):", + r"Connect(ion)?Error", # requests' ConnectionError + httpx' ConnectError + r"ReadTimeout|ConnectTimeout|Timeout", +] + +#: Apply to a test module (``pytestmark = flaky_api``) or class (``@flaky_api``) +#: that hits live USGS services, so a transient upstream failure is retried +#: instead of failing CI. Mocked tests are unaffected — the patterns match only +#: real round-trip error traces. +flaky_api = pytest.mark.flaky( + reruns=2, + reruns_delay=5, + only_rerun=_TRANSIENT_RERUN_PATTERNS, +) + def pytest_collection_modifyitems(config, items): """Apply relaxed ``pytest-httpx`` strict-mode settings to every test diff --git a/tests/ngwmn_test.py b/tests/ngwmn_test.py index 77b73d6e..ac9b191b 100644 --- a/tests/ngwmn_test.py +++ b/tests/ngwmn_test.py @@ -16,16 +16,11 @@ from dataretrieval import ngwmn from dataretrieval.utils import BaseMetadata +from tests.conftest import flaky_api -pytestmark = pytest.mark.flaky( - reruns=2, - reruns_delay=5, - only_rerun=[ - r"(?:RateLimited|RuntimeError):\s*(?:429|5\d\d):", - r"Connect(ion)?Error", - r"ReadTimeout|ConnectTimeout|Timeout", - ], -) +# These hit the live NGWMN OGC API, so retry transient upstream failures +# rather than flaking CI (see ``conftest.flaky_api``). +pytestmark = flaky_api # A site with water-level, construction, and lithology records (per the R # dataRetrieval NGWMN examples), plus a non-USGS-agency id to exercise the diff --git a/tests/nwis_test.py b/tests/nwis_test.py index dab46d6a..5eb379cd 100644 --- a/tests/nwis_test.py +++ b/tests/nwis_test.py @@ -24,6 +24,7 @@ preformat_peaks_response, what_sites, ) +from tests.conftest import flaky_api START_DATE = "2018-01-24" END_DATE = "2018-01-25" @@ -31,6 +32,10 @@ DATETIME_COL = "datetime" SITENO_COL = "site_no" +# Several tests in this module hit the live NWIS services, so retry a transient +# upstream failure rather than failing CI (see ``conftest.flaky_api``). +pytestmark = flaky_api + def _load_mock_json(file_name): """Helper to load mock JSON from tests/data.""" @@ -254,21 +259,27 @@ def test_get_record_defunct_service_water_use(self): class TestTZ: """Tests relating to GitHub Issue #60.""" - sites, _ = what_sites(stateCd="MD") + @pytest.fixture(scope="class") + def sites(self): + # Fetch once per class, at test time (not at collection) so a transient + # upstream failure is retried by the module ``flaky`` marker instead of + # aborting collection — a class-body call cannot be reran. + sites, _ = what_sites(stateCd="MD") + return sites - def test_multiple_tz_01(self): + def test_multiple_tz_01(self, sites): """Test based on GitHub Issue #60 - error merging different time zones.""" # this test fails before issue #60 is fixed - iv, _ = get_iv(sites=self.sites.site_no.values[:25].tolist()) + iv, _ = get_iv(sites=sites.site_no.values[:25].tolist()) # assert that the datetime column exists assert "datetime" in iv.index.names # assert that it is a datetime type assert isinstance(iv.index[0][1], datetime.datetime) - def test_multiple_tz_02(self): + def test_multiple_tz_02(self, sites): """Test based on GitHub Issue #60 - confirm behavior for same tz.""" # this test passes before issue #60 is fixed - iv, _ = get_iv(sites=self.sites.site_no.values[:20].tolist()) + iv, _ = get_iv(sites=sites.site_no.values[:20].tolist()) # assert that the datetime column exists assert "datetime" in iv.index.names # assert that it is a datetime type diff --git a/tests/utils_test.py b/tests/utils_test.py index abff0974..77b090ae 100644 --- a/tests/utils_test.py +++ b/tests/utils_test.py @@ -6,8 +6,12 @@ import pytest from dataretrieval import exceptions, nwis, utils +from tests.conftest import flaky_api +# Test_query hits the live NWIS services (the rest of this module is mocked), +# so retry transient upstream failures rather than flaking CI. +@flaky_api class Test_query: """Tests of the query function.""" diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index b48859c4..92b68618 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -40,33 +40,12 @@ _get_args, _normalize_str_iterable, ) +from tests.conftest import flaky_api -# Most tests in this module call the live USGS Water Data API. After -# PR #273, transient upstream errors (5xx / 429 / connection drops) -# propagate instead of silently truncating, which makes CI susceptible -# to flaking on a brief upstream blip. Auto-retry such failures, but -# only for the narrow set of transient-error trace patterns below — -# library bugs raising other exception types still fail on the first -# try. The marker is attached to every test in the module, but the -# patterns match only traces produced by real network round-trips -# (``_raise_for_non_200`` output, ``requests`` exceptions), so tests -# using ``httpx_mock`` or ``mock.patch`` are no-ops for the rerun. -pytestmark = pytest.mark.flaky( - reruns=2, - reruns_delay=5, - only_rerun=[ - # Transient HTTP errors (429 / 5xx) on the direct path: RateLimited / - # ServiceUnavailable carry a ": ..." message (the RuntimeError - # shape is kept for any legacy call site). - r"(?:RateLimited|ServiceUnavailable|RuntimeError):\s*(?:429|5\d\d):", - # The chunked fan-out wraps a transient sub-request as a ChunkInterrupted - # subclass (QuotaExhausted for 429, ServiceInterrupted for 5xx), whose - # message has no leading status token. - r"(?:QuotaExhausted|ServiceInterrupted):", - r"Connect(ion)?Error", # requests' ConnectionError + httpx' ConnectError - r"ReadTimeout|ConnectTimeout|Timeout", - ], -) +# Most tests in this module call the live USGS Water Data API; transient +# upstream errors propagate (since #273) instead of silently truncating, so +# retry them rather than flaking CI (see ``conftest.flaky_api``). +pytestmark = flaky_api @pytest.fixture(autouse=True)