chore: Write end to end tests for data commands#141
Conversation
There was a problem hiding this comment.
PR Review: chore: Write end to end tests for data commands
Summary
This PR adds comprehensive integration tests for the data output flow, testing the full lifecycle from definition creation through method execution to data verification. The tests are well-structured and provide good coverage of the CLI commands.
No Blocking Issues
The code is well-written and follows the project's conventions.
Code Quality Analysis
TypeScript Compliance:
- No
anytypes used - the code properly uses inline type annotations like{ modelName: string }and{ type: string }for dynamic data - All imports are named exports as required
- Code follows the existing patterns established in other integration tests
Test Structure:
- Tests are properly located in the
integration/directory as per CLAUDE.md guidelines - Uses
@std/assertassertions (assertEquals,assertStringIncludes) correctly - Follows the existing test patterns from
echo_model_test.tsandunified_data_test.ts - Good use of
withTempDirhelper for test isolation
DDD Principles:
- Tests properly use the domain entities (
Definition) and repository interfaces (YamlDefinitionRepository) - Tests interact with the system through the CLI commands (application layer) rather than directly manipulating infrastructure
- The separation of concerns is maintained
Test Coverage:
- Full E2E flow: create definition → run method → verify output and data
- Data commands: list, get, versions
- Partial ID matching for output lookup
- Field extraction from output data
- Multiple runs producing separate outputs
- Data versioning across multiple runs
- Error handling: non-existent output, model, and field cases
Security:
- No security concerns - tests create isolated temp directories and clean up properly
- No hardcoded credentials or sensitive data
Suggestions (Non-Blocking)
-
Consider extracting shared test types: The inline type annotations like
(o: { modelName: string })are repeated multiple times. While this works fine, extracting a shared interface could improve maintainability:interface OutputSearchResult { id: string; modelName: string; methodName: string; status: string; }
-
Consider testing edge case with empty repo: The error handling tests are good, but testing behavior with an empty repository (no models at all) could be valuable.
-
Comment notes about log tests: The comments explaining why log tests are skipped (echo model doesn't produce logs) are helpful for future maintainers.
Verification
The tests follow the established patterns in the codebase and should pass the CI checks (deno check, deno lint, deno fmt, deno run test).
LGTM! This is a valuable addition to the test suite that will help catch regressions in the data output flow.
…-club#141) (systeminit#1205) ## Summary - Fixes [swamp-club#141](https://swamp.club/lab/issues/141): extension authors whose `_test.ts` imports the model source hit `TS7006` (implicit `any` on execute parameters) under strict mode. - Ships a type-only `ModelDefinition<TGlobalArgs>` in `@systeminit/swamp-testing` that authors opt into via `satisfies ModelDefinition<typeof Schema>` (or the `defineModel` function-form alternative). The literal stays identical — only a trailing satisfies and a type-only import are added. - `context.globalArgs` narrows to the schema-inferred shape (verified by probing an undeclared field → TS2339). This is a real type-safety upgrade, not a TS7006 silencer. - Default authoring experience is **unchanged** — unannotated execute params remain the Quick Start. Existing extensions do not need to migrate. The `: any` workaround keeps working for anyone already using it. ## What's included - `packages/testing/model_definition_types.ts` — new type-only file with `ModelDefinition`, `MethodDefinition`, `CheckDefinition`, `ResourceOutputSpec`, `FileOutputSpec`, `VersionUpgrade`, `CheckResult`, and the `defineModel` helper. - `packages/testing/types.ts` — `MethodContext<TGlobalArgs = Record<string, unknown>>` (additive, backward compatible). - `packages/testing/mod.ts` — new "Model authoring" export block. - `packages/testing/model_definition_types_test.ts` — type-level regression test asserting both `satisfies` and `defineModel` paths narrow `globalArgs` under strict+noImplicitAny. - `src/domain/models/testing_package_compat_test.ts` — extended to guard drift between testing and canonical `ModelDefinition` / `MethodDefinition`. - `.claude/skills/swamp-extension-model/references/typing.md` — new reference with before/after, "when not to use," and cross-links to the compat test and issue systeminit#141. - `.claude/skills/swamp-extension-model/SKILL.md` — Key Rule systeminit#5 points at typing.md. - `.claude/skills/swamp-extension-model/references/testing.md` — cross-link banner at the top. - `packages/testing/README.md` — new "Model authoring escape hatch" subsection. - `packages/testing/deno.json` — bumped 0.2.0 → 0.3.0. ## Author-facing usage ```typescript import { z } from "npm:zod@4"; import type { ModelDefinition } from "jsr:@systeminit/swamp-testing"; const GlobalArgsSchema = z.object({ region: z.string() }); export const model = { type: "@myorg/my-model", version: "2026.04.21.1", globalArguments: GlobalArgsSchema, methods: { run: { description: "Run the model", arguments: z.object({ bucket: z.string() }), execute: async (_args, context) => { // context.globalArgs narrows to { region: string } return { dataHandles: [] }; }, }, }, } satisfies ModelDefinition<typeof GlobalArgsSchema>; ``` ## Why not a per-method args-narrowing helper The plan's initial `defineModel` sketch intended per-method `args` inference via mapped-intersection generics. Both the F-bounded recursive constraint and the `Omit`-based override pattern failed contextual typing during inference — per-method args narrowing for inline literals requires a builder-style API (as used by tRPC / zod's own `.input()` chain), which is out of scope here. Authors narrow `args` per method with `MySchema.parse(args)` at runtime (the canonical Zod way). `typing.md` documents this. ## Test plan - [x] `deno check main.ts packages/testing/mod.ts` passes - [x] `deno run test` — all 4505 existing tests pass; new 4 type-level + smoke tests pass - [x] `deno lint` clean - [x] `deno fmt` applied - [x] `deno run review-skills` — all skills pass the 90% threshold - [x] Scratch-repo verification: reproduces TS7006 on the pre-fix shape; compiles cleanly with `satisfies ModelDefinition<typeof Schema>` and `defineModel({ ... })`; narrowing probe confirms `context.globalArgs.region` is typed and `context.globalArgs.undeclaredField` errors as expected. - [x] Drift-guard verification: injecting `description: number` into testing-side `MethodDefinition` immediately failed `testing_package_compat_test.ts` as designed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
No description provided.