diff --git a/src/base_classes/data_sources.py b/src/base_classes/data_sources.py index ac60f638..80c6177a 100644 --- a/src/base_classes/data_sources.py +++ b/src/base_classes/data_sources.py @@ -114,35 +114,68 @@ class ESPNDataSource(DataSource): return [] def fetch_standings(self, sport: str, league: str) -> Dict: - """Fetch standings from ESPN API.""" - # Try standings endpoint first (for professional leagues like NFL, NBA, etc.) - try: - url = f"{self.base_url}/{sport}/{league}/standings" - response = self.session.get(url, headers=self.get_headers(), timeout=15) - response.raise_for_status() - - data = response.json() - self.logger.debug(f"Fetched standings for {sport}/{league}") + """Fetch standings, or the poll for leagues that have one. + + Order matters and used to be wrong. College leagues publish a poll at + /rankings and a records table at /standings; professional leagues have + only /standings. The old code tried /standings first and fell back to + /rankings only on a 404 -- but college /standings answers 200, so the + fallback never fired and college rankings came back empty forever. + Nothing failed; the AP rank badge simply never appeared, and anything + else keyed off rankings quietly did nothing. + + A 200 that lacks the key is treated as a miss, so a league answering + both endpoints still ends up with whichever one actually carries a poll. + """ + league_name = (league or "").lower() + wants_poll = "college" in league_name or "ncaa" in league_name + endpoints = ["rankings", "standings"] if wants_poll else ["standings", "rankings"] + + 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: + response = self.session.get( + url, headers=self.get_headers(), timeout=15 + ) + response.raise_for_status() + data = response.json() + except (requests.RequestException, ValueError) as e: + status = getattr(getattr(e, "response", None), "status_code", None) + # Only a 404 is routine -- it is how a league says "no poll + # here". Everything else is worth an error, and `status is + # None` covers the ones that matter most: ConnectionError, + # Timeout, a body that would not parse. Silencing those left a + # board that could not reach ESPN with one debug line, and the + # ranked filter running on an empty table. + if status != 404: + self.logger.error( + f"Error fetching {endpoint} from ESPN for " + 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 - except Exception as e: - # If standings doesn't exist, try rankings (for college sports) - if hasattr(e, 'response') and hasattr(e.response, 'status_code') and e.response.status_code == 404: - try: - url = f"{self.base_url}/{sport}/{league}/rankings" - response = self.session.get(url, headers=self.get_headers(), timeout=15) - response.raise_for_status() - - data = response.json() - self.logger.debug(f"Fetched rankings for {sport}/{league}") - return data - except Exception: - # Both endpoints failed - standings/rankings may not be available for this sport/league - self.logger.debug(f"Standings/rankings not available for {sport}/{league} from ESPN API") - return {} - else: - # Non-404 error - log at debug level since standings are optional - self.logger.debug(f"Error fetching standings from ESPN for {sport}/{league}: {e}") - return {} + self.logger.debug( + f"Standings/rankings not available for {sport}/{league} from ESPN API" + ) + return {} class MLBAPIDataSource(DataSource): diff --git a/test/test_data_sources.py b/test/test_data_sources.py index 9aad0588..d7366923 100644 --- a/test/test_data_sources.py +++ b/test/test_data_sources.py @@ -92,10 +92,108 @@ class TestESPNDataSource: assert result == payload 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") 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): assert "espn.com" in self.source.base_url