Skip to content

Add GitHub issue tests#67

Open
gaurav wants to merge 112 commits into
mainfrom
add-github-issue-tests
Open

Add GitHub issue tests#67
gaurav wants to merge 112 commits into
mainfrom
add-github-issue-tests

Conversation

@gaurav
Copy link
Copy Markdown
Collaborator

@gaurav gaurav commented Jan 8, 2026

Large feature PR (3001+/158-) introducing a GitHub-issue-driven test framework for babel-validation. Major pieces:

  • Library carve-out: shared code moves from tests/common/ to src/babel_validation/ (assertions, services, sources, core, tools)
  • Assertion framework: 8 AssertionHandler types (Resolves, DoesNotResolve, ResolvesWith, DoesNotResolveWith, HasLabel, ResolvesWithType, SearchByName, Needed) with auto-generated README.md
  • GitHub integration (PyGitHub): parses {{BabelTest|...}} wiki and yaml babel_tests: blocks from issue bodies
  • CSV→YAML CLI (csv-to-babeltests) for bulk authoring
  • CI workflow running pytest -m unit on PRs
  • xdist parallelism with FileLock-coordinated CSV/issue caches

gaurav and others added 10 commits February 15, 2026 02:39
Each assertion type (Resolves, DoesNotResolve, ResolvesWith, ResolvesWithType,
SearchByName, Needed) is now a self-contained class in
src/babel_validation/assertions/, grouped by which service it targets
(nodenorm.py, nameres.py, common.py). A central ASSERTION_HANDLERS registry
in __init__.py maps lowercase assertion names to handler instances, and
NodeNormAssertion / NameResAssertion marker base classes allow isinstance()
checks for applicability. GitHubIssueTest.test_with_nodenorm() and
test_with_nameres() are now 3-line dispatchers. A README.md documents all
supported assertion types with examples in both wiki and YAML syntax.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav and others added 2 commits February 19, 2026 19:03
…pendently.

Bumps pytest to >=9.0 (which includes built-in subtests support). Each
TestResult is now evaluated in its own subtest block, so a failure no longer
short-circuits the rest. Adds post-loop state-consistency subtests: a closed
issue with failing tests fails with a "consider reopening" message, and an open
issue where all tests pass emits an xfail "consider closing" hint.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously, get_github_issue_id() and GitHubIssueTest.__str__() both resolved
the org/repo name via github_issue.repository.organization.name, which triggers
lazy PyGitHub API calls. Parse org/repo from html_url instead (always present
in the issue JSON, no extra round-trip needed). Also resolves the TODO comment
in get_test_issues_from_issue().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav and others added 2 commits May 8, 2026 01:33
…_init__

- Move dotenv.load_dotenv() inside _get_github_issues_test_cases() so the
  .env file is only loaded when GitHub tests are actually collected, not at
  module import time for every pytest run.
- Add tests/github_issues/__init__.py (was missing).
- Simplify open-issue xfail logic in test_github_issue: guard against zero
  subtests producing a ZeroDivisionError, remove the misleading assert True
  (the xfail marker handles XPASS when all subtests pass), and rename
  count_subtests_xfailed to count_subtests_failed for clarity.
- Update test_yaml_null_babel_tests_raises to expect the new ValueError
  (with message match) instead of the old AttributeError/TypeError.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard prewarm call in NodeNormTest.test_with_nodenorm against an empty
  CURIE set: when all param_sets yield no CURIEs (e.g. bare
  {{BabelTest|Resolves}} with no args), normalize_curies([]) raised
  ValueError before the per-param_set validation could yield a clean
  TestResult failure.
- Raise ValueError (with a clear message) in get_issues_by_ids() when an
  issue ID can't be resolved, instead of silently returning an empty list
  that would cause a confusing IndexError at the call site.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav and others added 3 commits May 8, 2026 01:48
logging.getLogger(str(self)) registered a unique logger per instance
because __str__ includes the issue ID and serialized param sets. The
logging module caches loggers by name, so this grew without bound and
made log filtering by logger name useless. Switch to __name__ (module
logger) and pass instance details in the log message instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gaurav and others added 2 commits May 11, 2026 12:40
- conftest.py: fix lock-file cleanup deleting .csv.lock instead of .lock
- test_nameres_from_gsheet.py: reflow biolink_type dict literal to idiomatic form
- github_issues_test_cases.py: replace useless Github repr log with repo list;
  guard get_issues_by_ids against missing '/' in configured repo name
- test_assertions_docs.py: normalise CRLF before README comparison (Windows safety)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When a 401 GithubException is raised during test collection or fixture
hydration, surface a single failing test with a clear message (including
the URL to regenerate the token and instructions to delete the cache file)
rather than crashing collection and failing all tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@gaurav gaurav requested a review from Copilot May 17, 2026 19:09
- Fix CachedNameRes.bulk_lookup: cache key now includes params (was
  ignoring them, causing stale hits when called with different params);
  use api_params copy instead of mutating the kwargs dict in-place
- Fix CachedNameRes.delete_query: both lookup and bulk_lookup now use
  (query, params_key) tuple keys, so eviction works for both
- Fix NeededHandler: was yielding failures from both test_with_nodenorm
  and test_with_nameres, doubling the failure count per issue
- Extract GitHubIssueTest._get_handler() to eliminate duplicated
  lookup+raise and the diverging error messages
- Unify HasLabelHandler and SearchByNameHandler param-count checks into
  a single len(params) != 2 branch
- Promote GitHubIssuesTestCases regex patterns to class-level constants
- Remove per-instance self.logger from GitHubIssueTest; use module-level
- Fix dead isinstance check in GitHubIssueTest.__init__ (ran after the
  None default was applied, so was always True)
- SearchByNameHandler: request exactly pass_if_found_in_top results
  instead of 2x; remove now-unreachable else branch
- Remove redundant issue_id alias in test_github_issues.py
- Remove "what" comments in handler methods
- Convert f-strings in logging calls to lazy %s format

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 41 changed files in this pull request and generated 4 comments.

Comment thread src/babel_validation/services/nameres.py
Comment on lines +173 to +179
babel_tests = yaml_dict.get('babel_tests') if isinstance(yaml_dict, dict) else None
if babel_tests is None:
raise ValueError(
f"YAML block in issue {github_issue_id} matched the detection pattern "
f"but contains no 'babel_tests' top-level key: {match!r}"
)

Comment thread tests/tools/test_csv_to_babeltests.py Outdated
Comment thread tests/targets.ini
- Replace pytest.xfail() inside subtests.test() context with assert False:
  XFailed raised inside a pytest-subtests context may be caught by the
  subtest handler rather than propagating as xfail. The outer xfail marker
  and the pytest.xfail() at the end of the function already handle the
  open-issue XFAIL outcome correctly.

- Convert null YAML assertion param list from TypeError to ValueError:
  When a YAML block contains 'Assertion: null', iterating None raised an
  opaque TypeError. Now raises ValueError with a clear actionable message.
  Updated the corresponding test to assert ValueError with "null param list".

- Fix eager f-string evaluation in CachedNodeNorm logging calls:
  logger.debug/info with f-strings evaluate the message even when the log
  level suppresses output. Switched to %s/%d format strings to match the
  lazy-evaluation style already used in CachedNameRes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 41 changed files in this pull request and generated 4 comments.

Comment thread src/babel_validation/sources/github/github_issues_test_cases.py Outdated
Comment thread .github/workflows/tests.yaml Outdated
Comment thread tests/tools/test_csv_to_babeltests.py Outdated
Comment thread tests/nameres/test_blocklist.py Outdated
gaurav and others added 5 commits May 17, 2026 23:21
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
A bare YAML scalar (e.g. 'Resolves: CHEBI:15365') is now wrapped as a
single one-element param_set rather than iterating its characters. This
test documents that behaviour and guards against regression.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants