From 7cf9b87ca0a2a2f75d9ce49d025d17af24364494 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 09:57:26 -0500 Subject: [PATCH 1/8] Auto-retry live-API tests on transient upstream errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #273 surfaced a transient HTTP 502 from upstream on its merge-to-main CI run that pre-PR-273 would have been silently swallowed by _walk_pages and turned into an empty DataFrame. The status-code-aware behavior is correct for users, but it makes every test that hits the live USGS Water Data API susceptible to flaking on a transient blip. This PR adds: - pytest-rerunfailures to the `test` optional dependency set. - tests/conftest.py that (a) registers a `live` pytest marker; (b) auto-applies it to every test that does not take `requests_mock` as a fixture (the existing mock-driven convention in this repo); and (c) configures live-marked tests to retry up to twice on a 5-second backoff — but ONLY when the failure trace matches one of a narrow set of transient-upstream patterns: `429:` / `5xx:` prefixes from `_raise_for_non_200`, `ConnectionError` shapes, and timeout strings from the requests/urllib3 stack. Library bugs raising other exception types are not retried — the `only_rerun` pattern set deliberately matches on the failure-text shape produced by `_raise_for_non_200` rather than on generic `RuntimeError`, so a real bug fails on the first attempt. Verified: pytest collection identifies 59 of 61 tests in waterdata_test.py as live (the two `*_mock_*` tests correctly stay unmarked). Offline tests (`_get_args`, `_check_*`, `_normalize_str_*`, `_construct_api_requests_*`) pass unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- NEWS.md | 2 ++ pyproject.toml | 1 + tests/conftest.py | 71 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 tests/conftest.py diff --git a/NEWS.md b/NEWS.md index 18fb12b5..1aade894 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,5 @@ +**05/15/2026:** Live-API tests are now auto-retried on transient upstream errors. PR #273 (above) intentionally surfaces mid-pagination HTTP failures to callers instead of silently truncating results, which made any CI test that hits the live USGS Water Data API susceptible to flaking on transient 429/5xx/connection blips — a 502 Bad Gateway from upstream broke CI on the #273 merge to main. A new `tests/conftest.py` registers a `live` pytest marker, auto-applies it to every test that does not take `requests_mock` (the existing mock-driven convention), and via `pytest-rerunfailures` retries those tests up to twice on a 5-second backoff — but only when the failure trace matches a narrow transient-upstream pattern (`429:` / `5xx:` prefixes from `_raise_for_non_200`, plus `ConnectionError` / timeout shapes). Library bugs raising other exception types still fail on the first attempt. + **05/14/2026:** Fixed two latent bugs in the paginated `waterdata` request loop (`_walk_pages` and `get_stats_data`). Previously, when `requests.Session.request(...)` itself raised mid-pagination (network error, timeout), the except block called `_error_body()` on the *prior page's* response, so the logged "error" described the wrong request and could itself crash on non-JSON bodies. Separately, no status-code check was performed on subsequent paginated responses, so a 5xx body that didn't include `numberReturned` was silently treated as an empty page — pagination quietly stopped and the user got truncated data with no error logged. The loop now status-checks each page like the initial request and reports the actual exception. The "best-effort" behavior (return whatever pages were collected) is preserved. **05/07/2026:** Bumped the declared minimum Python version from **3.8** to **3.9** (`pyproject.toml`'s `requires-python` and the ruff target). This brings the manifest in line with what was already being tested — CI's matrix has long covered only 3.9, 3.13, and 3.14, the `waterdata` test module already skipped itself on Python < 3.10, and several modules already use 3.9-only stdlib (e.g. `zoneinfo`). Users on 3.8 will no longer be able to install the package; please upgrade. diff --git a/pyproject.toml b/pyproject.toml index 35edcc5e..5c9fbda0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -35,6 +35,7 @@ dataretrieval = ["py.typed"] test = [ "pytest > 5.0.0", "pytest-cov[all]", + "pytest-rerunfailures", "coverage", "requests-mock", "ruff", diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..d397c708 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,71 @@ +"""Test infrastructure shared across all test modules. + +After #273 the paginated ``waterdata`` getters surface mid-walk HTTP +errors (429 / 5xx / connection drops) to the caller instead of silently +truncating the result. That's the correct behavior for users — but it +makes any CI test that hits the live USGS Water Data API susceptible to +flaking on a transient upstream blip (e.g. the 502 Bad Gateway that +broke CI on #273's merge to main). + +This file: + +* registers a ``live`` pytest marker; +* auto-applies it to every test that does **not** take ``requests_mock`` + as a fixture — the existing convention in this repo for mock-driven + tests, so the marker tracks "this test hits the network" without + needing to decorate 35 functions by hand; +* via ``pytest-rerunfailures``, configures live-marked tests to retry + up to twice (5-second backoff) ONLY when the failure trace matches a + transient-upstream pattern: ``429:`` / ``5xx:`` prefixes that + ``_raise_for_non_200`` produces, plus ``ConnectionError`` / timeout + shapes from the ``requests`` library. + +Library bugs that raise unrelated exception types are NOT retried — +the regex set deliberately omits generic ``RuntimeError`` matches. +""" + +import pytest + +# Match anywhere in the failure traceback. Anchor-free because the +# trace embeds the exception message after a long preamble. +_TRANSIENT_PATTERNS = [ + r"RuntimeError:\s*(?:429|5\d\d):", # _raise_for_non_200 output + r"ConnectionError", # requests/urllib3 + r"ReadTimeout|ConnectTimeout|Timeout", + r"timed out", + r"Bad Gateway|Service Unavailable|Gateway Timeout", +] + +_MAX_RERUNS = 2 +_RERUN_DELAY_SEC = 5 + + +def pytest_configure(config): + """Register the ``live`` marker so pytest doesn't warn about it.""" + config.addinivalue_line( + "markers", + "live: marks tests that hit the live USGS Water Data API; " + "retried up to twice on transient upstream HTTP errors.", + ) + + +def pytest_collection_modifyitems(config, items): + """Auto-mark live-API tests with ``live`` + ``flaky`` retry config. + + Heuristic: any test that does NOT request the ``requests_mock`` + fixture is treated as live. This catches every test in + ``waterdata_test.py`` and similar modules without needing to + decorate them individually, and it auto-tracks new tests written + in the same style. + """ + for item in items: + if "requests_mock" in getattr(item, "fixturenames", ()): + continue + item.add_marker(pytest.mark.live) + item.add_marker( + pytest.mark.flaky( + reruns=_MAX_RERUNS, + reruns_delay=_RERUN_DELAY_SEC, + only_rerun=_TRANSIENT_PATTERNS, + ) + ) From 72854adb8d3afb95909eaa269f7241aa82524a24 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:03:57 -0500 Subject: [PATCH 2/8] Apply review feedback to PR #277 + bump Actions to Node-24 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify-review consensus across three reviewers (reuse / quality / efficiency) on the conftest.py from the previous commit: - Drop the `pytest.mark.live` marker double-application: no `-m live` selector is wired anywhere in CI; only `pytest.mark.flaky` carries behavior. The `live` marker plus its `pytest_configure` registration was dead metadata. - Tighten `_TRANSIENT_PATTERNS`: drop `r"timed out"` (subsumed by the `Timeout` arm) and `r"Bad Gateway|Service Unavailable|Gateway Timeout"` (subsumed by the `RuntimeError:\s*5\d\d:` prefix from `_raise_for_non_200`). Three load-bearing patterns left. - Drop defensive `getattr(item, "fixturenames", ())` — pytest items reliably expose `fixturenames`; the default-tuple fallback is dead defense. - Shorten the module docstring: keep the WHY paragraph (PR #273 → surfaced errors → CI flake risk) and drop the WHAT bullets that narrate the code below. - Add a one-line justification for the 5-second backoff (USGS upstream typically recovers from brief 5xx within that window). - Tighten the NEWS.md entry: drop conftest implementation detail (registers a marker, applies via collection hook, …) and keep the user-relevant claim. Skipped from the review (reviewer can decide): - Inverting the auto-mark heuristic to opt-in `@pytest.mark.live` would force decorating ~35 test functions; the current "everything not using requests_mock is live" is zero-cost on success and self-extending for new tests. - Centralizing the transient-error regex set in `dataretrieval/ waterdata/utils.py` would couple library code to test infrastructure for one pattern. Separately, bump GitHub Actions to Node-24-compatible versions (@v6 across all three workflows) ahead of the deprecation warning surfaced on the previous CI run: actions/checkout@v4 → v6, actions/setup-python@v5 → v6. Node 20 is deprecated and will be removed from runners on 2026-09-16; the default flips on 2026-06-02. NEWS.md entry updated to mention the bump. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/python-package.yml | 8 ++-- .github/workflows/python-publish.yml | 4 +- .github/workflows/sphinx-docs.yml | 4 +- NEWS.md | 2 +- tests/conftest.py | 68 ++++++++-------------------- 5 files changed, 28 insertions(+), 58 deletions(-) diff --git a/.github/workflows/python-package.yml b/.github/workflows/python-package.yml index c24b4e6a..0ab3d142 100644 --- a/.github/workflows/python-package.yml +++ b/.github/workflows/python-package.yml @@ -13,9 +13,9 @@ jobs: lint: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Python 3.14 - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: "3.14" cache: "pip" @@ -36,9 +36,9 @@ jobs: python-version: ["3.9", "3.13", "3.14"] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: ${{ matrix.python-version }} cache: "pip" diff --git a/.github/workflows/python-publish.yml b/.github/workflows/python-publish.yml index 0a493ccd..1fd3e112 100644 --- a/.github/workflows/python-publish.yml +++ b/.github/workflows/python-publish.yml @@ -21,9 +21,9 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: '3.x' cache: 'pip' diff --git a/.github/workflows/sphinx-docs.yml b/.github/workflows/sphinx-docs.yml index 06909c84..2018279e 100644 --- a/.github/workflows/sphinx-docs.yml +++ b/.github/workflows/sphinx-docs.yml @@ -11,11 +11,11 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v4 + uses: actions/checkout@v6 with: persist-credentials: false - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: "3.13" cache: "pip" diff --git a/NEWS.md b/NEWS.md index 1aade894..7db6f222 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -**05/15/2026:** Live-API tests are now auto-retried on transient upstream errors. PR #273 (above) intentionally surfaces mid-pagination HTTP failures to callers instead of silently truncating results, which made any CI test that hits the live USGS Water Data API susceptible to flaking on transient 429/5xx/connection blips — a 502 Bad Gateway from upstream broke CI on the #273 merge to main. A new `tests/conftest.py` registers a `live` pytest marker, auto-applies it to every test that does not take `requests_mock` (the existing mock-driven convention), and via `pytest-rerunfailures` retries those tests up to twice on a 5-second backoff — but only when the failure trace matches a narrow transient-upstream pattern (`429:` / `5xx:` prefixes from `_raise_for_non_200`, plus `ConnectionError` / timeout shapes). Library bugs raising other exception types still fail on the first attempt. +**05/15/2026:** CI test suite is now resilient to transient USGS Water Data API blips. PR #273 (above) intentionally surfaces mid-pagination HTTP errors to callers rather than silently truncating — the right behavior, but it made any live-API test in CI flaky against transient 429/5xx/connection errors. A new `tests/conftest.py` uses `pytest-rerunfailures` to retry live-API tests up to twice on a 5-second backoff, but only when the failure trace matches a narrow transient-upstream pattern. Library bugs raising other exception types still fail on the first attempt. Also bumps `actions/checkout` and `actions/setup-python` to Node-24-compatible versions (`@v6`) ahead of the GitHub Actions Node 20 runtime deprecation. **05/14/2026:** Fixed two latent bugs in the paginated `waterdata` request loop (`_walk_pages` and `get_stats_data`). Previously, when `requests.Session.request(...)` itself raised mid-pagination (network error, timeout), the except block called `_error_body()` on the *prior page's* response, so the logged "error" described the wrong request and could itself crash on non-JSON bodies. Separately, no status-code check was performed on subsequent paginated responses, so a 5xx body that didn't include `numberReturned` was silently treated as an empty page — pagination quietly stopped and the user got truncated data with no error logged. The loop now status-checks each page like the initial request and reports the actual exception. The "best-effort" behavior (return whatever pages were collected) is preserved. diff --git a/tests/conftest.py b/tests/conftest.py index d397c708..0eb07d15 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,71 +1,41 @@ -"""Test infrastructure shared across all test modules. - -After #273 the paginated ``waterdata`` getters surface mid-walk HTTP -errors (429 / 5xx / connection drops) to the caller instead of silently -truncating the result. That's the correct behavior for users — but it -makes any CI test that hits the live USGS Water Data API susceptible to -flaking on a transient upstream blip (e.g. the 502 Bad Gateway that -broke CI on #273's merge to main). - -This file: - -* registers a ``live`` pytest marker; -* auto-applies it to every test that does **not** take ``requests_mock`` - as a fixture — the existing convention in this repo for mock-driven - tests, so the marker tracks "this test hits the network" without - needing to decorate 35 functions by hand; -* via ``pytest-rerunfailures``, configures live-marked tests to retry - up to twice (5-second backoff) ONLY when the failure trace matches a - transient-upstream pattern: ``429:`` / ``5xx:`` prefixes that - ``_raise_for_non_200`` produces, plus ``ConnectionError`` / timeout - shapes from the ``requests`` library. - -Library bugs that raise unrelated exception types are NOT retried — -the regex set deliberately omits generic ``RuntimeError`` matches. +"""Auto-retry live-API tests on transient upstream errors. + +After PR #273, paginated ``waterdata`` getters propagate mid-walk HTTP +errors (429 / 5xx / connection drops) instead of silently truncating the +result. That's the correct behavior for users but makes any CI test that +hits the live USGS Water Data API susceptible to flaking on a transient +upstream blip — e.g. the HTTP 502 Bad Gateway that broke CI on PR #273's +merge to main. + +Heuristic: any test that does NOT request the ``requests_mock`` fixture +is treated as live and gets retried on transient-error patterns only. +Library bugs raising other exception types still fail on the first try. """ import pytest -# Match anywhere in the failure traceback. Anchor-free because the -# trace embeds the exception message after a long preamble. +# Anchored loosely — the failure trace embeds the exception message +# after a long preamble. _TRANSIENT_PATTERNS = [ r"RuntimeError:\s*(?:429|5\d\d):", # _raise_for_non_200 output r"ConnectionError", # requests/urllib3 r"ReadTimeout|ConnectTimeout|Timeout", - r"timed out", - r"Bad Gateway|Service Unavailable|Gateway Timeout", ] +# 5 seconds is generous enough for a USGS upstream replica to recover +# from a brief 5xx but short enough that CI doesn't drag. +_RETRY_DELAY_SEC = 5 _MAX_RERUNS = 2 -_RERUN_DELAY_SEC = 5 - - -def pytest_configure(config): - """Register the ``live`` marker so pytest doesn't warn about it.""" - config.addinivalue_line( - "markers", - "live: marks tests that hit the live USGS Water Data API; " - "retried up to twice on transient upstream HTTP errors.", - ) def pytest_collection_modifyitems(config, items): - """Auto-mark live-API tests with ``live`` + ``flaky`` retry config. - - Heuristic: any test that does NOT request the ``requests_mock`` - fixture is treated as live. This catches every test in - ``waterdata_test.py`` and similar modules without needing to - decorate them individually, and it auto-tracks new tests written - in the same style. - """ for item in items: - if "requests_mock" in getattr(item, "fixturenames", ()): + if "requests_mock" in item.fixturenames: continue - item.add_marker(pytest.mark.live) item.add_marker( pytest.mark.flaky( reruns=_MAX_RERUNS, - reruns_delay=_RERUN_DELAY_SEC, + reruns_delay=_RETRY_DELAY_SEC, only_rerun=_TRANSIENT_PATTERNS, ) ) From 89062997337375350a01569ddae024a8d0630981 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:04:31 -0500 Subject: [PATCH 3/8] Drop NEWS.md entry for the CI infra fix This PR exists only to keep existing tests green against transient upstream blips; no user-facing behavior changes. NEWS entries are for library behavior changes, not CI plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) --- NEWS.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 7db6f222..18fb12b5 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,3 @@ -**05/15/2026:** CI test suite is now resilient to transient USGS Water Data API blips. PR #273 (above) intentionally surfaces mid-pagination HTTP errors to callers rather than silently truncating — the right behavior, but it made any live-API test in CI flaky against transient 429/5xx/connection errors. A new `tests/conftest.py` uses `pytest-rerunfailures` to retry live-API tests up to twice on a 5-second backoff, but only when the failure trace matches a narrow transient-upstream pattern. Library bugs raising other exception types still fail on the first attempt. Also bumps `actions/checkout` and `actions/setup-python` to Node-24-compatible versions (`@v6`) ahead of the GitHub Actions Node 20 runtime deprecation. - **05/14/2026:** Fixed two latent bugs in the paginated `waterdata` request loop (`_walk_pages` and `get_stats_data`). Previously, when `requests.Session.request(...)` itself raised mid-pagination (network error, timeout), the except block called `_error_body()` on the *prior page's* response, so the logged "error" described the wrong request and could itself crash on non-JSON bodies. Separately, no status-code check was performed on subsequent paginated responses, so a 5xx body that didn't include `numberReturned` was silently treated as an empty page — pagination quietly stopped and the user got truncated data with no error logged. The loop now status-checks each page like the initial request and reports the actual exception. The "best-effort" behavior (return whatever pages were collected) is preserved. **05/07/2026:** Bumped the declared minimum Python version from **3.8** to **3.9** (`pyproject.toml`'s `requires-python` and the ruff target). This brings the manifest in line with what was already being tested — CI's matrix has long covered only 3.9, 3.13, and 3.14, the `waterdata` test module already skipped itself on Python < 3.10, and several modules already use 3.9-only stdlib (e.g. `zoneinfo`). Users on 3.8 will no longer be able to install the package; please upgrade. From 88dd3f63641debdcae41c3372e6944ea120bcc54 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:12:59 -0500 Subject: [PATCH 4/8] Tighten live-test selection: source-inspection on public getters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "auto-mark every test that doesn't take requests_mock" heuristic was overkill — out of 244 collected tests it marked 200, including every pure unit test of internal helpers (rdb parsing, _to_str, CQL filter splitting, NWIS_Metadata setters, validation logic, etc). Marker over-application is cheap on the success path but conveys the wrong intent and is one more thing to keep in sync. Refined heuristic: a test is live iff (a) it does not take requests_mock as a fixture, AND (b) its function-body source references one of the package's public user-facing getters (the _PUBLIC_GETTERS set — all `get_*` / `what_*` entry points exposed by waterdata, wqp, samples, nadp, streamstats, nldi, nwis, and ngwmn modules). The set is maintained explicitly because there's no programmatic way for the conftest to know "what counts as a user-facing entry point"; adding a new public getter means adding it here. A regex with `\b` word boundaries avoids matching internal helpers that happen to share a prefix (e.g. `_get_args` won't match `get_args`). Dry-run against the current test inventory: - Old heuristic marked: 200 / 244 - New heuristic marked: 89 / 244 - Now-unmarked unit tests: 111 Spot-checked the 111 de-marked tests — all are genuinely offline: rdb_test.py parsers, utils_test.py to_str/BaseMetadata/datetime helpers, waterdata_filters_test.py CQL-filter logic, nldi_test.py validators, nwis_test.py metadata setters, etc. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/conftest.py | 104 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 93 insertions(+), 11 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 0eb07d15..c9ff7c49 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,21 +1,97 @@ """Auto-retry live-API tests on transient upstream errors. After PR #273, paginated ``waterdata`` getters propagate mid-walk HTTP -errors (429 / 5xx / connection drops) instead of silently truncating the -result. That's the correct behavior for users but makes any CI test that -hits the live USGS Water Data API susceptible to flaking on a transient -upstream blip — e.g. the HTTP 502 Bad Gateway that broke CI on PR #273's -merge to main. - -Heuristic: any test that does NOT request the ``requests_mock`` fixture -is treated as live and gets retried on transient-error patterns only. -Library bugs raising other exception types still fail on the first try. +errors (429 / 5xx / connection drops) instead of silently truncating — +the right behavior, but it makes any CI test that calls the live USGS +Water Data API susceptible to flaking on a transient upstream blip +(e.g. the HTTP 502 Bad Gateway that broke CI on PR #273's merge). + +Selection: a test is "live" iff (a) it does not request the +``requests_mock`` fixture, AND (b) its function body references one of +the package's public user-facing getters (the ``_PUBLIC_GETTERS`` set). +This skips pure unit tests of internal helpers (``_get_args``, +``_check_*``, ``_normalize_*``, ``_construct_api_requests``) that +share the test file with live tests. + +Retry: live tests are retried up to twice on a 5-second backoff, +but only when the failure trace matches a narrow transient-upstream +pattern. Library bugs raising other exception types still fail on +the first try. """ +import inspect +import re + import pytest -# Anchored loosely — the failure trace embeds the exception message -# after a long preamble. +# Public, network-doing entry points. The set is intentionally exhaustive +# so the source-inspection heuristic catches every test that exercises +# the library's user-facing API surface. Add new public getters here. +_PUBLIC_GETTERS = frozenset( + { + # waterdata + "get_channel", + "get_combined_metadata", + "get_continuous", + "get_daily", + "get_field_measurements", + "get_field_measurements_metadata", + "get_latest_continuous", + "get_latest_daily", + "get_monitoring_locations", + "get_nearest_continuous", + "get_peaks", + "get_ratings", + "get_reference_table", + "get_time_series_metadata", + "get_stats_por", + "get_stats_date_range", + # wqp / samples + "get_results", + "get_samples", + "get_samples_summary", + "what_activities", + "what_activity_metrics", + "what_detection_limits", + "what_habitat_metrics", + "what_organizations", + "what_project_weights", + "what_projects", + "what_sites", + # nadp + "get_annual_MDN_map", + "get_annual_NTN_map", + "get_zip", + # streamstats / nldi + "get_basin", + "get_features", + "get_features_by_data_source", + "get_flowlines", + "get_sample_watershed", + "get_watershed", + # nwis (legacy) + "get_discharge_measurements", + "get_discharge_peaks", + "get_dv", + "get_gwlevels", + "get_info", + "get_iv", + "get_pmcodes", + "get_qwdata", + "get_record", + "get_stats", + "get_water_use", + # ngwmn + "get_data", + "get_levels", + "get_sites", + } +) + +_PUBLIC_GETTER_RE = re.compile( + r"\b(?:" + "|".join(re.escape(n) for n in _PUBLIC_GETTERS) + r")\b" +) + _TRANSIENT_PATTERNS = [ r"RuntimeError:\s*(?:429|5\d\d):", # _raise_for_non_200 output r"ConnectionError", # requests/urllib3 @@ -32,6 +108,12 @@ def pytest_collection_modifyitems(config, items): for item in items: if "requests_mock" in item.fixturenames: continue + try: + src = inspect.getsource(item.function) + except (OSError, TypeError): + continue + if not _PUBLIC_GETTER_RE.search(src): + continue item.add_marker( pytest.mark.flaky( reruns=_MAX_RERUNS, From 6f41cb34c70c99e8d53e22f2fd224ef1220addc3 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:13:36 -0500 Subject: [PATCH 5/8] Drop ngwmn getters from _PUBLIC_GETTERS tests/ngwmn_test.py is local-only work not yet in the repo. The ngwmn module itself is committed, but no upstream tests reference it through the conftest's selection path, so listing get_data/get_levels/get_sites in _PUBLIC_GETTERS adds maintenance surface for no upstream benefit. Add these back if/when ngwmn tests land. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/conftest.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index c9ff7c49..4b1e65d7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -81,10 +81,6 @@ "get_record", "get_stats", "get_water_use", - # ngwmn - "get_data", - "get_levels", - "get_sites", } ) From 86ac286d12afceba151aa0125b7b62cc86abbcb4 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:19:12 -0500 Subject: [PATCH 6/8] Limit live-test retry scope to waterdata module PR #273 fixed pagination loops in dataretrieval.waterdata.utils specifically; other modules (wqp, samples, nadp, streamstats, nldi, nwis) reach the network through different paths and don't share that regression. Narrow _PUBLIC_GETTERS to waterdata's 16 user-facing getters and add a tests/waterdata file-path scope check, which also prevents name-collisions with legacy nwis getters (get_ratings, get_daily) from spuriously marking nwis tests. After this change, the retry marker is applied to 55 tests across tests/waterdata_test.py, tests/waterdata_nearest_test.py, tests/waterdata_filters_test.py, and tests/waterdata_ratings_test.py, with zero matches outside that directory. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/conftest.py | 71 +++++++++++++++++------------------------------ 1 file changed, 26 insertions(+), 45 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 4b1e65d7..852b39cf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,12 +6,25 @@ Water Data API susceptible to flaking on a transient upstream blip (e.g. the HTTP 502 Bad Gateway that broke CI on PR #273's merge). -Selection: a test is "live" iff (a) it does not request the -``requests_mock`` fixture, AND (b) its function body references one of -the package's public user-facing getters (the ``_PUBLIC_GETTERS`` set). -This skips pure unit tests of internal helpers (``_get_args``, -``_check_*``, ``_normalize_*``, ``_construct_api_requests``) that -share the test file with live tests. +Scope: ``waterdata`` only. PR #273 specifically fixed the pagination +loops in ``dataretrieval.waterdata.utils`` (``_walk_pages`` / +``get_stats_data``); other modules (``wqp``, ``samples``, ``nadp``, +``streamstats``, ``nldi``, ``nwis``) reach the network through +different paths with their own error semantics and don't share PR +#273's regression. + +Selection: a test is "live" iff (a) its file path is under +``tests/waterdata`` (other modules' tests are out of scope), AND (b) +it does not request the ``requests_mock`` fixture, AND (c) its +function body references one of the waterdata public user-facing +getters (the ``_PUBLIC_GETTERS`` set). The file-path scope is +necessary because some getter names (``get_ratings``, ``get_daily``) +collide with legacy ``nwis`` function names; without the scope, a +test of the legacy ``nwis.get_ratings`` would be retried under +waterdata's transient-error patterns. The function-body check skips +pure unit tests of internal helpers (``_get_args``, ``_check_*``, +``_normalize_*``, ``_construct_api_requests``) that share the test +file with live tests. Retry: live tests are retried up to twice on a 5-second backoff, but only when the failure trace matches a narrow transient-upstream @@ -24,12 +37,11 @@ import pytest -# Public, network-doing entry points. The set is intentionally exhaustive -# so the source-inspection heuristic catches every test that exercises -# the library's user-facing API surface. Add new public getters here. +# Public, network-doing entry points of the waterdata module. +# Other modules' getters are intentionally out of scope (see module +# docstring). Add new waterdata getters here. _PUBLIC_GETTERS = frozenset( { - # waterdata "get_channel", "get_combined_metadata", "get_continuous", @@ -46,41 +58,6 @@ "get_time_series_metadata", "get_stats_por", "get_stats_date_range", - # wqp / samples - "get_results", - "get_samples", - "get_samples_summary", - "what_activities", - "what_activity_metrics", - "what_detection_limits", - "what_habitat_metrics", - "what_organizations", - "what_project_weights", - "what_projects", - "what_sites", - # nadp - "get_annual_MDN_map", - "get_annual_NTN_map", - "get_zip", - # streamstats / nldi - "get_basin", - "get_features", - "get_features_by_data_source", - "get_flowlines", - "get_sample_watershed", - "get_watershed", - # nwis (legacy) - "get_discharge_measurements", - "get_discharge_peaks", - "get_dv", - "get_gwlevels", - "get_info", - "get_iv", - "get_pmcodes", - "get_qwdata", - "get_record", - "get_stats", - "get_water_use", } ) @@ -102,6 +79,10 @@ def pytest_collection_modifyitems(config, items): for item in items: + # Scope: waterdata test files only. See module docstring for why + # this file-path filter is needed in addition to the name check. + if "tests/waterdata" not in item.nodeid.replace("\\", "/"): + continue if "requests_mock" in item.fixturenames: continue try: From 047130fadcda5a1b8298066eaf1e82200aacdc7e Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 10:25:07 -0500 Subject: [PATCH 7/8] Replace conftest auto-marking with module-level pytestmark The previous conftest hook auto-applied pytest.mark.flaky to tests by inspecting function source for references to a central _PUBLIC_GETTERS frozenset. This was implicit and required keeping that set in sync with the public API. Switch to explicit opt-in: drop conftest.py and declare the marker at the top of waterdata_test.py via pytestmark. The other waterdata_*_test.py files are entirely mocked or pure unit tests and need no retry. The retry config (reruns count, delay, transient-error trace patterns) is co-located with the tests it protects. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/conftest.py | 100 ---------------------------------------- tests/waterdata_test.py | 18 ++++++++ 2 files changed, 18 insertions(+), 100 deletions(-) delete mode 100644 tests/conftest.py diff --git a/tests/conftest.py b/tests/conftest.py deleted file mode 100644 index 852b39cf..00000000 --- a/tests/conftest.py +++ /dev/null @@ -1,100 +0,0 @@ -"""Auto-retry live-API tests on transient upstream errors. - -After PR #273, paginated ``waterdata`` getters propagate mid-walk HTTP -errors (429 / 5xx / connection drops) instead of silently truncating — -the right behavior, but it makes any CI test that calls the live USGS -Water Data API susceptible to flaking on a transient upstream blip -(e.g. the HTTP 502 Bad Gateway that broke CI on PR #273's merge). - -Scope: ``waterdata`` only. PR #273 specifically fixed the pagination -loops in ``dataretrieval.waterdata.utils`` (``_walk_pages`` / -``get_stats_data``); other modules (``wqp``, ``samples``, ``nadp``, -``streamstats``, ``nldi``, ``nwis``) reach the network through -different paths with their own error semantics and don't share PR -#273's regression. - -Selection: a test is "live" iff (a) its file path is under -``tests/waterdata`` (other modules' tests are out of scope), AND (b) -it does not request the ``requests_mock`` fixture, AND (c) its -function body references one of the waterdata public user-facing -getters (the ``_PUBLIC_GETTERS`` set). The file-path scope is -necessary because some getter names (``get_ratings``, ``get_daily``) -collide with legacy ``nwis`` function names; without the scope, a -test of the legacy ``nwis.get_ratings`` would be retried under -waterdata's transient-error patterns. The function-body check skips -pure unit tests of internal helpers (``_get_args``, ``_check_*``, -``_normalize_*``, ``_construct_api_requests``) that share the test -file with live tests. - -Retry: live tests are retried up to twice on a 5-second backoff, -but only when the failure trace matches a narrow transient-upstream -pattern. Library bugs raising other exception types still fail on -the first try. -""" - -import inspect -import re - -import pytest - -# Public, network-doing entry points of the waterdata module. -# Other modules' getters are intentionally out of scope (see module -# docstring). Add new waterdata getters here. -_PUBLIC_GETTERS = frozenset( - { - "get_channel", - "get_combined_metadata", - "get_continuous", - "get_daily", - "get_field_measurements", - "get_field_measurements_metadata", - "get_latest_continuous", - "get_latest_daily", - "get_monitoring_locations", - "get_nearest_continuous", - "get_peaks", - "get_ratings", - "get_reference_table", - "get_time_series_metadata", - "get_stats_por", - "get_stats_date_range", - } -) - -_PUBLIC_GETTER_RE = re.compile( - r"\b(?:" + "|".join(re.escape(n) for n in _PUBLIC_GETTERS) + r")\b" -) - -_TRANSIENT_PATTERNS = [ - r"RuntimeError:\s*(?:429|5\d\d):", # _raise_for_non_200 output - r"ConnectionError", # requests/urllib3 - r"ReadTimeout|ConnectTimeout|Timeout", -] - -# 5 seconds is generous enough for a USGS upstream replica to recover -# from a brief 5xx but short enough that CI doesn't drag. -_RETRY_DELAY_SEC = 5 -_MAX_RERUNS = 2 - - -def pytest_collection_modifyitems(config, items): - for item in items: - # Scope: waterdata test files only. See module docstring for why - # this file-path filter is needed in addition to the name check. - if "tests/waterdata" not in item.nodeid.replace("\\", "/"): - continue - if "requests_mock" in item.fixturenames: - continue - try: - src = inspect.getsource(item.function) - except (OSError, TypeError): - continue - if not _PUBLIC_GETTER_RE.search(src): - continue - item.add_marker( - pytest.mark.flaky( - reruns=_MAX_RERUNS, - reruns_delay=_RETRY_DELAY_SEC, - only_rerun=_TRANSIENT_PATTERNS, - ) - ) diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index e2ba4da8..0c12ee27 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -33,6 +33,24 @@ _normalize_str_iterable, ) +# 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 — library +# bugs raising other exception types still fail on the first try. +# Tests in this file that use ``requests_mock`` or ``mock.patch`` won't +# match these patterns and so are effectively unaffected. +pytestmark = pytest.mark.flaky( + reruns=2, + reruns_delay=5, + only_rerun=[ + r"RuntimeError:\s*(?:429|5\d\d):", # _raise_for_non_200 output + r"ConnectionError", + r"ReadTimeout|ConnectTimeout|Timeout", + ], +) + def mock_request(requests_mock, request_url, file_path): """Mock request code""" From 5ca434cf74c1ce217a9dcac01c239d2ecd76e344 Mon Sep 17 00:00:00 2001 From: thodson-usgs Date: Fri, 15 May 2026 11:10:13 -0500 Subject: [PATCH 8/8] Clarify pytestmark comment per Copilot review Make explicit that the marker is attached to every test in the module, but the only_rerun patterns are tied to real-network failure traces, so mocked tests are no-ops for the rerun by construction. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/waterdata_test.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/waterdata_test.py b/tests/waterdata_test.py index 0c12ee27..7a1a919a 100644 --- a/tests/waterdata_test.py +++ b/tests/waterdata_test.py @@ -37,10 +37,12 @@ # 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 — library -# bugs raising other exception types still fail on the first try. -# Tests in this file that use ``requests_mock`` or ``mock.patch`` won't -# match these patterns and so are effectively unaffected. +# 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 ``requests_mock`` or ``mock.patch`` are no-ops for the rerun. pytestmark = pytest.mark.flaky( reruns=2, reruns_delay=5,