Skip to content

Commit 94b3d02

Browse files
aRustyDevaRustyDev
authored
Merge pull request #834 from aRustyDev/pr/5-test-migration
refactor: migrate test files to proper packages
2 parents 47940c1 + 4587141 commit 94b3d02

76 files changed

Lines changed: 4042 additions & 2613 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎.claude/plans/agents-cli/test-migration/PLAN.md‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
# Test Migration Plan: Move CLI Tests to Proper Packages
2+
3+
## Status: PLANNED
4+
5+
## Problem
6+
7+
The CLI package contains 63 test files (1557 pass, 10 fail, 3 skip) but only ~23 of
8+
those files actually test CLI-specific behavior. The remaining 40 files test SDK, Core,
9+
or KG functionality and were never moved when those packages were extracted. This creates:
10+
11+
- **False ownership**: Tests live in CLI but test SDK/Core/KG code
12+
- **Import indirection**: Tests import via `@agents/sdk/...` from a sibling package
13+
- **Skewed metrics**: CLI appears heavily tested; SDK/Core/KG appear under-tested
14+
- **Maintenance burden**: Failures in CLI tests may actually be SDK regressions
15+
16+
## Current State (baseline)
17+
18+
| Package | Test files | Pass | Fail | Skip |
19+
|---------|-----------|------|------|------|
20+
| Core | 1 | 10 | 0 | 0 |
21+
| SDK | 23 | 381 | 0 | 1 |
22+
| KG | 3 | 127 | 0 | 0 |
23+
| CLI | 63 | 1557 | 10 | 3 |
24+
| **Total** | **90** | **2075** | **10** | **4** |
25+
26+
## Target State (after migration)
27+
28+
| Package | Test files | Estimated pass |
29+
|---------|-----------|----------------|
30+
| Core | 10 | ~19+ |
31+
| SDK | 51+ | ~1500+ |
32+
| KG | 5 | ~135+ |
33+
| CLI | 23 | ~400 |
34+
| **Total** | **89+** | **2075+** |
35+
36+
Note: One file deleted (`graph-spike.test.ts`), so 90 - 1 = 89, plus any new
37+
test-utils files created in SDK.
38+
39+
## Classification Summary
40+
41+
| Destination | Count | Files |
42+
|------------|-------|-------|
43+
| Move to SDK | 28 (+3 duplicates resolved toward SDK) | catalog-*, skill-*, external-skills-*, manifest, lockfile, schemas, search, output, init-component, registry |
44+
| Move to Core | 9 | git, github, github-token, hash, uuid, symlink, source-parser, types, file-io |
45+
| Move to KG | 2 | degradation, kg-commands |
46+
| Keep in CLI | 23 | agents, prompts, commands/*, component/factory, component/skill-ops-impl, lib/config, mcp, plugin-*, skill-cli-wiring, skill-integration |
47+
| Delete | 1 | graph-spike.test.ts (exploratory spike, no maintained code) |
48+
49+
## Duplicate Resolution Strategy
50+
51+
Five test files exist in BOTH CLI and SDK. These must be resolved before any moves:
52+
53+
| Test | CLI (lines) | SDK (lines) | Resolution |
54+
|------|------------|------------|------------|
55+
| `manager.test.ts` | 535 | 278 | Keep CLI version (more comprehensive), delete SDK `providers/manager.test.ts` |
56+
| `pagination.test.ts` | 74 | 110 | Keep SDK version (better coverage), delete CLI `component/pagination.test.ts` |
57+
| `types.test.ts` | 829 | 108 | Keep CLI version (massive), merge unique SDK tests, delete SDK `context/types.test.ts` |
58+
| `registry.test.ts` | 693 | 72 | Keep CLI version (much larger), delete SDK `context/registry.test.ts` |
59+
| `github.test.ts` | 149 | 111 | Different scope -- CLI tests `@agents/core/git`, SDK tests GitHub provider. Keep both, move CLI version to Core |
60+
61+
## Complication Handling
62+
63+
### 1. `mock.module()` paths (3 files)
64+
65+
Files using `mock.module()`:
66+
67+
- `skill-outdated.test.ts` -- mocks `@agents/core/git`
68+
- `skill-update.test.ts` -- mocks `@agents/sdk/providers/local/skill/outdated` and `@agents/sdk/providers/local/skill/add`
69+
- `external-skills-refactor.test.ts` -- mocks `@agents/core/git`
70+
71+
These already use package-absolute paths (`@agents/core/...`, `@agents/sdk/...`),
72+
so they will work identically after moving to SDK. No path changes needed in the
73+
mock.module calls themselves.
74+
75+
### 2. `createCliAgentResolver` dependency (6 files)
76+
77+
Files importing `createCliAgentResolver` from `../src/lib/agents`:
78+
79+
- `skill-filters.test.ts`
80+
- `skill-info.test.ts`
81+
- `skill-integration.test.ts` (stays in CLI)
82+
- `skill-remove.test.ts`
83+
- `skill-add.test.ts`
84+
- `skill-list.test.ts`
85+
86+
**Strategy**: Create a `packages/sdk/test/_utils/mock-resolver.ts` that provides a
87+
test-only AgentResolver implementation. This breaks the cross-package test dependency.
88+
The mock resolver returns the same default agent paths that `createCliAgentResolver()`
89+
returns but without importing from CLI.
90+
91+
### 3. `content/` fixture paths (7+ files)
92+
93+
Files that read real `content/skills/`, `content/plugins/` fixtures:
94+
95+
- `manifest.test.ts` -- uses `WORKTREE = resolve(import.meta.dir, '../../..')`
96+
- `schemas.test.ts` -- uses `REPO_ROOT = resolve(import.meta.dir, '../../..')`
97+
- `lockfile.test.ts` -- uses `REPO_ROOT = resolve(import.meta.dir, '../../..')`
98+
- `skill-commands.test.ts` -- uses `WORKTREE = resolve(import.meta.dir, '../../..')`
99+
- `skill-discovery.test.ts` -- uses `WORKTREE = resolve(import.meta.dir, '../../..')`
100+
- `kg-commands.test.ts` -- imports `PROJECT_ROOT` from module
101+
102+
**Strategy**: After moving from `packages/cli/test/` to `packages/sdk/test/` (or
103+
`packages/core/test/`), the depth changes from `../../..` (3 up) to `../../../..`
104+
(4 up). Update each file's root resolution.
105+
106+
**Better**: Create a shared `packages/sdk/test/_utils/paths.ts` helper:
107+
108+
```ts
109+
import { resolve } from 'path'
110+
export const PROJECT_ROOT = resolve(import.meta.dir, '../../../..')
111+
export const CONTENT_DIR = resolve(PROJECT_ROOT, 'content')
112+
```
113+
114+
### 4. `WORKTREE`/`PROJECT_ROOT` constants
115+
116+
Several tests compute project root via `resolve(import.meta.dir, '../../..')`.
117+
After moving one package level deeper, the depth changes. Each file must be audited.
118+
119+
## Phase Summary
120+
121+
| Phase | Description | Files | Dependencies |
122+
|-------|------------|-------|-------------|
123+
| 1 | Resolve duplicates | 5 pairs | None |
124+
| 2 | Move Core tests | 9 files | Phase 1 |
125+
| 3 | Move SDK tests | 28 files | Phase 1 |
126+
| 4 | Move KG tests | 2 files | Phase 1 |
127+
| 5 | Cleanup and verify | 1 delete + full suite | Phases 2-4 |
128+
129+
Phases 2, 3, and 4 are independent of each other (can be done in any order after
130+
Phase 1). Phase 5 must be last.
131+
132+
## Global Acceptance Criteria
133+
134+
1. **Zero test regressions**: Total pass count >= 2075 (current total)
135+
2. **No cross-package test imports**: No test in SDK imports from `@agents/cli/*`
136+
3. **All 4 suites pass independently**: `bun test packages/<pkg>` works for each
137+
4. **File counts match target**: CLI has ~23 test files, SDK has ~51+, Core has ~10, KG has ~5
138+
5. **No orphaned imports**: Every test file's imports resolve correctly
139+
6. **Duplicates eliminated**: No test file exists in two packages simultaneously
140+
141+
## Risk Mitigation
142+
143+
- **Run tests after every file move** -- catch import failures immediately
144+
- **Move in small batches** -- 3-5 files per sub-phase, verify green before continuing
145+
- **Keep git history** -- use `git mv` so blame/log survives
146+
- **Fallback**: If a file fails after move, temporarily revert that single file and continue with others

‎.claude/plans/agents-cli/test-migration/phase/1-resolve-duplicates.md‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
# Phase 1: Resolve Duplicate Test Files
2+
3+
## Status: PLANNED
4+
5+
## Goal
6+
7+
Eliminate the 5 test files that exist in both CLI and SDK packages. After this phase,
8+
each test file exists in exactly one package, ready for Phase 2-4 moves.
9+
10+
## Non-Goals
11+
12+
- Moving any files to new packages (that is Phases 2-4)
13+
- Changing any import paths
14+
- Creating new test utilities
15+
16+
## Prerequisites
17+
18+
- Worktree on `plan/test-migration` branch
19+
- `bun install` completed
20+
- Baseline test counts recorded (see PLAN.md)
21+
22+
## Duplicate Pairs
23+
24+
### 1. `manager.test.ts` -- CLI wins
25+
26+
| | Location | Lines | Tests |
27+
|-|----------|-------|-------|
28+
| CLI | `packages/cli/test/component/manager.test.ts` | 535 | More comprehensive |
29+
| SDK | `packages/sdk/test/providers/manager.test.ts` | 278 | Subset of CLI |
30+
31+
**Action**:
32+
33+
1. Diff the two files to identify any unique tests in SDK version
34+
2. If SDK has unique test cases, port them into CLI version
35+
3. Delete `packages/sdk/test/providers/manager.test.ts`
36+
4. Run `bun test packages/sdk/` -- verify pass count drops by SDK-only tests moved
37+
5. Run `bun test packages/cli/` -- verify pass count stable or increased
38+
39+
### 2. `pagination.test.ts` -- SDK wins
40+
41+
| | Location | Lines | Tests |
42+
|-|----------|-------|-------|
43+
| CLI | `packages/cli/test/component/pagination.test.ts` | 74 | Basic |
44+
| SDK | `packages/sdk/test/providers/pagination.test.ts` | 110 | Better coverage |
45+
46+
**Action**:
47+
48+
1. Diff the two files to identify any unique tests in CLI version
49+
2. If CLI has unique test cases, port them into SDK version
50+
3. Delete `packages/cli/test/component/pagination.test.ts`
51+
4. Run `bun test packages/cli/` -- verify pass count drops by CLI-only tests removed
52+
5. Run `bun test packages/sdk/` -- verify pass count stable or increased
53+
54+
### 3. `types.test.ts` -- CLI wins (massive)
55+
56+
| | Location | Lines | Tests |
57+
|-|----------|-------|-------|
58+
| CLI | `packages/cli/test/component/types.test.ts` | 829 | Comprehensive |
59+
| SDK | `packages/sdk/test/context/types.test.ts` | 108 | Small subset |
60+
61+
**Action**:
62+
63+
1. Diff to find unique SDK test cases
64+
2. Merge any unique tests from SDK into CLI version
65+
3. Delete `packages/sdk/test/context/types.test.ts`
66+
4. Verify both suites
67+
68+
### 4. `registry.test.ts` -- CLI wins (much larger)
69+
70+
| | Location | Lines | Tests |
71+
|-|----------|-------|-------|
72+
| CLI | `packages/cli/test/registry.test.ts` | 693 | Comprehensive |
73+
| SDK | `packages/sdk/test/context/registry.test.ts` | 72 | Minimal |
74+
75+
**Action**:
76+
77+
1. Diff to find unique SDK test cases
78+
2. Merge any unique tests from SDK into CLI version
79+
3. Delete `packages/sdk/test/context/registry.test.ts`
80+
4. Verify both suites
81+
82+
### 5. `github.test.ts` -- Keep both (different scope)
83+
84+
| | Location | Lines | Tests |
85+
|-|----------|-------|-------|
86+
| CLI | `packages/cli/test/github.test.ts` | 149 | Tests `@agents/core` git/github utilities |
87+
| SDK | `packages/sdk/test/providers/github.test.ts` | 111 | Tests GitHub provider (API client) |
88+
89+
**Action**: No changes needed. These test different modules:
90+
91+
- CLI's `github.test.ts` will move to Core in Phase 2
92+
- SDK's `github.test.ts` stays in SDK (tests provider layer)
93+
94+
## Import Rewrite Rules
95+
96+
None required in this phase. We are only deleting weaker versions and optionally
97+
merging unique tests into the surviving version.
98+
99+
## mock.module Path Updates
100+
101+
None in this phase.
102+
103+
## Fixture Path Updates
104+
105+
None in this phase.
106+
107+
## Acceptance Criteria
108+
109+
1. No test file name exists in more than one package (except `github.test.ts` which
110+
tests different code)
111+
2. Total pass count across all packages >= 2075 (no regressions)
112+
3. Any unique tests from deleted files have been ported to the surviving version
113+
4. `bun test packages/sdk/` passes with <= 23 test files (was 23, may drop to 20-21)
114+
5. `bun test packages/cli/` passes with <= 63 test files (was 63, may drop to 62)
115+
116+
## Verification Commands
117+
118+
```bash
119+
# After each deletion
120+
bun test packages/sdk/ 2>&1 | tail -5
121+
bun test packages/cli/ 2>&1 | tail -5
122+
123+
# Final check -- no duplicates
124+
comm -12 \
125+
<(ls packages/cli/test/**/*.test.ts | xargs -I{} basename {} | sort) \
126+
<(ls packages/sdk/test/**/*.test.ts | xargs -I{} basename {} | sort)
127+
# Expected: only github.test.ts
128+
```
129+
130+
## Failure Criteria and Fallback
131+
132+
- **If merging unique tests breaks the surviving file**: Revert the merge, keep both
133+
files temporarily, and flag for manual review in Phase 5
134+
- **If a deleted file turns out to be the only test for some code path**: Restore it
135+
from git and reassign to the correct package immediately

0 commit comments

Comments
 (0)