Add get_stats_data failure tests and tighten PR #273

Follow-up cleanups on top of the silent-truncation fix:

- New tests for the get_stats_data parallel pagination loop, mirroring
  the _walk_pages ConnectionError + 5xx pair (closes coverage gap from
  the prior commits).
- Lift the hard-coded logger-name string to a module-level _LOGGER_NAME
  constant derived from the utils module's __name__ (self-syncing).
- logger.error → logger.exception in both pagination except clauses so
  the traceback is automatically attached to the log record.
- _raise_for_non_200 docstring: drop the historical bug narration, add
  a one-line note distinguishing it from Response.raise_for_status
  (we route through _error_body for USGS-API-aware messages).
- Drop the inconsistent "resp may be stale" comment; the `as e` and
  variable naming already convey intent, and get_stats_data already
  sidesteps it by logging `url` instead of `resp.url`.
- _run_get_stats_data_with_failure: make monkeypatch a required arg
  (both callers pass it), and swap the lambda for MagicMock so future
  signature drift in _handle_stats_nesting isn't silently masked.
- Tighten test docstrings that narrated the PR/bug.
- Extract _error_log_messages(caplog) helper (used 4 times).
- Lift the conditional links ternary out of the dict literal in _resp_ok.
- NEWS.md: bump date to today.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: thodson-usgs

NEWS.md

@@ -1,4 +1,4 @@
-**05/08/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/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/06/2026:** Each remaining active function in `dataretrieval.nwis` now emits a per-function `DeprecationWarning` naming the `waterdata` replacement to migrate to (visible the first time users call each getter). The `nwis` module is scheduled for removal on or after **2027-05-06**.
 

dataretrieval/waterdata/utils.py

@@ -371,9 +371,10 @@ def _error_body(resp: requests.Response):
 def _raise_for_non_200(resp: requests.Response) -> None:
     """Raise ``RuntimeError(_error_body(resp))`` if ``resp`` is not 200.
 
-    Used by both the initial-request gate and the pagination loops; without
-    the loop-side check, a 5xx body lacking ``numberReturned`` would be
-    silently treated as an empty page and pagination would stop quietly.
+    Routes through ``_error_body`` (USGS-API-aware: handles 429/403
+    specially, extracts ``code``/``description`` from JSON error bodies)
+    rather than ``Response.raise_for_status``, which raises
+    ``HTTPError`` with a generic message.
     """
     if resp.status_code != 200:
         raise RuntimeError(_error_body(resp))
@@ -649,8 +650,7 @@ def _walk_pages(
                 dfs.append(_get_resp_data(resp, geopd=geopd))
                 curr_url = _next_req_url(resp)
             except Exception as e:  # noqa: BLE001
-                # `resp` may be stale (prior page) if client.request() raised.
-                logger.error("Request incomplete: %s", e)
+                logger.exception("Request incomplete: %s", e)
                 logger.warning(
                     "Request failed for URL: %s. Data download interrupted.", curr_url
                 )
@@ -1114,7 +1114,7 @@ def get_stats_data(
                 all_dfs.append(_handle_stats_nesting(body, geopd=GEOPANDAS))
                 next_token = body["next"]
             except Exception as e:  # noqa: BLE001
-                logger.error("Request incomplete: %s", e)
+                logger.exception("Request incomplete: %s", e)
                 logger.warning(
                     "Request failed for URL: %s. Data download interrupted.", url
                 )

tests/waterdata_utils_test.py

@@ -4,6 +4,7 @@
 import pandas as pd
 import requests
 
+import dataretrieval.waterdata.utils as _utils_module
 from dataretrieval.waterdata.utils import (
     _arrange_cols,
     _format_api_dates,
@@ -12,6 +13,8 @@
     _walk_pages,
 )
 
+_LOGGER_NAME = _utils_module.__name__
+
 
 def test_get_args_basic():
     local_vars = {
@@ -89,21 +92,25 @@ def test_walk_pages_multiple_mocked():
 
 def _resp_ok(features):
     """Build a 200-OK mock response carrying the given features list."""
+    links = [{"rel": "next", "href": "https://example.com/page2"}] if features else []
     resp = mock.MagicMock()
     resp.json.return_value = {
         "numberReturned": len(features),
         "features": features,
-        "links": [{"rel": "next", "href": "https://example.com/page2"}]
-        if features
-        else [],
+        "links": links,
     }
     resp.headers = {}
-    resp.links = {"next": {"url": "https://example.com/page2"}} if features else {}
     resp.status_code = 200
     resp.url = "https://example.com/page1"
     return resp
 
 
+def _error_log_messages(caplog):
+    """Pull ERROR-and-above message strings out of caplog. The four
+    pagination-failure tests below all assert against the same shape."""
+    return [r.getMessage() for r in caplog.records if r.levelno >= logging.ERROR]
+
+
 def _walk_pages_with_failure(failure_resp_or_exc):
     """Run _walk_pages where page 1 succeeds and page 2 fails as given."""
     resp1 = _resp_ok([{"id": "1", "properties": {"val": "a"}}])
@@ -125,22 +132,20 @@ def _walk_pages_with_failure(failure_resp_or_exc):
 
 def test_walk_pages_logs_actual_exception_when_request_raises(caplog):
     """Exception from client.request() must be logged with its actual message."""
-    caplog.set_level(logging.ERROR, logger="dataretrieval.waterdata.utils")
+    caplog.set_level(logging.ERROR, logger=_LOGGER_NAME)
 
     df, _ = _walk_pages_with_failure(requests.ConnectionError("boom"))
 
     # First page's data is preserved (best-effort behavior).
     assert list(df["val"]) == ["a"]
     # Logged error mentions the actual ConnectionError, not a stale page body.
-    error_messages = [
-        r.getMessage() for r in caplog.records if r.levelno >= logging.ERROR
-    ]
-    assert any("boom" in m for m in error_messages), error_messages
+    messages = _error_log_messages(caplog)
+    assert any("boom" in m for m in messages), messages
 
 
 def test_walk_pages_surfaces_5xx_mid_pagination(caplog):
     """A non-200 mid-pagination response must be logged, not silently swallowed."""
-    caplog.set_level(logging.ERROR, logger="dataretrieval.waterdata.utils")
+    caplog.set_level(logging.ERROR, logger=_LOGGER_NAME)
 
     page2_500 = mock.MagicMock()
     page2_500.status_code = 503
@@ -153,16 +158,82 @@ def test_walk_pages_surfaces_5xx_mid_pagination(caplog):
     df, _ = _walk_pages_with_failure(page2_500)
 
     assert list(df["val"]) == ["a"]
-    error_messages = [
-        r.getMessage() for r in caplog.records if r.levelno >= logging.ERROR
-    ]
-    # The 5xx is now visible in the error log (it would previously have
-    # been silently swallowed because _get_resp_data returned an empty
-    # frame and the loop stopped quietly).
-    assert any("503" in m or "ServiceUnavailable" in m for m in error_messages), (
-        error_messages
+    messages = _error_log_messages(caplog)
+    assert any("503" in m or "ServiceUnavailable" in m for m in messages), messages
+
+
+def _stats_initial_ok():
+    """A 200-OK initial stats response: empty data list, signals one more page."""
+    resp = mock.MagicMock()
+    resp.status_code = 200
+    resp.json.return_value = {
+        "next": "tok2",
+        "features": [],
+    }
+    resp.headers = {}
+    resp.url = "https://example.com/stats?service=foo"
+    return resp
+
+
+def _run_get_stats_data_with_failure(failure_resp_or_exc, monkeypatch):
+    """Exercise get_stats_data where the initial response succeeds and the
+    paginated follow-up fails as given. Mirrors _walk_pages_with_failure.
+    `monkeypatch` stubs ``_handle_stats_nesting`` so the synthetic minimal
+    response body doesn't need to parse — these tests only assert on the
+    pagination loop's error surfacing."""
+    from dataretrieval.waterdata.utils import get_stats_data
+
+    monkeypatch.setattr(
+        _utils_module,
+        "_handle_stats_nesting",
+        mock.MagicMock(return_value=pd.DataFrame()),
     )
 
+    mock_client = mock.MagicMock(spec=requests.Session)
+    mock_client.send.return_value = _stats_initial_ok()
+    if isinstance(failure_resp_or_exc, BaseException):
+        mock_client.request.side_effect = failure_resp_or_exc
+    else:
+        mock_client.request.return_value = failure_resp_or_exc
+
+    return get_stats_data(
+        args={"monitoring_location_id": "USGS-1"},
+        service="observationNormals",
+        expand_percentiles=False,
+        client=mock_client,
+    )
+
+
+def test_get_stats_data_logs_actual_exception_when_request_raises(caplog, monkeypatch):
+    """get_stats_data variant of the connection-error scenario."""
+    caplog.set_level(logging.ERROR, logger=_LOGGER_NAME)
+
+    _run_get_stats_data_with_failure(
+        requests.ConnectionError("stats-boom"),
+        monkeypatch,
+    )
+
+    messages = _error_log_messages(caplog)
+    assert any("stats-boom" in m for m in messages), messages
+
+
+def test_get_stats_data_surfaces_5xx_mid_pagination(caplog, monkeypatch):
+    """get_stats_data variant of the mid-pagination 5xx scenario."""
+    caplog.set_level(logging.ERROR, logger=_LOGGER_NAME)
+
+    page2_503 = mock.MagicMock()
+    page2_503.status_code = 503
+    page2_503.json.return_value = {
+        "code": "ServiceUnavailable",
+        "description": "upstream timeout",
+    }
+    page2_503.url = "https://example.com/stats?service=foo&next_token=tok2"
+
+    _run_get_stats_data_with_failure(page2_503, monkeypatch)
+
+    messages = _error_log_messages(caplog)
+    assert any("503" in m or "ServiceUnavailable" in m for m in messages), messages
+
 
 def test_handle_stats_nesting_tolerates_missing_drop_columns():
     """If the upstream stats response shape ever changes such that one of