agaltsovav/opencode
1
0
Fork 0
Code Issues Pull Requests Actions 87 Packages Projects Releases Wiki Activity

docs(effect): add cleanup roadmap (#27228)

Browse Source
This commit is contained in:
Kit Langton
2026-05-13 02:45:26 +00:00
committed by GitHub
parent 588b5240d0
commit 13fbc9acfc
7 changed files with 804 additions and 1072 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/opencode/test/EFFECT_TEST_MIGRATION.md
+98 -154
View File
@@ -1,16 +1,21 @@
# Effect Test Migration Plan
# Effect Test Migration
This document describes how to move opencode tests out of Promise-land and into the shared `testEffect` pattern.
Move tests that exercise Effect services out of Promise-land and into the
shared `testEffect` pattern.
This file is guidance, not a live inventory. Before claiming a migration,
search current `dev` for the exact anti-pattern and update any PR notes
with what you actually changed.
## Target Pattern
Every test file that exercises Effect services should have one local runner near the top:
Every Effect service test should have one local runner near the top:
```ts
const it = testEffect(layer)
```
Then each test should use one of the runner methods:
Use the runner method that matches the behavior:
```ts
it.effect("pure service behavior", () =>
@@ -23,7 +28,7 @@ it.effect("pure service behavior", () =>
it.instance("instance-local behavior", () =>
Effect.gen(function* () {
const test = yield* TestInstance
// test.directory is a scoped temp opencode instance
expect(test.directory).toContain("opencode-test-")
}),
)
@@ -35,40 +40,22 @@ it.live("live filesystem or process behavior", () =>
)
```
Use `it.effect` for pure Effect code that should run with `TestClock` and `TestConsole`.
Use `it.instance` when the test needs one scoped opencode instance.
Use `it.live` when the test depends on real time, filesystem mtimes, git, child processes, servers, file watchers, or OS behavior.
## Choosing The Runner
## Anti-Patterns To Remove
- `it.effect(...)` — pure Effect behavior with `TestClock` and
`TestConsole`.
- `it.instance(...)` — service behavior that needs one scoped opencode
instance.
- `it.live(...)` — real time, filesystem mtimes, child processes, git,
locks, servers, watchers, or OS behavior.
Avoid these in tests that already target Effect services:
- `test(..., async () => Effect.runPromise(...))`
- local `run(...)`, `load(...)`, `svc(...)`, or `runtime.runPromise(...)` wrappers that only provide a layer
- `tmpdir()` plus `WithInstance.provide(...)` in Promise test bodies
- custom `ManagedRuntime.make(...)` in test files
- Promise `try/catch` around Effect failures
- `Promise.withResolvers`, `Bun.sleep`, or `setTimeout` for synchronization when `Deferred`, `Fiber`, or `Effect.sleep` can express the same behavior
Promise helpers are acceptable at the boundary for non-Effect APIs, but they should be yielded from an Effect body with `Effect.promise(...)` rather than becoming the test harness.
Most integration-style tests use `it.live(...)` or `it.instance(...)`.
## Layer Rules
Compose tests from open service layers, not closed `defaultLayer` graphs when a dependency needs replacing.
Good:
```ts
const layer = Config.layer.pipe(
Layer.provide(AppFileSystem.defaultLayer),
Layer.provide(Env.defaultLayer),
Layer.provide(AuthTest.empty),
Layer.provide(AccountTest.empty),
Layer.provide(NpmTest.noop),
)
```
Avoid using a fully closed layer and hoping to override an inner dependency later. Once `Agent.defaultLayer` has already provided `Config.defaultLayer`, tests cannot cleanly swap the `Npm.Service` used by that config layer.
Compose tests from open service layers when a dependency needs replacing.
Do not use a closed `defaultLayer` and then try to override an inner
dependency after it has already been provided.
Prefer small reusable fake boundary layers in `test/fake/*`:
@@ -80,146 +67,103 @@ SkillTest.empty
ProviderTest.fake().layer
```
Do not add generic test-layer builders until repeated local compositions prove the need. Shared fake boundary services are the first reusable unit. Pre-composed subtrees such as `AgentTest.withPlugins` should come later, only after the same graph appears in multiple files.
Use `Layer.mock` for partial service stubs. Missing methods should fail
loudly if the test accidentally calls them.
Do not add generic test-layer builders until repeated local compositions
prove the need.
## Fixture Rules
Use Effect-aware fixtures from `test/fixture/fixture.ts`:
- `TestInstance` inside `it.instance(...)` for the current temp instance path
- `tmpdirScoped(...)` inside `Effect.gen` for additional temp directories
- `provideInstance(dir)(effect)` when one test needs to switch instance context
- `provideTmpdirInstance((dir) => effect, options)` when a live test needs custom instance setup or multiple instance scopes
- `disposeAllInstances()` in `afterEach` only for integration tests that intentionally touch shared instance registries
- `TestInstance` inside `it.instance(...)` for the current temp instance.
- `tmpdirScoped(...)` inside `Effect.gen` for extra temp directories.
- `provideInstance(dir)(effect)` when one test needs to switch instance
context.
- `provideTmpdirInstance((dir) => effect, options)` when a live test needs
custom instance setup or multiple instance scopes.
- `disposeAllInstances()` in `afterEach` only for integration tests that
intentionally touch shared instance registries.
Use finalizers only as a temporary bridge for existing global mutations:
Avoid mutable global setup. If a global mutation is unavoidable during a
migration, scope it with acquire/release and treat it as temporary.
```ts
yield *
Effect.acquireUseRelease(
Effect.sync(() => {
const previous = process.env.MY_FLAG
process.env.MY_FLAG = "1"
return previous
}),
() => testBody,
(previous) =>
Effect.sync(() => {
if (previous === undefined) delete process.env.MY_FLAG
else process.env.MY_FLAG = previous
}),
)
```
Long term, tests should not toggle `process.env`, `Global.Path`, or
mutable flags when behavior can be modeled with services. Prefer layers
such as `RuntimeFlags.layer(...)` or focused fake services.
TODO: eliminate this pattern over time. Tests should not toggle process-global flags or env vars when the behavior can be modeled with services. Prefer moving flag/env reads behind injectable services such as `Config.Service`, `Env.Service`, or focused test layers, then provide the desired test value through the layer graph instead of mutating `process.env` or `Global.Path`.
## Anti-Patterns To Remove
- `test(..., async () => Effect.runPromise(...))`
- local `run(...)`, `load(...)`, `svc(...)`, or `runtime.runPromise(...)`
wrappers that only provide a layer
- `tmpdir()` plus legacy instance provision in Promise test bodies
- custom `ManagedRuntime.make(...)` in test files
- Promise `try/catch` around Effect failures
- `Promise.withResolvers`, `Bun.sleep`, or `setTimeout` for synchronization
when events, `Deferred`, fibers, or deterministic state checks fit
- mutable env/global/flag changes after layers are built
Promise helpers are acceptable at non-Effect boundaries, but yield them from
inside an Effect body with `Effect.promise(...)` rather than making them the
test harness.
## Conversion Recipe
1. Identify the real service under test and its open `*.layer`.
2. Build one top-level `layer` with real dependencies where they are relevant and `test/fake/*` layers at slow or external boundaries.
3. Replace local Promise wrappers with Effect helpers:
```ts
const run = Effect.fn("MyTest.run")(function* (input: Input) {
const service = yield* MyService.Service
return yield* service.run(input)
})
```
4. Convert `test(..., async () => { ... })` to `it.effect`, `it.instance`, or `it.live`.
1. Identify the real service under test and whether its open `layer` or
closed `defaultLayer` is appropriate.
2. Build one top-level `layer` with real dependencies where relevant and
fake layers at slow or external boundaries.
3. Replace local Promise wrappers with Effect helpers.
4. Convert `test(..., async () => { ... })` to `it.effect`, `it.instance`,
or `it.live`.
5. Move `await` calls inside `Effect.gen` as `yield*` calls.
6. Replace `await using tmp = await tmpdir(...)` with `yield* tmpdirScoped(...)` when the temp directory is inside an Effect test.
7. Replace `WithInstance.provide({ directory, fn })` with `it.instance(...)`, `provideInstance(directory)(effect)`, or `provideTmpdirInstance(...)`.
8. Replace Promise failure assertions with Effect assertions:
```ts
const exit = yield * run(input).pipe(Effect.exit)
expect(Exit.isFailure(exit)).toBe(true)
```
This is correct but still verbose. Track repeated assertion shapes during migration so we can add small test assertion helpers later instead of copying low-level `Exit` plumbing everywhere.
9. Keep concurrency concurrent by using `Effect.forkScoped`, `Fiber.join`, `Deferred`, or `Effect.all(..., { concurrency: "unbounded" })` instead of serializing formerly parallel Promise work.
10. Run the focused test file and `bun typecheck` from `packages/opencode`.
6. Replace `await using tmp = await tmpdir(...)` with
`yield* tmpdirScoped(...)` when the temp directory lives inside the
Effect test.
7. Replace Promise failure assertions with `Effect.exit`, `Effect.flip`, or
focused assertion helpers.
8. Preserve concurrency with fibers, `Deferred`, and
`Effect.all(..., { concurrency: "unbounded" })`; do not accidentally
serialize formerly parallel behavior.
9. Run the focused test file and `bun typecheck` from `packages/opencode`.
## Good Examples
Use these files as models:
Use current examples as patterns, but re-check them before copying because
test migrations are active:
- `test/tool/write.test.ts`: strong `it.instance` tests, top-level `testEffect(...)`, and Effect-native test helpers.
- `test/effect/instance-state.test.ts`: good `it.live` use for scoped directories, instance switching, reload/disposal, and concurrency.
- `test/bus/bus-effect.test.ts`: good `Deferred`, streams, and scoped fibers.
- `test/tool/truncation.test.ts`: good configured runners and concise live service tests.
- `test/tool/repo_clone.test.ts`: good live git integration while staying inside Effect fixtures.
- `test/server/httpapi-instance.test.ts`: good scoped integration layer setup and live HTTP assertions.
- `test/account/service.test.ts`: good service-level live tests, `Effect.flip`, typed errors, and fake HTTP clients.
- `test/agent/plugin-agent-regression.test.ts`: good example of open real service layers plus reusable fake boundary layers.
- `test/effect/instance-state.test.ts` — scoped directories, instance
switching, disposal, and concurrency.
- `test/bus/bus-effect.test.ts` — `Deferred`, streams, scoped fibers.
- `test/agent/plugin-agent-regression.test.ts` — real service layers plus
fake boundary layers.
- `test/account/service.test.ts` — service-level live tests, typed errors,
fake HTTP clients.
## Current Promise-Land Hotspots
## Migration Queue Policy
Start with files that already exercise Effect services but still manually run Promises:
Do not maintain a long file checklist here. It goes stale quickly.
- `test/config/config.test.ts`: many `Effect.runPromise`, `tmpdir()`, and `WithInstance.provide(...)` patterns despite already having `const it = testEffect(layer)`.
- `test/tool/shell.test.ts`: custom `ManagedRuntime`, Promise test helpers, and instance setup around shell execution.
- `test/tool/edit.test.ts`: manual runtime helpers and Promise concurrency patterns that should become fibers/deferreds.
- `test/session/messages-pagination.test.ts`: local Promise service facade over `Session.defaultLayer`.
- `test/snapshot/snapshot.test.ts`: Promise helper with `provideInstance` around snapshot operations.
- `test/file/index.test.ts`: Promise wrappers for `File.Service` plus repeated temp instance setup.
- `test/provider/provider.test.ts`: `AppRuntime.runPromise` helpers and mutable env/config setup.
- `test/project/vcs.test.ts`: Promise event waiting and `AppRuntime.runPromise` around VCS service calls.
When looking for the next target, search for current anti-patterns:
## Migration Order
```bash
git grep -n "Effect.runPromise\|ManagedRuntime\|Promise.withResolvers\|Bun.sleep\|WithInstance" -- packages/opencode/test
```
1. Convert one small file with straightforward service calls and no race behavior.
2. Convert `config.test.ts` incrementally by cluster, not in one PR.
3. Extract additional `test/fake/*` boundary layers only when a second test needs the same fake.
4. Convert files with concurrency or watchers after the simple files, preserving timing semantics with `Deferred` and fibers.
5. Leave pure non-Effect utility tests alone unless converting the underlying code to Effect.
Then choose one file or one small cluster, keep the PR focused, and mention
the focused verification in the PR body.
## Claimable Checklist
## Rough Edges To Watch
Use this as a migration queue. Each checkbox should be safe for one agent or one PR unless the notes say otherwise. Agents should claim one item, convert only that file or cluster, run the focused test file, run `bun typecheck`, and update this checklist in the PR description or follow-up note.
- [ ] `test/file/index.test.ts`: straightforward service wrapper cleanup. Replace local Promise helpers with Effect helpers and use `it.instance` / `it.live` around existing temp instance cases.
- [ ] `test/session/messages-pagination.test.ts`: convert the local `run(...)` / `svc(...)` facade to `testEffect(Session.defaultLayer...)` and direct service yields. Good early target.
- [ ] `test/snapshot/snapshot.test.ts`: convert snapshot operations to `it.live` with `tmpdirScoped` / `provideInstance`. Keep git/filesystem behavior live.
- [ ] `test/project/vcs.test.ts`: convert `AppRuntime.runPromise` service calls first. Leave event/watcher timing intact until the first Effect version is stable.
- [ ] `test/provider/provider.test.ts` cluster 1: convert provider service tests that only read config/env and do not mutate global state heavily.
- [ ] `test/provider/provider.test.ts` cluster 2: convert tests with env/config mutation after introducing or reusing service-backed test seams.
- [ ] `test/tool/shell.test.ts`: replace custom `ManagedRuntime` with `testEffect`, keep as `it.live`, and preserve process behavior.
- [ ] `test/tool/edit.test.ts` cluster 1: convert straightforward edit/read/write cases and remove manual runtime helpers.
- [ ] `test/tool/edit.test.ts` cluster 2: convert concurrency/race tests using `Deferred`, fibers, and `Effect.all` without serializing behavior.
- [ ] `test/config/config.test.ts` setup pass: replace inline fake layers with shared `test/fake/*` layers where possible and turn Promise helpers into Effect helpers.
- [ ] `test/config/config.test.ts` cluster 1: convert simple config load/merge tests that only need one instance.
- [ ] `test/config/config.test.ts` cluster 2: convert managed/global config tests that mutate `Global.Path` or managed config directories. Prefer service seams; use finalizers only as a bridge.
- [ ] `test/config/config.test.ts` cluster 3: convert plugin/dependency tests after ensuring `NpmTest.noop` or explicit fake NPM layers are used.
- [ ] `test/config/config.test.ts` cluster 4: convert remote/account/provider config tests after isolating auth/account/env dependencies through layers.
- [ ] Audit remaining `Effect.runPromise` in `packages/opencode/test/**/*.ts` and create follow-up checklist entries for any missed files.
- [ ] Audit remaining `WithInstance.provide` in `packages/opencode/test/**/*.ts` and convert cases that can use `it.instance` or `provideInstance` inside Effect.
- [ ] Audit repeated `Exit` / `Cause` assertion shapes and propose `test/lib/effect-assert.ts` helpers if at least three files repeat the same pattern.
Parallelization notes:
- The first four items are mostly independent and good for separate worktrees.
- `provider.test.ts`, `tool/edit.test.ts`, and `config.test.ts` should be split by cluster so agents do not edit the same file concurrently.
- Any new fake boundary layer under `test/fake/*` should be small and independently useful. Do not add a fake just for one assertion unless it removes a real external dependency.
- Do not combine assertion-helper design with file migrations. First collect repeated shapes, then add helpers in a separate pass.
Orchestration rules:
- Prefer supervised foreground agents for implementation. Background agents are acceptable for research-only surveys, but code migrations need a returned diff, focused test output, and local commit before moving on.
- Create one worktree per claim and verify the branch/worktree path before edits. A status check should include `git status --short --branch` from the claimed worktree.
- After an agent reports completion, the coordinator must independently inspect `git status`, run the focused test, run `bun typecheck`, and review the diff before pushing.
- If an agent edits the wrong worktree, move the patch deliberately with `git diff` / `git apply`, then clean the accidental worktree before opening a PR.
- Keep dependency setup boring. Prefer reusing existing installed dependencies via worktrees or symlinks over running a fresh `bun install` in a temporary path unless the native build path is known to work.
- Do not delete worktrees with unpushed commits or uncommitted changes. Once a migration PR branch is pushed and clean, the local worktree can be removed while leaving the branch on the fork.
## Effectified Test Rough Edges
Track patterns that are technically Effect-native but still too noisy. These should become a second cleanup pass after the Promise-land migration is underway.
- Failure assertions against `Exit` / `Cause` are often verbose. Consider helpers such as `expectEffectFailure(effect)`, `expectTaggedError(effect, Tag)`, or custom Bun matchers if the same shapes repeat.
- Some tests still need `Effect.promise(...)` around Node/Bun filesystem helpers. Prefer Effect platform services when the surrounding code already uses them, but do not block migrations on perfect filesystem abstraction.
- Scoped global mutation with `process.env`, `Global.Path`, or flags should disappear behind injectable services over time.
- Layer composition can be noisy when a test needs a real service subtree plus fake boundaries. Keep extracting small `test/fake/*` boundary layers before inventing larger builders.
- Concurrency tests can become harder to read after replacing Promise resolvers with `Deferred` and fibers. Look for repeated patterns that deserve named helpers.
- Failure assertions against `Exit` / `Cause` can get verbose. Add helpers
only after the same shape repeats across multiple files.
- Some tests still need `Effect.promise(...)` around Node/Bun APIs. Prefer
Effect platform services when the surrounding code already uses them, but
do not block useful migrations on perfect abstraction.
- Layer composition can be noisy when a test needs real service subtrees plus
fake boundaries. Extract small `test/fake/*` layers before inventing
larger builders.
- Concurrency tests can get harder to read after replacing Promise
resolvers. Look for repeated patterns that deserve named helpers.