Improve GitHub issue blockers: fail loud on bad issue refs, harden state handling

cardano_node_tests/utils/blockers.py

@@ -22,7 +22,9 @@ class GH:
     Attributes:
         issue: A GitHub issue number.
         repo: A repository where the issue belongs to. Default: `IntersectMBO/cardano-node`.
-        fixed_in: A version of the project where the issue is fixed. Ignored on unknown projects.
+        fixed_in: A version of the project where the issue is fixed. On projects other than
+            cardano-node, cardano-cli and cardano-db-sync, it is the cardano-node version
+            into which the fix was integrated.
         message: A message to be added to blocking outcome.
     """
 
@@ -33,6 +35,14 @@ def __init__(
         fixed_in: str = "",
         message: str = "",
     ) -> None:
+        # Validate eagerly so an invalid version is reported already when the issue is defined
+        if fixed_in:
+            try:
+                version.parse(fixed_in)
+            except version.InvalidVersion as excp:
+                msg = f"Invalid `fixed_in` version for issue '{repo}#{issue}': '{fixed_in}'"
+                raise ValueError(msg) from excp
+
         self.issue = issue
         self.repo = repo
         self.fixed_in = fixed_in
@@ -48,26 +58,52 @@ def __init__(
             self.is_blocked = self._issue_is_blocked
 
     def _issue_blocked_in_version(self, product_version: version.Version) -> bool:
-        """Check if an issue is blocked in given product version."""
+        """Check if an issue is blocked in given product version.
+
+        Args:
+            product_version: A version of the product to check the issue against.
+
+        Returns:
+            Whether the issue is considered blocked.
+
+        Raises:
+            ValueError: If the GitHub issue doesn't exist or is not accessible.
+        """
+        issue_id = f"{self.repo}#{self.issue}"
+
         # Assume that the issue is blocked if no GitHub token was provided and so the check
         # cannot be performed.
         if not self.gh_issue.TOKEN:
             LOGGER.warning(
-                "No GitHub token provided, cannot check if issue '%s' is blocked",
-                f"{self.repo}#{self.issue}",
+                "No GitHub token provided, cannot check if issue '%s' is blocked", issue_id
             )
             return True
 
-        # The issue is blocked if it is was not closed yet
-        if not self.gh_issue.is_closed():
+        state = self.gh_issue.get_state()
+
+        # Fail early when the issue cannot be found, e.g. because of a typo in the issue
+        # number or repo name. Otherwise the test would be silently xfailed forever.
+        if state == gh_issue.STATE_UNKNOWN:
+            msg = f"Issue '{issue_id}' doesn't exist or is not accessible"
+            raise ValueError(msg)
+
+        # Assume that the issue is blocked when its state could not be determined,
+        # e.g. due to an API failure or rate limiting.
+        if state == gh_issue.STATE_FAILURE:
+            LOGGER.warning(
+                "Could not determine state of issue '%s', assuming it is blocked", issue_id
+            )
             return True
 
-        # The issue is blocked if it was fixed or integrated into a product version that is greater
-        # than the product version we are currently running.
-        if self.fixed_in and version.parse(self.fixed_in) > product_version:  # noqa:SIM103
+        # The issue is blocked if it was not closed yet
+        if state != gh_issue.STATE_CLOSED:
             return True
 
-        return False
+        # The issue is blocked if it was fixed or integrated into a product version that is greater
+        # than the product version we are currently running.
+        if not self.fixed_in:
+            return False
+        return version.parse(self.fixed_in) > product_version
 
     def _cli_issue_is_blocked(self) -> bool:
         """Check if cardano-cli issue is blocked."""
@@ -82,7 +118,17 @@ def _issue_is_blocked(self) -> bool:
         return self._issue_blocked_in_version(VERSIONS.node)
 
     def finish_test(self, force_blocked: bool = False) -> None:
-        """Fail or Xfail test with GitHub issue reference."""
+        """Fail or Xfail test with GitHub issue reference.
+
+        Never returns - the test outcome is always set via `pytest.xfail` or `pytest.fail`.
+
+        Args:
+            force_blocked: Treat the issue as blocked without checking its state.
+
+        Raises:
+            ValueError: If the GitHub issue doesn't exist or is not accessible.
+                Cannot happen with `force_blocked`, as the issue state is not checked.
+        """
         reason = f"{self.gh_issue}: {self.message}"
         log_message = f"{self.gh_issue.url} => {self.message}"
 
@@ -106,8 +152,21 @@ def __repr__(self) -> str:
         return f"<GH: issue='{self.repo}#{self.issue}', fixed_in='{self.fixed_in}'>"
 
 
-def finish_test(issues: tp.Iterable[GH]) -> None:
-    """Fail or Xfail test with references to multiple GitHub issues."""
+def finish_test(issues: tp.Collection[GH]) -> None:
+    """Fail or Xfail test with references to multiple GitHub issues.
+
+    Never returns - the test outcome is always set via `pytest.xfail` or `pytest.fail`.
+
+    Args:
+        issues: GitHub issues to report. Must not be empty.
+
+    Raises:
+        ValueError: If no issues were provided, or if a referenced GitHub issue doesn't
+            exist or is not accessible.
+    """
+    if not issues:
+        msg = "No issues were provided"
+        raise ValueError(msg)
 
     def _get_outcome(issue: GH) -> tuple[bool, str, str]:
         blocked = issue.is_blocked()

cardano_node_tests/utils/gh_issue.py

@@ -7,6 +7,13 @@
 
 LOGGER = logging.getLogger(__name__)
 
+# State of a closed issue.
+STATE_CLOSED: tp.Final[str] = "closed"
+# State reported when the issue cannot be found, e.g. wrong issue number or repo name.
+STATE_UNKNOWN: tp.Final[str] = "unknown"
+# State reported when the issue state could not be retrieved, e.g. due to an API failure.
+STATE_FAILURE: tp.Final[str] = "get_state_failure"
+
 
 class GHIssue:
     """GitHub issue."""
@@ -51,11 +58,17 @@ def github(self) -> github.Github | None:
     def url(self) -> str:
         return f"https://github.com/{self.repo}/issues/{self.number}"
 
-    def get_state(self) -> str | None:
-        """Get issue state."""
+    def get_state(self) -> str:
+        """Get issue state.
+
+        Returns:
+            The state as reported by GitHub, e.g. "open" or `STATE_CLOSED`; `STATE_UNKNOWN`
+            when the issue cannot be found, or `STATE_FAILURE` when the state could not
+            be retrieved.
+        """
         if not self.github:
             LOGGER.error("Failed to get GitHub instance")
-            return None
+            return STATE_FAILURE
 
         identifier = f"{self.repo}#{self.number}"
         cached_state = self.issue_cache.get(identifier)
@@ -65,17 +78,14 @@ def get_state(self) -> str | None:
                 cached_state = self.github.get_repo(self.repo).get_issue(self.number).state.lower()
             except github.UnknownObjectException:
                 LOGGER.exception("Unknown issue '%s'", identifier)
-                cached_state = "unknown"
+                cached_state = STATE_UNKNOWN
             except Exception:
                 LOGGER.exception("Failed to get issue '%s'", identifier)
-                cached_state = "get_state_failure"
+                # Don't cache the failure, the retrieval may succeed on the next call
+                return STATE_FAILURE
             self.issue_cache[identifier] = cached_state
 
         return cached_state
 
-    def is_closed(self) -> bool:
-        """Check if issue is closed."""
-        return self.get_state() == "closed"
-
     def __repr__(self) -> str:
         return f"<GHIssue: {self.repo}#{self.number}>"

framework_tests/test_blockers.py

@@ -0,0 +1,172 @@
+"""Unit tests for `cardano_node_tests.utils.blockers`.
+
+The tests must not touch the GitHub API. The issue state is controlled by monkeypatching
+`GHIssue.get_state` and the token by monkeypatching `GHIssue.TOKEN`.
+"""
+
+import pytest
+from packaging import version
+
+from cardano_node_tests.utils import blockers
+from cardano_node_tests.utils import gh_issue
+
+PRODUCT_VERSION = version.parse("2.0.0")
+
+
+@pytest.fixture
+def gh_token(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Set a dummy GitHub token so the blocked check is not skipped."""
+    monkeypatch.setattr(gh_issue.GHIssue, "TOKEN", "dummy_token")
+
+
+def _set_state(monkeypatch: pytest.MonkeyPatch, state: str) -> None:
+    """Make `GHIssue.get_state` return the given state without any API call."""
+    monkeypatch.setattr(gh_issue.GHIssue, "get_state", lambda *_a, **_kw: state)
+
+
+def _forbid_get_state(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Make any `GHIssue.get_state` call fail the test."""
+    monkeypatch.setattr(
+        gh_issue.GHIssue,
+        "get_state",
+        lambda *_a, **_kw: pytest.fail("get_state must not be called"),
+    )
+
+
+class TestFixedIn:
+    """Tests for the `fixed_in` handling."""
+
+    def test_invalid_fixed_in(self):
+        """Report an invalid `fixed_in` version already when the issue is defined."""
+        with pytest.raises(ValueError, match="Invalid `fixed_in` version for issue 'r/r#1'"):
+            blockers.GH(issue=1, repo="r/r", fixed_in="not-a-version")
+
+    @pytest.mark.usefixtures("gh_token")
+    def test_fixed_in_changed_after_copy(self, monkeypatch: pytest.MonkeyPatch):
+        """Respect a `fixed_in` value that was changed after init."""
+        _set_state(monkeypatch, gh_issue.STATE_CLOSED)
+        issue = blockers.GH(issue=1, fixed_in="1.0.0")
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is False
+
+        issue_copy = issue.copy()
+        issue_copy.fixed_in = "3.0.0"
+        assert issue_copy._issue_blocked_in_version(PRODUCT_VERSION) is True
+
+
+class TestDispatch:
+    """Tests for the `is_blocked` dispatch based on repo."""
+
+    @pytest.mark.parametrize(
+        ("repo", "expected"),
+        [
+            ("IntersectMBO/cardano-cli", "_cli_issue_is_blocked"),
+            ("IntersectMBO/cardano-db-sync", "_dbsync_issue_is_blocked"),
+            ("IntersectMBO/cardano-node", "_issue_is_blocked"),
+            ("IntersectMBO/ouroboros-consensus", "_issue_is_blocked"),
+        ],
+    )
+    def test_is_blocked_dispatch(self, repo: str, expected: str):
+        """Select the version check that corresponds to the repo."""
+        issue = blockers.GH(issue=1, repo=repo)
+        assert issue.is_blocked.__name__ == expected
+
+
+@pytest.mark.usefixtures("gh_token")
+class TestIsBlocked:
+    """Tests for the issue blocked check."""
+
+    def test_open_issue_is_blocked(self, monkeypatch: pytest.MonkeyPatch):
+        """Treat an open issue as blocked."""
+        _set_state(monkeypatch, "open")
+        issue = blockers.GH(issue=1)
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is True
+
+    def test_closed_issue_is_not_blocked(self, monkeypatch: pytest.MonkeyPatch):
+        """Treat a closed issue without `fixed_in` as not blocked."""
+        _set_state(monkeypatch, gh_issue.STATE_CLOSED)
+        issue = blockers.GH(issue=1)
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is False
+
+    def test_closed_fixed_in_future_version(self, monkeypatch: pytest.MonkeyPatch):
+        """Treat a closed issue as blocked when the fix is in a newer product version."""
+        _set_state(monkeypatch, gh_issue.STATE_CLOSED)
+        issue = blockers.GH(issue=1, fixed_in="3.0.0")
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is True
+
+    def test_closed_fixed_in_current_version(self, monkeypatch: pytest.MonkeyPatch):
+        """Treat a closed issue as not blocked when the fix is in the current version."""
+        _set_state(monkeypatch, gh_issue.STATE_CLOSED)
+        issue = blockers.GH(issue=1, fixed_in="2.0.0")
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is False
+
+    def test_nonexistent_issue(self, monkeypatch: pytest.MonkeyPatch):
+        """Raise an error when the issue cannot be found, instead of xfailing forever."""
+        _set_state(monkeypatch, gh_issue.STATE_UNKNOWN)
+        issue = blockers.GH(issue=1, repo="r/r")
+        with pytest.raises(ValueError, match="Issue 'r/r#1' doesn't exist"):
+            issue._issue_blocked_in_version(PRODUCT_VERSION)
+
+    def test_undetermined_state_is_blocked(
+        self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
+    ):
+        """Assume blocked and warn when the issue state could not be determined."""
+        _set_state(monkeypatch, gh_issue.STATE_FAILURE)
+        issue = blockers.GH(issue=1)
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is True
+        assert "Could not determine state" in caplog.text
+
+    def test_no_token_is_blocked(
+        self, monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture
+    ):
+        """Assume blocked and warn when no GitHub token is available."""
+        monkeypatch.setattr(gh_issue.GHIssue, "TOKEN", None)
+        _forbid_get_state(monkeypatch)
+        issue = blockers.GH(issue=1)
+        assert issue._issue_blocked_in_version(PRODUCT_VERSION) is True
+        assert "No GitHub token provided" in caplog.text
+
+
+@pytest.mark.usefixtures("gh_token")
+class TestFinishTest:
+    """Tests for `GH.finish_test` and the module-level `finish_test`."""
+
+    def test_blocked_issue_xfails(self, monkeypatch: pytest.MonkeyPatch):
+        """Xfail the test when the issue is blocked."""
+        _set_state(monkeypatch, "open")
+        issue = blockers.GH(issue=1, message="msg1")
+        with pytest.raises(pytest.xfail.Exception, match="msg1"):
+            issue.finish_test()
+
+    def test_unblocked_issue_fails(self, monkeypatch: pytest.MonkeyPatch):
+        """Fail the test when the issue is not blocked."""
+        _set_state(monkeypatch, gh_issue.STATE_CLOSED)
+        issue = blockers.GH(issue=1, message="msg1")
+        with pytest.raises(pytest.fail.Exception, match="msg1"):
+            issue.finish_test()
+
+    def test_force_blocked_skips_state_check(self, monkeypatch: pytest.MonkeyPatch):
+        """Xfail without checking the issue state when `force_blocked` is used."""
+        _forbid_get_state(monkeypatch)
+        issue = blockers.GH(issue=1, message="msg1")
+        with pytest.raises(pytest.xfail.Exception, match="msg1"):
+            issue.finish_test(force_blocked=True)
+
+    def test_no_issues(self):
+        """Reject an empty issues collection."""
+        with pytest.raises(ValueError, match="No issues were provided"):
+            blockers.finish_test(issues=[])
+
+    def test_all_blocked_xfails(self, monkeypatch: pytest.MonkeyPatch):
+        """Xfail the test when all issues are blocked."""
+        _set_state(monkeypatch, "open")
+        issues = [blockers.GH(issue=1, message="msg1"), blockers.GH(issue=2, message="msg2")]
+        with pytest.raises(pytest.xfail.Exception, match=r"msg1.*msg2"):
+            blockers.finish_test(issues=issues)
+
+    def test_some_unblocked_fails(self, monkeypatch: pytest.MonkeyPatch):
+        """Fail the test when at least one issue is not blocked."""
+        states = {1: "open", 2: gh_issue.STATE_CLOSED}
+        monkeypatch.setattr(gh_issue.GHIssue, "get_state", lambda self: states[self.number])
+        issues = [blockers.GH(issue=1, message="msg1"), blockers.GH(issue=2, message="msg2")]
+        with pytest.raises(pytest.fail.Exception, match=r"XFAIL.*msg1.*FAIL.*msg2"):
+            blockers.finish_test(issues=issues)