Skip to content

test(ci): add unit-test seam for check-stuck-required-checks.mjs#7490

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
lourincedaging0-commits:chore/check-stuck-required-checks-test-seam-7455
Jul 20, 2026
Merged

test(ci): add unit-test seam for check-stuck-required-checks.mjs#7490
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
lourincedaging0-commits:chore/check-stuck-required-checks-test-seam-7455

Conversation

@lourincedaging0-commits

Copy link
Copy Markdown
Contributor

Closes #7455

What

check-stuck-required-checks.mjs's findStuckChecksForPr/minutesSince detection and the watchdog's dry-run + marker-based idempotency only ran inside the un-guarded live-GitHub driver, so none of it was testable in isolation.

  • Refactors githubApi into an injected dependency (a makeGithubApi(token) factory used only by the live entrypoint), matching this repo's injectable-fetchImpl script-test pattern.
  • Exports minutesSince, findStuckChecksForPr, hasExistingWatchdogComment, MARKER, REQUIRED_CONTEXTS, and a runStuckCheckWatchdog driver.
  • Gates the env-reading + live-call entrypoint behind import.meta.url === pathToFileURL(process.argv[1]).href, so importing for tests makes no network calls. Running the script directly is unchanged.
  • Adds scripts/check-stuck-required-checks.d.mts (the declaration that pairs with the imported .mjs) and test/unit/check-stuck-required-checks-script.test.ts.

Tests (green locally under vitest run test/unit)

  • A required check past threshold is flagged; one under threshold is not.
  • A not-completed check with no started_at (elapsedMinutes === null, e.g. still queued) is excluded.
  • Non-required and already-completed checks are ignored even when old.
  • The MARKER makes the watchdog idempotent (a PR already commented on is skipped — no POST).
  • --dry-run never calls the comment-post endpoint.

Verified: importing with empty env neither throws nor fetches; the typed .mjs import resolves via the .d.mts (no TS7016). scripts/** isn't in coverage.include.

The stuck/threshold detection (findStuckChecksForPr, minutesSince) and the watchdog's
dry-run + marker-based idempotency only ran inside the un-guarded live-GitHub driver, so
none of it was testable in isolation.

Refactor githubApi into an injected dependency (a makeGithubApi factory for the live
entrypoint), export minutesSince/findStuckChecksForPr/hasExistingWatchdogComment plus a
runStuckCheckWatchdog driver, and gate the env-reading/live-call entrypoint behind
import.meta.url === argv[1]. Add scripts/check-stuck-required-checks.d.mts (declaration
pairing the imported .mjs) and test/unit/check-stuck-required-checks-script.test.ts
covering: a check past threshold is flagged, one under threshold is not, a not-completed
check with no started_at is excluded, non-required/completed checks are ignored, the
marker makes the watchdog idempotent, and --dry-run never POSTs. Runtime behavior of the
script when invoked directly is unchanged.

Closes JSONbored#7455
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.41%. Comparing base (4ae3854) to head (86d9358).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7490   +/-   ##
=======================================
  Coverage   91.41%   91.41%           
=======================================
  Files         717      717           
  Lines       73032    73032           
  Branches    21633    21633           
=======================================
  Hits        66762    66762           
  Misses       5227     5227           
  Partials     1043     1043           
Flag Coverage Δ
shard-1 32.41% <ø> (+1.15%) ⬆️
shard-2 35.38% <ø> (-0.16%) ⬇️
shard-3 31.81% <ø> (-3.70%) ⬇️
shard-4 43.65% <ø> (+0.28%) ⬆️
shard-5 37.18% <ø> (-1.59%) ⬇️
shard-6 36.12% <ø> (+3.54%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-20 12:51:09 UTC

3 files · 1 AI reviewer · no blockers · readiness 98/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This refactors check-stuck-required-checks.mjs to inject githubApi as a dependency and gates the live env/network-reading entrypoint behind an import.meta.url guard, exporting minutesSince, findStuckChecksForPr, hasExistingWatchdogComment, MARKER, REQUIRED_CONTEXTS, and a new runStuckCheckWatchdog driver. The accompanying test file exercises the real exported logic (threshold math, non-required/completed exclusion, null started_at exclusion, marker-based idempotency, and --dry-run) via a mock githubApi that mirrors the real endpoints called in the implementation, so these are genuine tests of the production code path, not fabricated scenarios. The .d.mts declaration matches the actual exported signatures, and the entrypoint guard correctly preserves live-script behavior (env checks, live githubApi construction) only when run directly.

Nits — 5 non-blocking
  • scripts/check-stuck-required-checks.mjs:45 — the `204` no-content check and other magic numbers (thresholdMinutes default `20`) could be named constants for clarity, though this predates/carries over from the original code.
  • The JSDoc-style block comments above `makeGithubApi` and `runStuckCheckWatchdog` are a bit verbose for this repo's terser inline-comment style seen elsewhere in the file; consider trimming to the non-obvious rationale only.
  • `pathToFileURL(process.argv[1] ?? "").href` silently falls back to an empty string when argv[1] is undefined (e.g. some REPL/exotic invocation) — worth a one-line comment on why that's an acceptable no-op rather than a throw, though it's not reachable in normal script invocation.
  • Consider adding a test where `githubApi` itself rejects (e.g. a 404 on `/check-runs`) to confirm errors propagate rather than being swallowed, since `postComment`/`findStuckChecksForPr` don't catch anything.
  • The `.d.mts` file duplicates type shapes already implicit in the `.mjs`; a short top-of-file comment noting it must be kept in sync manually would help future edits avoid drift.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #7455
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 100 registered-repo PR(s), 49 merged, 5 issue(s).
Contributor context ✅ Confirmed Gittensor contributor lourincedaging0-commits; Gittensor profile; 100 PR(s), 5 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The PR exports minutesSince, findStuckChecksForPr, hasExistingWatchdogComment, MARKER, REQUIRED_CONTEXTS with an injectable githubApi, gates the live-driving logic behind an import.meta.url entrypoint guard, and adds test/unit/check-stuck-required-checks-script.test.ts covering all the required cases (threshold flagging, queued/null elapsedMinutes exclusion, idempotent marker check, and --dry-run

Review context
  • Author: lourincedaging0-commits
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: HTML
  • Official Gittensor activity: 100 PR(s), 5 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit a3f2baa into JSONbored:main Jul 20, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(ci): check-stuck-required-checks.mjs has no unit-test seam, unlike its sibling drift/check scripts

1 participant