test(github): backfill coverage for issue/PR/label helpers (#511)#539
Conversation
Characterize the logic-bearing github.ts functions that lacked tests: issue fetch/PR-filtering, PR-vs-issue guard, comment clamp/slice, label mutations (incl. removeIssueLabel 404 tolerance), syncStatusLabels ordering, token validation, review-decision derivation, check-failure filtering, and PR-health state mapping. Closes the deferred half of #511.
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
PR Review: test(github): backfill coverage for issue/PR/label helpers (PR 539)
Recommendation: Approve
This is a pure test-coverage backfill PR adding 426 lines of unit tests for src/lib/github.ts functions that previously had no test coverage. All CI checks passed, and the implementation is straightforward, well-structured, and follows repository conventions.
Change-by-Change Findings
src/lib/github.test.ts — Backfill coverage for 9 function groups (+426 lines)
1. fetchIssues tests
- Verifies PRs are filtered out from the issues endpoint (GitHub returns both)
- Confirms
state=openis the default;state=allused whenincludeClosed: true
2. fetchIssue tests
- Happy path returns the issue object
- Guard correctly rejects items with
pull_requestproperty - Non-ok responses throw with formatted error message
3. fetchIssueComments tests
per_pageclamped to 100 (verified via URL inspection)- Response sliced to
maxComments - Non-array payload throws "expected comments array"
- HTTP errors propagate correctly
4. Issue mutation tests
updateIssueLabels→ PUT with full label arrayaddIssueLabel→ POST with single-label arrayupdateIssueTitleAndBody→ PATCH with partial bodycloseIssue→ PATCH withstate: closedremoveIssueLabel→ DELETE with URL-encoded label- 404 tolerance verified:
removeIssueLabelreturns void on 404 (label already absent), throws on other errors
5. syncStatusLabels tests
- Confirms delete-then-add ordering (removes happen before adds)
- No-op when both lists are empty
6. validateGitHubToken tests
- Returns
trueon ok response - Returns
falseon 401 - Returns
falseon network errors
7. fetchPullRequestHealthSignals / deriveReviewDecision tests
- Returns
mergeStateStatusandreviewDecision CHANGES_REQUESTEDtakes precedence overAPPROVED(later timestamp wins)- Latest review per reviewer used (earlier
CHANGES_REQUESTEDignored whenAPPROVEDfollows) COMMENTEDreviews producenullreviewDecision- Merge state degrades to
nullon HTTP errors (reviews still returned)
8. fetchPullRequestCheckFailures tests
- Filters to completed runs with
failureortimed_outconclusions pending(null conclusion) runs excluded- Ref URL-encoded for branch names with slashes
- Returns
[]gracefully on HTTP errors and network failures
9. fetchLinkedPrHealthInput tests
merged_atpresent → statemergedstate: closed+ nomerged_at→ stateclosedstate: open→ stateopen- Assembles all signals (review decision, merge state, check failures) correctly
Standards Compliance
| Standard | Status |
|---|---|
| Test patterns | Uses beforeEach, describe, it, expect with vi.fn() stubs |
| Error handling coverage | Both happy and sad paths tested for all functions |
| No secrets committed | Only test fixtures used; no real credentials |
| TypeScript | Type imports verified; toMatchObject used appropriately |
| Naming conventions | Matches repository test file conventions |
Linked Issue Fit
The PR body references PR 511 (the parent issue for GitHub helper test coverage). From git history:
- PR 511 is the parent tracking issue
- PR 524 was the agent-queue test backfill (already merged)
- This PR closes the deferred half of PR 511 (the
github.tshelper coverage)
The PR description accurately lists all covered behaviors and matches the diff content.
Evidence Provider Findings
No evidence providers were configured for this PR.
Tool Harness Findings
All 4 tool reads of the test file succeeded:
- Imports and mock setup verified
- Existing
fetchPaginated,fetchRepositoryMetadata,searchRepositoryCode,addIssueCommenttests confirmed - New test blocks for all 9 function groups confirmed in their expected positions
- The
routeByUrlmock helper correctly handles parallelPromise.allfetches infetchLinkedPrHealthInput
Unknowns / Needs Verification
None. The diff, tool harness reads, and CI results are complete and consistent.
Final Assessment
This is a well-executed test coverage backfill:
- ✅ All functions listed in the PR description have corresponding tests
- ✅ Both success and error paths covered
- ✅ Edge cases tested (404 tolerance, empty arrays, parallel fetches)
- ✅ CI passes (61 tests, up from 29)
- ✅ No modifications to production code
- ✅ Follows existing test file conventions
No changes requested.
Summary
github.tsfunctions that had none. Closes the deferred half of test: backfill agent-queue-fetch and github.ts coverage #511 (the agent-queue half landed in test(agent-queue-fetch): cover the queue/next-task fetch path #524).Covered behaviors (not thin
fetchPaginatedpassthroughs, which are already tested):fetchIssuesPR filtering +state=open|allfetchIssuePR-vs-issue guard + error pathfetchIssueCommentsper_pageclamp + slice-to-max + non-array guardremoveIssueLabel404 tolerance vs. other errorssyncStatusLabelsremove-then-add ordering (the closed⇒done writer from feat(sync): closed issues always move to status/done #521)validateGitHubTokenok/not-ok/throwsfetchPullRequestHealthSignals→deriveReviewDecision(CHANGES_REQUESTED precedence, latest-per-reviewer, ignores COMMENTED)fetchPullRequestCheckFailuresfailure-conclusion filter + graceful [] on errorfetchLinkedPrHealthInputmerged/closed/open state mappingVerification
npx vitest run src/lib/github.test.ts→ 61 passed (was 29)tsc --noEmitclean;eslintclean