mirror of
https://github.com/stablyai/orca.git
synced 2026-10-07 00:02:29 +00:00
test: retire long-tail cases whose assertion is decided by the test itself (#24132)
Resumes the backlog sweep at a chunk size that actually gets read. Six auditors, 84 files
each, and all six read their full scope case-by-case against production — the first wave
where every chunk closed with no gap. 33 case declarations removed across 22 files, 1 test
file deleted, 826 lines gone. No production code touched.
This wave exists because a conclusion of mine was wrong. I had recorded that yield collapsed
~36x and that deletion was no longer the high-value work. I was dividing cases removed by
files IN SCOPE while the fraction auditors actually READ fell from 100% to about 4%, because
I kept handing them 300-800 files. Recomputed against files read, yield has been flat at 4-7
per 100 with no downward trend. This wave came in at 8.2.
The most instructive removal looked like the most valuable test in scope.
`orchestration-worker-release-reap-fixed.func.test.ts` cites a production bug by two
identifiers, describes orphaned PTYs accumulating until `TasksMax=4096` aborts processes on
EAGAIN, and advertises itself as the functional tier wiring the real orchestration RPC
surface, the real `OrchestrationDb` and the real release modules. Deleting it leaves no
reference to that bug anywhere in `src`.
It still had to go: its fake runtime performed the fence it asserted —
if (pty.incarnationId !== inc) { return null }
handleTable.set('term_reminted', { ptyId, epoch: rendererGraphEpoch })
— so the case checking that a reused ptyId with a mismatched incarnation does not resolve was
checking a decision its own spy made twenty lines earlier. The real fence is owned by
`orca-runtime-terminal-handle-incarnation.test.ts:257`, and the other two cases replay
`orchestration-worker-release-incarnation-fallback.test.ts` (which uses a plain
`mockReturnValue` rather than reimplementing the remint) and `worker/worker-release.test.ts:23`.
"Integration test" and "wires real modules" describe the scaffolding, not the asserted step.
Other removals: a self-comparison disguised by an alias, where
`export const getIssueOwnerRepo = getOwnerRepo` makes a case asserting the two "agree" into
`f(x) === f(x)`; four cases whose `vi.mock` of `resolveIssueSource` made both the preference
value and the topology inert; five verdict-precedence cases owned by a verdict-agnostic block;
three call-shape probes on one-line store pass-throughs whose real contracts are driven by
behavioural neighbours; and a `export type _Ref = [...]` declaration whose own comment admits
it exists only to preserve test-only module-surface references.
Kept after checking production rather than shape. An auditor found two near-identical
ten-reconnect loops and kept both: one uses a test-local live-lease filter, the other the
shipped `sshRemotePtyLeaseAllowsReattach` predicate, and the file's own comment explains the
duality is deliberate "so the two cannot drift". Another kept a paths-alignment case that
looks like a validator tested against its own list, because adding a generated file without
registering its path does fail it — and `shellReadyWrappersExist` uses that registered list to
decide whether a partial tree needs regeneration.
Production duplication is now confirmed four times over, and it is why mirrored tests exist:
`createUpdateWorktreeLineage`/`createAssignWorktreeParent` differ by one `console.error`
string; `terminal-path-tap.ts` and `document/path-tap.ts` carry hand-maintained copies of
`matchFilePathAtColumn` under a docblock reading "keep the two in sync". In those cases both
test sides are load-bearing and the duplication belongs on a refactor list.
`mobile/tests-typecheck-baseline.txt` loses one entry. Trimming
`relay-host-signed-out-verdict.test.ts` made it typecheck clean, so the ratchet required
pruning its grandfathered entry — the file graduates from exempt to enforced. Baseline is now
124 entries, down from 125.
Verified: 690 test files / 7,560 cases pass across the touched desktop areas; the modified
mobile files pass (162 cases); `check-tests-typecheck-ratchet.mjs` OK (898 files in program,
124 grandfathered); `check-reliability-gates.mjs` 140 gates; the deleted file is absent from
the gate manifest, `cloud/package.json` and the mobile baseline; nothing under
`mobile/src/test-support/rpc-recording/` or `mobile/rpc-foundation/goldens/` touched.
This commit is contained in:
Neil
@@ -730,23 +730,6 @@ describe('GitHub issue source split', () => {
|
||||
expect(result.issueSourceFellBack).toBeUndefined()
|
||||
})
|
||||
|
||||
it("preference='auto' + no upstream → queries origin", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'solo', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'auto')
|
||||
|
||||
expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(1, issueSearchArgs('solo/orca'), {
|
||||
cwd: '/repo-root'
|
||||
})
|
||||
})
|
||||
|
||||
it("preference='auto' + upstream exists → PRs query upstream too", async () => {
|
||||
// Why: fork-contribution PRs live on the upstream repo — the fork's own
|
||||
// PR list is almost always empty. 'auto' must resolve PRs upstream-first
|
||||
@@ -799,22 +782,6 @@ describe('GitHub issue source split', () => {
|
||||
)
|
||||
})
|
||||
|
||||
it("preference='upstream' + upstream exists → queries upstream", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'stablyai', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
const result = await listWorkItems('/repo-root', 10, undefined, undefined, 'upstream')
|
||||
|
||||
expect(decodedIssueSearchPath(0)).toContain('q=repo:stablyai/orca is:issue is:open')
|
||||
expect(result.issueSourceFellBack).toBeUndefined()
|
||||
})
|
||||
|
||||
it("preference='upstream' + no upstream → falls back to origin with fellBack=true", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
@@ -831,36 +798,6 @@ describe('GitHub issue source split', () => {
|
||||
expect(result.issueSourceFellBack).toBe(true)
|
||||
})
|
||||
|
||||
it("preference='origin' + upstream exists → queries origin (not upstream)", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'fork', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'origin')
|
||||
|
||||
expect(decodedIssueSearchPath(0)).toContain('q=repo:fork/orca is:issue is:open')
|
||||
})
|
||||
|
||||
it("preference='origin' + no upstream → queries origin", async () => {
|
||||
resolveIssueSourceMock.mockResolvedValueOnce({
|
||||
source: { owner: 'solo', repo: 'orca' },
|
||||
fellBack: false
|
||||
})
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'solo', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }).mockResolvedValueOnce({
|
||||
stdout: '[]'
|
||||
})
|
||||
|
||||
await listWorkItems('/repo-root', 10, undefined, undefined, 'origin')
|
||||
|
||||
expect(decodedIssueSearchPath(0)).toContain('q=repo:solo/orca is:issue is:open')
|
||||
})
|
||||
|
||||
it('surfaces upstreamCandidate in sources regardless of effective preference', async () => {
|
||||
// Why: the renderer selector needs to keep rendering after the user picks
|
||||
// 'origin'. That requires the envelope to carry the raw upstream even
|
||||
|
||||
@@ -144,32 +144,6 @@ describe('getPRForBranch', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('omits maintainerCanModify when the API does not report the flag', async () => {
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
ghExecFileAsyncMock.mockResolvedValueOnce({
|
||||
stdout: JSON.stringify({
|
||||
head: {
|
||||
ref: 'fix-sidebar',
|
||||
repo: {
|
||||
full_name: 'stablyai/orca',
|
||||
name: 'orca',
|
||||
clone_url: 'https://github.com/stablyai/orca.git',
|
||||
ssh_url: 'git@github.com:stablyai/orca.git',
|
||||
owner: { login: 'stablyai' }
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
await expect(getPullRequestPushTarget('/repo-root', 1738)).resolves.toEqual({
|
||||
pushTarget: {
|
||||
remoteName: 'origin',
|
||||
branchName: 'fix-sidebar'
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
it('uses origin for same-repository PR push targets', async () => {
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
getOwnerRepoForRemoteMock.mockResolvedValueOnce({ owner: 'stablyai', repo: 'orca' })
|
||||
|
||||
@@ -149,16 +149,6 @@ describe('github owner/repo resolution', () => {
|
||||
expect(gitRemoteGetUrlCalls('origin')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('prefers upstream for issue owner/repo resolution', async () => {
|
||||
mockGitRemoteCommands({
|
||||
origin: 'git@github.com:fork/orca.git\n',
|
||||
upstream: 'git@github.com:stablyai/orca.git\n'
|
||||
})
|
||||
|
||||
await expect(getIssueOwnerRepo('/repo')).resolves.toEqual({ owner: 'stablyai', repo: 'orca' })
|
||||
expect(gitRemoteGetUrlCalls('upstream')).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('falls back to origin when upstream is present but non-GitHub', async () => {
|
||||
mockGitRemoteCommands({
|
||||
origin: 'git@github.com:fork/orca.git\n',
|
||||
|
||||
@@ -30,7 +30,7 @@ vi.mock('./local-git-config-signature', () => ({
|
||||
|
||||
import { _resetRemoteNameListingCache } from '../git/remote-name-listing'
|
||||
import { getOwnerRepoForRemote, _resetOwnerRepoCache } from './github-repository-identity'
|
||||
import { getOwnerRepo, getIssueOwnerRepo } from './github-owner-repo-selection'
|
||||
import { getOwnerRepo } from './github-owner-repo-selection'
|
||||
import { getRepoUpstream } from './client'
|
||||
|
||||
const FORK_PATH = '/tmp/fork-checkout'
|
||||
@@ -84,13 +84,6 @@ describe('issue #7331: fork PR owner/repo resolution', () => {
|
||||
expect(prRepo).toEqual({ owner: 'stablyai', repo: 'orca' })
|
||||
})
|
||||
|
||||
it('getOwnerRepo and getIssueOwnerRepo agree on a fork checkout', async () => {
|
||||
const prRepo = await getOwnerRepo(FORK_PATH)
|
||||
const issueRepo = await getIssueOwnerRepo(FORK_PATH)
|
||||
|
||||
expect(prRepo).toEqual(issueRepo)
|
||||
})
|
||||
|
||||
it('getOwnerRepo falls back to origin when there is no upstream remote', async () => {
|
||||
const prRepo = await getOwnerRepo(NON_FORK_PATH)
|
||||
|
||||
|
||||
@@ -708,76 +708,6 @@ describe('getWorkItemDetails', () => {
|
||||
expect(getWorkItemMock).toHaveBeenCalledWith('/repo-root', 42, 'pr', null, {}, 'origin')
|
||||
})
|
||||
|
||||
// Why: a rate-limited/auth-failed file fetch must not render as an empty PR;
|
||||
// the Files tab keys its retry state off details.filesUnavailable.
|
||||
it('flags filesUnavailable when the PR file fetch fails but leaves the PR empty otherwise intact', async () => {
|
||||
getWorkItemMock.mockResolvedValueOnce({
|
||||
id: 'pr:8305',
|
||||
type: 'pr',
|
||||
number: 8305,
|
||||
title: 'Files fetch fails',
|
||||
state: 'open',
|
||||
url: 'https://github.com/acme/widgets/pull/8305',
|
||||
labels: [],
|
||||
updatedAt: '2026-07-11T00:00:00Z',
|
||||
author: 'pr-author'
|
||||
})
|
||||
getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' })
|
||||
getPRCommentsMock.mockResolvedValue([])
|
||||
getPRChecksMock.mockResolvedValue([])
|
||||
ghExecFileAsyncMock.mockImplementation(async (args: string[]) => {
|
||||
const target = args.at(-1)
|
||||
if (target === 'repos/acme/widgets/pulls/8305') {
|
||||
return {
|
||||
stdout: JSON.stringify({ head: { sha: 'head-sha' }, base: { sha: 'base-sha' } })
|
||||
}
|
||||
}
|
||||
if (target === 'repos/acme/widgets/pulls/8305/files?per_page=100') {
|
||||
throw new Error('gh: API rate limit exceeded (403)')
|
||||
}
|
||||
return { stdout: JSON.stringify({ data: {} }) }
|
||||
})
|
||||
|
||||
const details = await getWorkItemDetails('/repo-root', 8305, 'pr')
|
||||
|
||||
expect(details?.filesUnavailable).toBe(true)
|
||||
expect(details?.files).toBeUndefined()
|
||||
})
|
||||
|
||||
it('treats an empty file list as a genuinely empty PR, not an unavailable one', async () => {
|
||||
getWorkItemMock.mockResolvedValueOnce({
|
||||
id: 'pr:8306',
|
||||
type: 'pr',
|
||||
number: 8306,
|
||||
title: 'Empty PR',
|
||||
state: 'open',
|
||||
url: 'https://github.com/acme/widgets/pull/8306',
|
||||
labels: [],
|
||||
updatedAt: '2026-07-11T00:00:00Z',
|
||||
author: 'pr-author'
|
||||
})
|
||||
getOwnerRepoForRemoteMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' })
|
||||
getPRCommentsMock.mockResolvedValue([])
|
||||
getPRChecksMock.mockResolvedValue([])
|
||||
ghExecFileAsyncMock.mockImplementation(async (args: string[]) => {
|
||||
const target = args.at(-1)
|
||||
if (target === 'repos/acme/widgets/pulls/8306') {
|
||||
return {
|
||||
stdout: JSON.stringify({ head: { sha: 'head-sha' }, base: { sha: 'base-sha' } })
|
||||
}
|
||||
}
|
||||
if (target === 'repos/acme/widgets/pulls/8306/files?per_page=100') {
|
||||
return { stdout: '[]' }
|
||||
}
|
||||
return { stdout: JSON.stringify({ data: {} }) }
|
||||
})
|
||||
|
||||
const details = await getWorkItemDetails('/repo-root', 8306, 'pr')
|
||||
|
||||
expect(details?.filesUnavailable).toBe(false)
|
||||
expect(details?.files).toEqual([])
|
||||
})
|
||||
|
||||
// Why: `gh pr view` omits avatar_url, so the login-based github.com URL 404s on
|
||||
// GHE. getWorkItemDetails must resolve author/reviewer/assignee avatars via the
|
||||
// GraphQL user(login:) batch and stamp them onto the returned item. See #8784.
|
||||
|
||||
Reference in New Issue
Block a user