Skip to content

test: remove 25 low-value tests and redundant parity suite - #34443

Merged
OrcaWin merged 1 commit into
mainfrom
test-audit-603-runtime-wsl-parity
Oct 27, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
test-audit-603-runtime-wsl-parity

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3 $\color{#cf222e}{\Huge{\mathbf{−}}}$​665 $\color{#cf222e}{\Huge{\mathbf{−}}}$​662
Prod 2 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​17 $\color{#cf222e}{\Huge{\mathbf{−}}}$​10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​7

Remove 25 low-value tests, including the entire six-case remote-server-parity.test.ts suite. The parity cases repeat stronger focus, placement and freshness tests; its append fixture supplies the same host and expected client order. The remaining deletions are mock result/argument echoes, script-string inventories and shared hook-service replays across GitLab, WSL and CodeBuddy tests.

Keep the real timeout/abort races, credential-owner refusal and precedence, manual-job suppression, binary installation, filesystem repair/failure, Windows hook migration and typed captured-authentication behavior. Four other reviewed suites stay unchanged. Counts use authored registrations; parameter expansions are not counted separately.

Update the two active reliability commands to name the stronger parity suites, update their test-file/assertion references, preserve historical executed commands through currentCommand, and remove the obsolete shard timing key.

Validation: isolated static formatting, lint, AST/retained-body checks, unit discovery/runtime routing and reliability manifest validation passed. Full PR CI must pass before merge; surviving roots and five parity counterparts will be checked in its shard evidence. No production code changes.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The change updates browser-preview and remote-agent-session reliability gates to use focused synchronization tests in place of the parity test. It removes the parity test’s CI timing entry and deletes selected tests from main-process and renderer test suites. No runtime implementation changes are shown.

Priority: Low

Merge Risk: Low · up to fa15b

The change is mergeable with owner awareness, but restoring these focused assertions would protect synchronization order, WSL development setup, GitHub errors, and GitLab routing from future regressions.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description follows the repository template in substance. It has an ELI5-style opening only in part, a What Changed section (removal of 25 tests and the parity suite), a Why-like rationale (duplic… Add the template sections as headings: ELI5 (plain-language summary), What Changed (before and after, mechanism), Why (reason for removal and alternatives considered), Linked Issue (replace 'Fixes #' with an actual issue number, or justify …
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title "test: remove 25 low-value tests and redundant parity suite" matches the main change. The PR removes 25 authored test registrations, including the six-case remote-server-parity.test.ts suite…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "test: remove 25 low-value tests and redundant parity suite" matches the main change. The PR removes 25 authored test registrations, including the six-case remote-server-parity.test.ts suite, and updates reliability manifest references. The title does not mention the reliability manifest or shard timing key, but those are supporting changes.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)

Full details: Description check

Explanation

The description follows the repository template in substance. It has an ELI5-style opening only in part, a What Changed section (removal of 25 tests and the parity suite), a Why-like rationale (duplicate coverage), and a Testing section with validation steps. It lacks a Linked Issue (the template requires an issue for outside contributors and the 'Fixes #' line is empty), has no ELI5 section, no explicit Visual Proof statement (N/A is not written), no AI Disclosure entry, and no completed checklist. The description does not follow the template headings, so the structure is largely different from the required template.

Resolution

Add the template sections as headings: ELI5 (plain-language summary), What Changed (before and after, mechanism), Why (reason for removal and alternatives considered), Linked Issue (replace 'Fixes #' with an actual issue number, or justify why none exists), Visual Proof (write N/A with a short reason, since this is test-only), Testing (state which platforms were tested and tick the testing checkboxes), AI Disclosure (state whether AI was used, or leave blank only if a Stably team member), Review and Agent skill upstream boundary (tick the applicable box), and the Checklist.

Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai 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.

Nitpick comments: 5
config/reliability-gates.jsonc: 1

6013-6013: Trivial · Functional Correctness · Quick win

Restore the three-terminal same-host append assertion in a focused suite.

The active gates still cover browser placement and remote-terminal placement around a local browser, but they do not cover a remote terminal appended after two existing same-host remote terminals. A regression that produces an order other than [host-tab-1, host-tab-2, host-tab-3] can therefore pass both gates. Restore this focused fixture in one selected synchronization suite. This is not an exact duplicate of the retained assertions.

src/main/cli/wsl-managed-cli.test.ts: 1

39-43: Trivial · Functional Correctness · Quick win

Restore the default-run check for the development WSL bridge.

The PR removes the only test in src/main/cli that checks the development bridge contents during a default test run. The remaining development-mode case in wsl-managed-cli.wsl.test.ts runs only on Windows with ORCA_TEST_MANAGED_WSL=1. Regressions in the bridge BOM, the ORCA_APP_EXECUTABLE_NEEDS_APP_ROOT assignment, or the NODE_OPTIONS removal no longer fail default tests.

Suggested fix
 import { afterEach, describe, expect, it } from 'vitest'
+import { installFakeAppEnvironment } from '../../../config/scripts/vitest-host-ports-setup'
 import { getManagedWslCliDir } from './wsl-managed-cli'
   it('provides nothing when user data cannot hold the CLI', () => {
     const host = fixture()
     writeFileSync(host.userDataPath, 'a file, not a directory')
     expect(getManagedWslCliDir(host)).toBeNull()
   })
+
+  it('runs the development CLI directly with the dev launcher env', () => {
+    const host = fixture()
+    const appPath = host.resourcesPath
+    const cliEntryPath = join(appPath, 'out', 'cli', 'index.js')
+    mkdirSync(join(appPath, 'out', 'cli'), { recursive: true })
+    writeFileSync(cliEntryPath, 'fixture')
+    installFakeAppEnvironment({ getPath: () => host.userDataPath, getAppPath: () => appPath })
+    const directory = getManagedWslCliDir({ ...host, isPackaged: false }) ?? ''
+    expect(readFileSync(join(directory, 'orca-dev'), 'utf8')).toContain(process.execPath)
+    const bridge = readFileSync(join(directory, 'orca-wsl-bridge.ps1'), 'utf8')
+    expect(bridge.startsWith('\uFEFF')).toBe(true)
+    expect(bridge).toContain(host.userDataPath)
+    expect(bridge).toContain(cliEntryPath)
+    expect(bridge).toContain('$env:ORCA_APP_EXECUTABLE_NEEDS_APP_ROOT')
+    expect(bridge).toContain('Remove-Item Env:NODE_OPTIONS')
+  })
 })
src/renderer/src/runtime/github-check-details-timeout.test.ts: 1

63-66: Trivial · Functional Correctness · Quick win

Restore the coverage for non-timeout rejections in withGitHubCheckDetailsTimeout.

The deleted test was the only test that checks the throw error path in the catch block. Every remaining test passes if the helper replaces all rejections with the localized timeout error. A regression like that would reach the three production callers and show a timeout message for an authentication failure. Restoring the removed test closes this gap with one assertion.

Suggested fix
     ).rejects.toMatchObject({ message: 'Localized timeout.' })
   })
+
+  it('preserves unrelated operation failures', async () => {
+    await expect(
+      withGitHubCheckDetailsTimeout(() => Promise.reject(new Error('authentication failed')))
+    ).rejects.toThrow('authentication failed')
+  })
 })
src/renderer/src/runtime/gitlab-job-trace-client.test.ts: 1

25-30: Trivial · Functional Correctness · Quick win

Keep a fork-project forwarding test for loadGitLabJobLogDetails.

The client passes args.projectRef to jobTrace, and fork pipelines require that project target. The remaining manual-job and timeout tests do not exercise this argument. Related tests mock the client or start at the RPC layer, so they do not detect the renderer client dropping or replacing projectRef.

Suggested fix
 describe('loadGitLabJobLogDetails', () => {
+  it('targets the MR project for a fork pipeline trace', async () => {
+    jobTrace.mockResolvedValue({ ok: true, trace: 'boom' })
+
+    await loadGitLabJobLogDetails({
+      repoPath: '/repo',
+      repoId: 'repo-1',
+      check: gitLabCheck,
+      projectRef: { host: 'gitlab.example.test', path: 'contributor/orca' }
+    })
+
+    expect(jobTrace).toHaveBeenCalledWith(
+      expect.objectContaining({
+        projectRef: { host: 'gitlab.example.test', path: 'contributor/orca' }
+      })
+    )
+  })
+
   it('does not request a trace for a manual job that cannot have one', async () => {
src/renderer/src/runtime/gitlab-owner-api.test.ts: 1

20-25: Trivial · Maintainability & Code Quality · Quick win

Restore the server-owned reviewer routing assertion.

The retained reviewer tests only check fallback and error propagation. They do not check the RPC target, method, or repository argument. A wrong runtime environment or repository argument can therefore pass these tests. Restore the success-path routing test.

Suggested fix
 describe('gitLabApiFor', () => {
+  it("loads a server-owned repo's reviewers from that server", async () => {
+    const reviewer = {
+      id: 7,
+      username: 'ana',
+      name: 'Ana',
+      avatarUrl: 'https://gitlab.example.com/ana.png'
+    }
+    rpc.call.mockResolvedValue([reviewer])
+    const selector = {
+      repoPath: '/work/app',
+      repoId: 'server-repo',
+      repoOwnerExecutionHostId: 'runtime:owner-server'
+    }
+
+    await expect(gitLabApiFor(selector).listAssignableUsers(selector)).resolves.toEqual([reviewer])
+    expect(rpc.call).toHaveBeenCalledWith(
+      { kind: 'environment', environmentId: 'owner-server' },
+      'gitlab.listAssignableUsers',
+      expect.objectContaining({ repo: 'id:server-repo' }),
+      { timeoutMs: 30_000 }
+    )
+  })
+
   it('offers no reviewers from a server older than the reviewer RPC', async () => {

Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b653bbb8-57fb-4220-a40d-215815753ca7
Commits

Reviewing files that changed from the base of the PR and between c98a2d6 and fa15b85.

Files selected for processing: 9
  • config/reliability-gates.jsonc
  • config/scripts/ci-shard-timings.json
  • src/main/browser/wsl-browser-network-relay-launch.test.ts
  • src/main/cli/wsl-managed-cli.test.ts
  • src/main/codebuddy/hook-service.test.ts
  • src/renderer/src/runtime/github-check-details-timeout.test.ts
  • src/renderer/src/runtime/gitlab-job-trace-client.test.ts
  • src/renderer/src/runtime/gitlab-owner-api.test.ts
  • src/renderer/src/runtime/remote-server-parity.test.ts
Files with no reviewable changes: 6
  • src/main/codebuddy/hook-service.test.ts
  • src/renderer/src/runtime/remote-server-parity.test.ts
  • src/renderer/src/runtime/github-check-details-timeout.test.ts
  • src/renderer/src/runtime/gitlab-job-trace-client.test.ts
  • src/main/cli/wsl-managed-cli.test.ts
  • config/scripts/ci-shard-timings.json

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

@OrcaWin
OrcaWin merged commit beb62fe into main Oct 27, 2026
33 checks passed
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.

1 participant