mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-10 17:16:36 +00:00
fix(standings): stop the endpoint handler from swallowing its own bugs
Addresses both CodeRabbit findings on #600. The handler caught `Exception`, so an AttributeError or TypeError raised while *inspecting* the payload was indistinguishable from an endpoint that failed. The loop would move on and, if the other endpoint had nothing either, return {} -- silently dropping rankings for a league that has them. That is the precise failure this function was written to fix, so the handler was able to reintroduce it. Only the request is guarded now. `requests.RequestException` covers the transport failures and `ValueError` covers a body that will not parse; payload inspection happens after the handler, where a bug surfaces instead of being logged as a missing poll. A non-dict payload is treated as a miss explicitly rather than by tripping over `.get`. Tests: the fallback paths had no coverage -- the old single-endpoint code would have passed the suite unchanged. Added order assertions for both league kinds, a 200-without-a-poll fall-through, 404 and non-404 recovery, a non-object payload, and a guard proving a bug is no longer swallowed. `test_fetch_standings_returns_empty_on_error` faked a transport failure with a bare `Exception`, which only passed because the handler caught everything. It now raises ConnectionError, which is what actually happens. Verified by mutation: restoring standings-first fails 5 tests, restoring the catch-all fails the bug-not-swallowed guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
This commit is contained in:
co-authored by
Claude Opus 5
parent
09b2e3ae42
commit
f56772ce3d
@@ -132,18 +132,19 @@ class ESPNDataSource(DataSource):
|
|||||||
endpoints = ["rankings", "standings"] if wants_poll else ["standings", "rankings"]
|
endpoints = ["rankings", "standings"] if wants_poll else ["standings", "rankings"]
|
||||||
|
|
||||||
for endpoint in endpoints:
|
for endpoint in endpoints:
|
||||||
|
url = f"{self.base_url}/{sport}/{league}/{endpoint}"
|
||||||
|
# Only the request is guarded. Inspecting the payload happens
|
||||||
|
# below, outside the handler, so that a bug in this method cannot
|
||||||
|
# be mistaken for an endpoint that failed -- that mistake would
|
||||||
|
# silently drop rankings for a league that has them, which is the
|
||||||
|
# exact failure this function was written to fix.
|
||||||
try:
|
try:
|
||||||
url = f"{self.base_url}/{sport}/{league}/{endpoint}"
|
|
||||||
response = self.session.get(
|
response = self.session.get(
|
||||||
url, headers=self.get_headers(), timeout=15
|
url, headers=self.get_headers(), timeout=15
|
||||||
)
|
)
|
||||||
response.raise_for_status()
|
response.raise_for_status()
|
||||||
data = response.json()
|
data = response.json()
|
||||||
if endpoint == "rankings" and not data.get("rankings"):
|
except (requests.RequestException, ValueError) as e:
|
||||||
continue
|
|
||||||
self.logger.debug(f"Fetched {endpoint} for {sport}/{league}")
|
|
||||||
return data
|
|
||||||
except Exception as e:
|
|
||||||
status = getattr(getattr(e, "response", None), "status_code", None)
|
status = getattr(getattr(e, "response", None), "status_code", None)
|
||||||
# Only a 404 is routine -- it is how a league says "no poll
|
# Only a 404 is routine -- it is how a league says "no poll
|
||||||
# here". Everything else is worth an error, and `status is
|
# here". Everything else is worth an error, and `status is
|
||||||
@@ -156,6 +157,21 @@ class ESPNDataSource(DataSource):
|
|||||||
f"Error fetching {endpoint} from ESPN for "
|
f"Error fetching {endpoint} from ESPN for "
|
||||||
f"{sport}/{league}: {e}"
|
f"{sport}/{league}: {e}"
|
||||||
)
|
)
|
||||||
|
continue
|
||||||
|
|
||||||
|
if not isinstance(data, dict):
|
||||||
|
# A list or a bare string is not something the callers can
|
||||||
|
# read. Treat it as a miss so the other endpoint still gets a
|
||||||
|
# turn, but say so -- this means ESPN changed shape.
|
||||||
|
self.logger.error(
|
||||||
|
f"Unexpected {endpoint} payload for {sport}/{league}: "
|
||||||
|
f"got {type(data).__name__}, expected an object"
|
||||||
|
)
|
||||||
|
continue
|
||||||
|
if endpoint == "rankings" and not data.get("rankings"):
|
||||||
|
continue
|
||||||
|
self.logger.debug(f"Fetched {endpoint} for {sport}/{league}")
|
||||||
|
return data
|
||||||
self.logger.debug(
|
self.logger.debug(
|
||||||
f"Standings/rankings not available for {sport}/{league} from ESPN API"
|
f"Standings/rankings not available for {sport}/{league} from ESPN API"
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -92,10 +92,108 @@ class TestESPNDataSource:
|
|||||||
assert result == payload
|
assert result == payload
|
||||||
|
|
||||||
def test_fetch_standings_returns_empty_on_error(self):
|
def test_fetch_standings_returns_empty_on_error(self):
|
||||||
with patch.object(self.source.session, "get", side_effect=Exception("error")):
|
# A transport failure is a RequestException, not a bare Exception.
|
||||||
|
# The old stand-in passed only because the handler caught everything,
|
||||||
|
# including bugs in the method under test.
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=requests.ConnectionError("error")):
|
||||||
result = self.source.fetch_standings("football", "nfl")
|
result = self.source.fetch_standings("football", "nfl")
|
||||||
assert result == {}
|
assert result == {}
|
||||||
|
|
||||||
|
# ------------------------------------------------------------------
|
||||||
|
# fetch_standings endpoint selection
|
||||||
|
#
|
||||||
|
# College leagues publish a poll at /rankings and a records table at
|
||||||
|
# /standings; professional leagues have only /standings. Probing them in
|
||||||
|
# the wrong order still returns 200 -- just without a poll in it -- so
|
||||||
|
# nothing failed and the rank badge simply never appeared. Order is the
|
||||||
|
# behaviour here, so these tests assert it directly.
|
||||||
|
# ------------------------------------------------------------------
|
||||||
|
|
||||||
|
@staticmethod
|
||||||
|
def _requested_endpoints(mock_get):
|
||||||
|
"""The endpoint names probed, in the order they were requested."""
|
||||||
|
return [call.args[0].rsplit("/", 1)[-1] for call in mock_get.call_args_list]
|
||||||
|
|
||||||
|
def test_professional_league_asks_standings_first(self):
|
||||||
|
payload = {"standings": []}
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
return_value=_mock_response(payload)) as mock_get:
|
||||||
|
result = self.source.fetch_standings("football", "nfl")
|
||||||
|
assert result == payload
|
||||||
|
assert self._requested_endpoints(mock_get) == ["standings"]
|
||||||
|
|
||||||
|
def test_college_league_asks_rankings_first(self):
|
||||||
|
poll = {"rankings": [{"name": "AP Top 25"}]}
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
return_value=_mock_response(poll)) as mock_get:
|
||||||
|
result = self.source.fetch_standings("football", "college-football")
|
||||||
|
assert result == poll
|
||||||
|
assert self._requested_endpoints(mock_get) == ["rankings"]
|
||||||
|
|
||||||
|
def test_rankings_200_without_a_poll_falls_through_to_standings(self):
|
||||||
|
"""A 200 is not the same as an answer.
|
||||||
|
|
||||||
|
This is the case the old code could not see: the endpoint responded,
|
||||||
|
so nothing raised, but the body carried no poll.
|
||||||
|
"""
|
||||||
|
empty_poll = _mock_response({"rankings": []})
|
||||||
|
table = _mock_response({"standings": [{"entries": []}]})
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=[empty_poll, table]) as mock_get:
|
||||||
|
result = self.source.fetch_standings(
|
||||||
|
"basketball", "mens-college-basketball")
|
||||||
|
assert result == {"standings": [{"entries": []}]}
|
||||||
|
assert self._requested_endpoints(mock_get) == ["rankings", "standings"]
|
||||||
|
|
||||||
|
def test_404_on_the_first_endpoint_falls_through_quietly(self):
|
||||||
|
missing = _mock_response({}, status_code=404)
|
||||||
|
table = _mock_response({"standings": []})
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=[missing, table]) as mock_get:
|
||||||
|
result = self.source.fetch_standings("baseball", "college-baseball")
|
||||||
|
assert result == {"standings": []}
|
||||||
|
assert self._requested_endpoints(mock_get) == ["rankings", "standings"]
|
||||||
|
|
||||||
|
def test_recovers_from_a_non_404_failure_on_the_first_endpoint(self):
|
||||||
|
table = _mock_response({"standings": [{"entries": []}]})
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=[requests.ConnectionError("reset"), table]) as mock_get:
|
||||||
|
result = self.source.fetch_standings("football", "college-football")
|
||||||
|
assert result == {"standings": [{"entries": []}]}
|
||||||
|
assert self._requested_endpoints(mock_get) == ["rankings", "standings"]
|
||||||
|
|
||||||
|
def test_both_endpoints_failing_returns_empty(self):
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=requests.ConnectionError("down")) as mock_get:
|
||||||
|
result = self.source.fetch_standings("football", "nfl")
|
||||||
|
assert result == {}
|
||||||
|
assert self._requested_endpoints(mock_get) == ["standings", "rankings"]
|
||||||
|
|
||||||
|
def test_a_non_object_payload_is_treated_as_a_miss(self):
|
||||||
|
odd = _mock_response(["not", "an", "object"])
|
||||||
|
table = _mock_response({"standings": []})
|
||||||
|
with patch.object(self.source.session, "get",
|
||||||
|
side_effect=[odd, table]) as mock_get:
|
||||||
|
result = self.source.fetch_standings("football", "college-football")
|
||||||
|
assert result == {"standings": []}
|
||||||
|
assert self._requested_endpoints(mock_get) == ["rankings", "standings"]
|
||||||
|
|
||||||
|
def test_a_bug_in_this_method_is_not_swallowed_as_a_failed_endpoint(self):
|
||||||
|
"""The guard for the narrowed handler.
|
||||||
|
|
||||||
|
An error raised while reading the payload used to be caught by the
|
||||||
|
endpoint handler and reported as 'no poll here', which would silently
|
||||||
|
drop rankings for a league that has them. It must surface instead.
|
||||||
|
"""
|
||||||
|
boom = Mock(spec=requests.Response)
|
||||||
|
boom.status_code = 200
|
||||||
|
boom.raise_for_status = Mock()
|
||||||
|
boom.json.side_effect = TypeError("a bug, not a network failure")
|
||||||
|
with patch.object(self.source.session, "get", return_value=boom):
|
||||||
|
with pytest.raises(TypeError):
|
||||||
|
self.source.fetch_standings("football", "nfl")
|
||||||
|
|
||||||
def test_base_url_set_correctly(self):
|
def test_base_url_set_correctly(self):
|
||||||
assert "espn.com" in self.source.base_url
|
assert "espn.com" in self.source.base_url
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user