diff --git a/cardano_node_tests/utils/blockers.py b/cardano_node_tests/utils/blockers.py index fb823d691..efe9b6df2 100644 --- a/cardano_node_tests/utils/blockers.py +++ b/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"" -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() diff --git a/cardano_node_tests/utils/gh_issue.py b/cardano_node_tests/utils/gh_issue.py index bd8f2dc0a..90bae7d40 100644 --- a/cardano_node_tests/utils/gh_issue.py +++ b/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"" diff --git a/framework_tests/test_blockers.py b/framework_tests/test_blockers.py new file mode 100644 index 000000000..e4c5d64b1 --- /dev/null +++ b/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) diff --git a/framework_tests/test_gh_issue.py b/framework_tests/test_gh_issue.py new file mode 100644 index 000000000..2112a5662 --- /dev/null +++ b/framework_tests/test_gh_issue.py @@ -0,0 +1,81 @@ +"""Unit tests for `cardano_node_tests.utils.gh_issue`. + +The tests must not touch the GitHub API. The `GHIssue._get_github` classmethod is +monkeypatched with a fake that returns canned issue states. +""" + +import types +import typing as tp + +import github +import pytest + +from cardano_node_tests.utils import gh_issue + + +class _FakeGithub: + """Fake `github.Github` returning canned issue states and counting API calls.""" + + def __init__(self, responses: tp.Sequence[str | Exception]) -> None: + self.responses = iter(responses) + self.calls = 0 + + def get_repo(self, _repo: str) -> tp.Self: + return self + + def get_issue(self, _number: int) -> types.SimpleNamespace: + self.calls += 1 + response = next(self.responses) + if isinstance(response, Exception): + raise response + return types.SimpleNamespace(state=response) + + +@pytest.fixture +def clean_cache(monkeypatch: pytest.MonkeyPatch) -> None: + """Give each test a fresh issue cache.""" + monkeypatch.setattr(gh_issue.GHIssue, "issue_cache", {}) + + +def _set_github(monkeypatch: pytest.MonkeyPatch, fake: _FakeGithub | None) -> None: + """Make `GHIssue` use the given fake GitHub instance.""" + monkeypatch.setattr(gh_issue.GHIssue, "_get_github", classmethod(lambda _cls: fake)) + + +@pytest.mark.usefixtures("clean_cache") +class TestGetState: + """Tests for `GHIssue.get_state`.""" + + def test_state_cached(self, monkeypatch: pytest.MonkeyPatch): + """Retrieve the state once and serve subsequent calls from the cache.""" + fake = _FakeGithub(responses=["CLOSED"]) + _set_github(monkeypatch, fake) + issue = gh_issue.GHIssue(number=1, repo="r/r") + assert issue.get_state() == gh_issue.STATE_CLOSED + assert issue.get_state() == gh_issue.STATE_CLOSED + assert fake.calls == 1 + + def test_unknown_issue_cached(self, monkeypatch: pytest.MonkeyPatch): + """Cache the state of a nonexistent issue, it cannot appear later.""" + fake = _FakeGithub(responses=[github.UnknownObjectException(status=404)]) + _set_github(monkeypatch, fake) + issue = gh_issue.GHIssue(number=1, repo="r/r") + assert issue.get_state() == gh_issue.STATE_UNKNOWN + assert issue.get_state() == gh_issue.STATE_UNKNOWN + assert fake.calls == 1 + + def test_transient_failure_not_cached(self, monkeypatch: pytest.MonkeyPatch): + """Don't cache a failed state retrieval, the next call may succeed.""" + fake = _FakeGithub(responses=[RuntimeError("API is down"), "OPEN"]) + _set_github(monkeypatch, fake) + issue = gh_issue.GHIssue(number=1, repo="r/r") + assert issue.get_state() == gh_issue.STATE_FAILURE + assert not gh_issue.GHIssue.issue_cache + assert issue.get_state() == "open" + assert fake.calls == 2 + + def test_no_github_instance(self, monkeypatch: pytest.MonkeyPatch): + """Report a state retrieval failure when the GitHub instance is not available.""" + _set_github(monkeypatch, None) + issue = gh_issue.GHIssue(number=1, repo="r/r") + assert issue.get_state() == gh_issue.STATE_FAILURE