Improve GitHub issue blockers: fail loud on bad issue refs, harden state handling - #3584
Merged
Conversation
The docstring claimed fixed_in is ignored on unknown projects, but the code compares it against the cardano-node version. Existing issue definitions rely on this behavior, so document it instead.
A typo in an issue number resulted in state "unknown", which was
treated as an open issue, so the test was silently xfailed forever.
Raise ValueError instead so the wrong issue reference surfaces
immediately. Transient failures ("get_state_failure", missing
GitHub instance) still conservatively assume the issue is blocked.
Calling finish_test with an empty iterable silently xfailed the test with an empty reason, masking a bug in the caller. Raise ValueError instead.
An invalid fixed_in version string raised InvalidVersion only when the blocked check ran inside a test, looking like a test failure. Parse it eagerly in __init__ so the error points at the issue definition. Also avoids reparsing on every check.
- Name the state sentinels (STATE_UNKNOWN, STATE_FAILURE) so callers don't have to match bare strings. - Don't cache transient retrieval failures. A single rate-limit burst or network blip no longer marks the issue as failed for the whole pytest run, and the failure is logged on every attempt. - Drop is_closed(), its last caller now works with get_state() directly. - Document get_state() return values.
- Log a warning when the issue state could not be determined (API failure, rate limiting, missing GitHub instance) instead of silently xfailing the test as if the issue was known to be open. - Use named state sentinels from gh_issue instead of bare strings. - Turn _fixed_in_version into a property so the blocked check stays correct when the fixed_in attribute is changed after init (the copy() + mutate pattern used with the message attribute). Keep eager validation in __init__ and include the issue reference in the error message. - Broaden the nonexistent-issue wording: UnknownObjectException is also raised for inaccessible or renamed repos. - Add missing Args/Returns/Raises docstring sections.
Cover eager fixed_in validation, the blocked check for all issue states (open, closed, unknown, undetermined), fixed_in comparison, the no-token path, and both finish_test variants. GitHub API access is mocked via GHIssue.get_state.
An unavailable GitHub instance and a failed state retrieval are handled identically by the only caller, so collapse the None signal into STATE_FAILURE. Also add STATE_CLOSED so no caller needs a bare state string.
- Drop the _fixed_in_version property, parse fixed_in inline at its single use site. Validation stays in __init__ and post-init changes to fixed_in keep being respected. - Keep the __init__ attribute assignments contiguous by validating before assigning. - Build the repeated repo#issue string once per check. - Use gh_issue.STATE_CLOSED instead of a bare string and drop the None handling, get_state now always returns str. - Check for empty issues before any GitHub API call and take tp.Collection instead of tp.Iterable. - Document that both finish_test variants never return. - Adjust tests: shared helpers for state patching, dispatch test for the repo to version mapping, lint fixes.
Cover the caching semantics of get_state: real states and unknown issues are cached, transient retrieval failures are not, and a missing GitHub instance reports STATE_FAILURE.
- Use plain comments instead of the Sphinx #: marker, which is not used anywhere else in the codebase. - Clarify get_state Returns wording. - Use types.SimpleNamespace and an iterator in the fake GitHub helper instead of a dynamic class and list.pop bookkeeping.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the GitHub-issue-based “blocker” mechanism used by the test framework by making failures explicit (bad issue refs, undetermined states, invalid fixed_in) and by adding unit tests that fully mock GitHub interactions.
Changes:
- Introduces explicit issue state sentinels in
gh_issueand updatesget_state()caching semantics (don’t cache transient failures). - Makes
blockers.GHfail loud on nonexistent issues, warn-and-assume-blocked on undetermined state / missing token, and eagerly validatesfixed_in. - Adds unit test coverage for
gh_issueandblockers(including caching behavior, dispatch, andfinish_testpaths).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| framework_tests/test_gh_issue.py | Adds unit tests for GHIssue.get_state caching and failure semantics (GitHub API mocked). |
| framework_tests/test_blockers.py | Adds unit tests for blocker decision logic, eager fixed_in validation, dispatch, and finish_test. |
| cardano_node_tests/utils/gh_issue.py | Adds state constants and refines get_state() to return a string sentinel on failure and avoid caching transient errors. |
| cardano_node_tests/utils/blockers.py | Makes blocker checks stricter (bad refs raise, undetermined state warns) and validates fixed_in at definition time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixtures in framework_tests consistently annotate the return type, only tests omit the None return type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes several silent-failure modes in
blockers.py/gh_issue.pyand adds unit testcoverage for both modules.
Bug fixes
repo name used to be treated as an open issue, so the test was xfailed indefinitely
with no signal. It now raises
ValueErrorpointing at the bad reference.failure, rate limiting, missing GitHub instance), the issue is still conservatively
treated as blocked, but a warning is logged instead of pretending the issue is known
to be open.
single rate-limit burst or network blip marked the issue as failed for the entire
pytest run, silencing all subsequent logging. Retrieval is now retried on the next
call and the failure is logged on every attempt.
fixed_inversion fails at issue definition time (import ofissues.py,i.e. test collection) with the issue reference in the error message, instead of
surfacing as a confusing
InvalidVersioninside the first test that runs the check.finish_test()with an empty issue list raisesValueErrorinstead of xfailingthe test with an empty reason, and the check runs before any GitHub API call.
Documentation
fixed_indocstring: it claimed the value is ignored on unknownprojects, but it is in fact compared against the cardano-node version, which is what
the existing issue definitions rely on (e.g.
consensus_*,ledger_*).Args/Returns/Raisesdocstring sections and fixed stale comments.Refactoring
STATE_CLOSED,STATE_UNKNOWN,STATE_FAILURE) ingh_issuereplace bare strings shared across the two modules.get_state()always returnsstr; theNonecase (no GitHub instance) is foldedinto
STATE_FAILURE, which the only caller handled identically anyway.GHIssue.is_closed().Tests
framework_tests/test_blockers.py: eagerfixed_invalidation, the blockedcheck for all issue states, version comparison, the no-token path, repo-to-version
dispatch, and both
finish_testvariants (GitHub API fully mocked).framework_tests/test_gh_issue.py:get_statecaching semantics, includingthat transient failures are not cached.
Notes for reviewers
force_blocked=Truestill skips the issue existence check on purpose: that pathdeliberately avoids any API dependence.
api_829(
fixed_in="10.5.0.0") andapi_1261(fixed_in="11.0.2") intests/issues.pylook like cardano-api release numbers, but the value is compared against the
cardano-node version. Worth double-checking which product version was meant.