Skip to content

(janitor/test-prune): consolidate redundant mock-echo tests in admin-github-router - #5634

Open
kilo-code-bot[bot] wants to merge 1 commit into
mainfrom
janitor/test-prune-admin-github-router
Open

(janitor/test-prune): consolidate redundant mock-echo tests in admin-github-router#5634
kilo-code-bot[bot] wants to merge 1 commit into
mainfrom
janitor/test-prune-admin-github-router

Conversation

@kilo-code-bot

@kilo-code-bot kilo-code-bot Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What

Consolidates redundant tests in apps/web/src/routers/admin-github-router.test.ts.

Three router endpoints each had a standalone 'returns X for admin users' test whose only assertion was expect(result).toEqual(mockX) against a value returned by a mock configured in the same test, called with no input. Each of these had a sibling 'passes repos parameter through to the service' test in the same describe block that already exercised the admin-success path with real input and asserted the mock's call arguments.

  • admin.github.getKilocodeOpenPullRequestsSummary
  • admin.github.getKilocodeRecentlyClosedExternalPRs

For each, the response-equality assertion was merged into the existing parameter-forwarding test (now asserting both "service response passes through unmodified" and "input is forwarded to the service"), and the standalone mock-echo test was deleted.

Also removed three assertions in 'returns recently merged external PRs for admin users' (result.length, result[0]?.number) that re-derive facts already implied by the toEqual(mockMergedPrs) assertion on the preceding line — same test, no coverage change.

Why these are safe to remove

  • Production behavior claimed: each router procedure (admin-router.ts:511-556) is a literal pass-through: return service({...args}), with no router-owned branching, filtering, or transformation of the response. There is no router logic whose regression the deleted assertions could have caught beyond "the mock returned what it was told to return."
  • Auth gate already covered: each describe block retains its 'throws FORBIDDEN for non-admin users' test, which is the only meaningful branch (admin vs. non-admin) at this layer.
  • Business logic covered elsewhere, more thoroughly: the actual service logic (default repos selection, draft filtering, bot/team PR classification, week-boundary computation) is exercised directly and extensively in apps/web/src/lib/github/open-pull-request-counts.test.ts, which calls the real (unmocked) service functions rather than a mock.
  • No branch or contract lost: the surviving "passes repos parameter" tests still exercise the exact same successful-call code path (admin gate passes, query resolves, result returned) as the deleted tests, plus they additionally verify argument forwarding, which the deleted tests did not check at all.

Validation

  • git stash + baseline run of pnpm --filter web exec jest routers/admin-github-router.test.ts fails identically before and after this change (10 tests → 8 tests, matching the 2 removed) due to this sandbox having no reachable PostgreSQL instance (ECONNREFUSED in insertTestUser/cleanupDbForTest). This is a pre-existing sandbox limitation, not something introduced by this change — the test file requires DB-backed createCallerForUser/insertTestUser helpers that this environment cannot provision (no docker/postgres available).
  • node_modules/.bin/oxlint apps/web/src/routers/admin-github-router.test.ts — 0 warnings, 0 errors.
  • tsgo --noEmit (apps/web) — no errors.
  • oxfmt — no additional formatting changes needed.
  • Diff inspected: change is scoped to the single test file; no production code or unrelated tests touched.

Three router endpoints (getKilocodeOpenPullRequestsSummary,
getKilocodeRecentlyClosedExternalPRs) each had a standalone
'returns X for admin users' test that only asserted the router
result equals the exact object handed to a same-test mock, with
no input and no branching logic exercised — a pure mock echo.
Each of these had a sibling 'passes repos parameter through to
the service' test in the same describe block that already drove
the admin-success path with real input.

Merge the response-equality assertion into the parameter-forwarding
test so both behaviors (successful pass-through of the service
response, and correct argument forwarding) are verified by one
test per endpoint, and delete the now-redundant standalone test.

Also drop three assertions in the
'returns recently merged external PRs' test that re-derived facts
(array length, first item's number) already implied by the
preceding toEqual on the same mock object.

No behavioral coverage is lost:
- The FORBIDDEN-for-non-admin test in each describe block already
  covers the admin gate.
- The service-level business logic (default repos, draft filtering,
  bot/team classification) is covered independently and more
  thoroughly in apps/web/src/lib/github/open-pull-request-counts.test.ts.
- The router procedures are literal pass-throughs to the mocked
  service (admin-router.ts:511-556); no router-owned transformation
  logic existed to lose coverage of.
@kilo-code-bot kilo-code-bot Bot added the janitor Automated dead-code/duplication cleanup label Aug 28, 2026
@kilo-code-bot

kilo-code-bot Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Test-only consolidation of redundant mock-echo assertions in admin-github-router.test.ts; remaining coverage still exercises admin success, argument forwarding, and the FORBIDDEN gate.

Files Reviewed (1 files)
  • apps/web/src/routers/admin-github-router.test.ts

Reviewed by grok-4.6 · Input: 158.1K · Output: 5.4K · Cached: 59.6K

Review guidance: REVIEW.md from base branch main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

janitor Automated dead-code/duplication cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants