Skip to content

test(desktop): remove host-service-coordinator test that pollutes other tests#3734

Merged
Kitenite merged 1 commit into
mainfrom
fix-merge-pull-request-test
Apr 25, 2026
Merged

test(desktop): remove host-service-coordinator test that pollutes other tests#3734
Kitenite merged 1 commit into
mainfrom
fix-merge-pull-request-test

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 25, 2026

Summary

The test added in #3732 calls mock.module() for node:child_process and shell-env. Bun hoists these globally and there's no per-file isolation — they persist for the entire bun test process. This caused 17 unrelated tests (merge-pull-request, git, teardown, listExternalWorktrees, Shell Environment) to fail or timeout when run in the full suite.

I tried less invasive fixes (spreading actual exports, re-mocking in afterAll, removing just the shell-env mock) — none restored live ESM bindings already captured by other test files. Deleting the test is the only mechanical fix.

The product fix in host-service-coordinator.ts (#3732) is unchanged. Coverage should be restored via a refactor that injects spawn / getProcessEnvWithShellPath as constructor options rather than mocking modules globally.

Result

  • Full local bun test: 1830 pass / 0 fail (was 1739 pass / 7 fail / 6 errors on main)

Test plan

  • CI Test job passes on this branch

Summary by CodeRabbit

  • Tests
    • Removed an existing test suite for the host service coordinator, including its network fetch setup/teardown and multiple scenarios around process adoption and spawning.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2fd51e2a-2e38-4311-a7bf-b98e0f27ffa4

📥 Commits

Reviewing files that changed from the base of the PR and between 41de282 and fd3589f.

📒 Files selected for processing (1)
  • apps/desktop/src/main/lib/host-service-coordinator.test.ts
💤 Files with no reviewable changes (1)
  • apps/desktop/src/main/lib/host-service-coordinator.test.ts

📝 Walkthrough

Walkthrough

A test file apps/desktop/src/main/lib/host-service-coordinator.test.ts was removed; the deletion included mocks and tests that exercised HostServiceCoordinator.start under multiple scenarios and managed global fetch setup/teardown.

Changes

Cohort / File(s) Summary
Removed Test File
apps/desktop/src/main/lib/host-service-coordinator.test.ts
Deleted full test suite that mocked node:child_process, @superset/shared/device-info, local helpers (shell-env, host-service-manifest, host-service-utils), exercised HostServiceCoordinator.start across three scenarios (spawn detached, adopt existing manifest, reject outdated manifest), and managed globalThis.fetch setup/teardown.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I hopped into tests and found a file gone,

Left a quiet burrow at the break of dawn.
Mocks and scenarios packed up and hopped away,
I nibble on crumbs of legacy today. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing a problematic test that causes pollution in other tests.
Description check ✅ Passed The description provides clear context about the test pollution issue, attempts made, and the solution, but lacks some template structure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-merge-pull-request-test

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 and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 25, 2026

Greptile Summary

This PR fixes a process-wide mock pollution bug in host-service-coordinator.test.ts where mock.module() for shell-env only exported getProcessEnvWithShellPath, stripping all other exports (execWithShellEnv, getShellEnvironment, etc.) from subsequent tests in the same Bun process. The fix spreads ...actualShellEnv into the mock factory — identical to the existing pattern used for node:child_process in the same file — so only the one function is overridden.

Confidence Score: 5/5

Safe to merge — minimal, targeted fix with no logic changes and no new risks.

The change is a two-line addition that follows the established pattern already present in this file. It restores broken test isolation without altering any production code. No P0/P1 findings.

No files require special attention.

Important Files Changed

Filename Overview
apps/desktop/src/main/lib/host-service-coordinator.test.ts Imports actualShellEnv and spreads it into the mock.module factory so only getProcessEnvWithShellPath is overridden, preserving all other exports for subsequent tests.

Sequence Diagram

sequenceDiagram
    participant Bun as Bun Test Runner
    participant HSC as host-service-coordinator.test.ts
    participant MPR as merge-pull-request.test.ts
    participant SE as shell-env module

    Note over Bun: Before fix
    HSC->>SE: mock.module({ getProcessEnvWithShellPath })
    Note over SE: Only getProcessEnvWithShellPath exported
    MPR->>SE: import { execWithShellEnv }
    SE-->>MPR: SyntaxError: Export named 'execWithShellEnv' not found

    Note over Bun: After fix
    HSC->>SE: import * as actualShellEnv
    HSC->>SE: mock.module({ ...actualShellEnv, getProcessEnvWithShellPath })
    Note over SE: All real exports preserved, only getProcessEnvWithShellPath overridden
    MPR->>SE: import { execWithShellEnv }
    SE-->>MPR: resolves successfully
Loading

Reviews (1): Last reviewed commit: "fix(desktop): preserve real shell-env ex..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

…er tests

The mock.module() calls in this test for node:child_process and shell-env are
hoisted by Bun and persist process-wide, causing 17 unrelated tests
(merge-pull-request, git, teardown, listExternalWorktrees) to fail or timeout
when run in the same suite. Bun does not provide per-file mock isolation, and
re-mocking back in afterAll fires too late to restore live ESM bindings already
captured by other test files.

Removes the test entirely to unblock CI. Coverage of the host-service
restart-adoption fix should be restored via a refactor that injects spawn /
getProcessEnvWithShellPath as constructor options instead of mocking modules.
@Kitenite Kitenite force-pushed the fix-merge-pull-request-test branch from 41de282 to fd3589f Compare April 25, 2026 20:19
@Kitenite Kitenite changed the title fix(desktop): preserve real shell-env exports in host-service-coordinator test test(desktop): remove host-service-coordinator test that pollutes other tests Apr 25, 2026
@Kitenite Kitenite merged commit 0fe65d2 into main Apr 25, 2026
6 checks passed
@Kitenite Kitenite deleted the fix-merge-pull-request-test branch April 25, 2026 20:32
@github-actions
Copy link
Copy Markdown
Contributor

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ⚠️ Neon database branch

Thank you for your contribution! 🎉

MocA-Love added a commit to MocA-Love/superset that referenced this pull request Apr 26, 2026
PR1〜PR5 (#435 #436 #437 #438 #440) で 13 commits 全件 cherry-pick + 手動 conflict 解消で取り込み済み。
本コミットは git 履歴上 behind=0 とするための ours マージ記録。

取り込み済み 13 commits:
- 1f55c62 Fix host service restart adoption (superset-sh#3732)
- 0fe65d2 test(desktop): remove host-service-coordinator test (superset-sh#3734)
- 3012b5a Add optimistic Electric collection updates (superset-sh#3722)
- c272a51 fix(desktop): drop branch row from v2 sidebar workspace item (superset-sh#3733)
- c2f3fdc feat(desktop): add fade-edge mask utilities (superset-sh#3735)
- 682d07c fix v2 terminal osc links (superset-sh#3736)
- 7c0d22b feat(ports): surface remote host-service ports in the sidebar (superset-sh#3676)
- 6a3be2d [codex] Stabilize v2 terminal resize (superset-sh#3739)
- 8928ac6 [codex] Improve v2 pane header responsiveness (superset-sh#3737)
- 5fe3d22 refactor(desktop): tidy v2 terminal session dropdown (superset-sh#3743)
- 66c23d6 Fix automation timezone scheduling (superset-sh#3738)
- 16e270c [codex] Add terminal session titles (superset-sh#3740)
- 583fa5d fix(desktop): refit v2 terminal after font settle (superset-sh#3742)
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