Transform graph testing & verification#40
Open
galdre wants to merge 10 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new ChangesTransform-test command feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as run.ts command
participant Suite as parseSuite/assert
participant Subgraph as subgraph.ts
participant Server as Metabase API
CLI->>Suite: loadSuite / resolveAssertions
CLI->>Subgraph: runSubgraph(client, args, ctx)
Subgraph->>Subgraph: buildSubgraphForm(args)
Subgraph->>Server: POST subgraph run endpoint (FormData)
Server-->>Subgraph: TestRunResult
Subgraph->>Subgraph: shouldFail(result)
Subgraph->>CLI: renderRunResult(result, ctx)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/domain/transform-test-run.test.ts (1)
13-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse full deep equality assertions for schema parse tests.
Tests 1, 2, and 4 use
.successchecks and piecemeal property assertions instead oftoEqual. Per coding guidelines, assertions must be full deep equality where possible.♻️ Proposed refactor
it("accepts a schema-qualified input table", () => { - expect(TestRunInput.safeParse(base).success).toBe(true); + expect(TestRunInput.parse(base)).toEqual(base); }); it("accepts a null schema (engines without schemas)", () => { - const parsed = TestRunInput.safeParse({ ...base, schema: null }); - expect(parsed.success).toBe(true); - expect(parsed.data?.schema).toBeNull(); + expect(TestRunInput.parse({ ...base, schema: null })).toEqual({ ...base, schema: null }); }); it("rejects a missing schema key", () => { const { schema: _schema, ...rest } = base; expect(TestRunInput.safeParse(rest).success).toBe(false); }); it("compact pick preserves a null schema", () => { - expect(TestRunInputCompact.parse({ ...base, schema: null }).schema).toBeNull(); + expect(TestRunInputCompact.parse({ ...base, schema: null })).toEqual({ + table_id: 229, + schema: null, + name: "orders", + columns: ["id", "total"], + }); });Additionally, consider adding tests for
AssertionResult,TestRunResult, and their compact variants to match the coverage depth ofTestRunInput.As per coding guidelines: "Assertions in unit tests must be full deep equality where possible (
toEqual(full object/array)), not piecemeal property checks."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/transform-test-run.test.ts` around lines 13 - 30, The schema parse tests currently rely on `.success` and single-property checks instead of full object comparisons, so update the assertions in the `TestRunInput` and `TestRunInputCompact` cases to use deep equality with the parsed object where applicable. In the `TestRunInput.safeParse` and `TestRunInputCompact.parse` tests, assert the entire parsed result/object rather than checking `success` or only `schema`. Also add similar full-equality coverage for `AssertionResult`, `TestRunResult`, and their compact variants to match the same validation depth.Source: Coding guidelines
src/commands/transform-test/assert.ts (2)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the discriminated union members per coding guidelines.
The guideline requires inline object types in unions to be named with
interfaceortype. Extract each member:♻️ Proposed refactor
-export type AssertToken = { kind: "file"; path: string } | { kind: "glob"; pattern: string }; +export interface AssertFileToken { + kind: "file"; + path: string; +} + +export interface AssertGlobToken { + kind: "glob"; + pattern: string; +} + +export type AssertToken = AssertFileToken | AssertGlobToken;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/transform-test/assert.ts` at line 15, The AssertToken union in assert.ts uses inline object members, which violates the naming guideline. Extract each union member into separately named interfaces or type aliases, then update AssertToken to reference those named members while keeping the same discriminant field (`kind`) and existing properties (`path`, `pattern`).Source: Coding guidelines
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove WHAT comments per coding guidelines.
These comments restate what the code already expresses clearly. The guideline says to avoid comments unless the WHY is non-obvious and to never write WHAT comments.
♻️ Proposed refactor
-// An --assert value is a `.sql` file or a glob of them; inline SQL is not supported. export type AssertToken = AssertFileToken | AssertGlobToken; -// basename with `*` ⇒ glob; plain `.sql` ⇒ file; anything else rejected. export function classifyAssertToken(token: string): AssertToken {And at line 94:
-// Each `.sql` file → one assertion, named by basename without extension; severity defaults to error. export async function resolveAssertions(tokens: AssertToken[]): Promise<AssertionDef[]> {Also applies to: 17-17, 94-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/transform-test/assert.ts` at line 14, Remove the WHAT-style comments in assert.ts that simply restate the code, including the top-level note about --assert inputs and the similar comment near the assert handling logic; keep only comments that explain non-obvious WHY, and use the surrounding symbols in this file (such as the assert parsing/validation code and related helpers) to preserve readability without redundant commentary.Source: Coding guidelines
src/commands/transform-test/run.ts (1)
143-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exporting and reusing
DEFAULT_TARGET_TYPEinstead of the inline"transform"literal.Line 145 uses
?? "transform"as a type-narrowing fallback that never triggers at runtime (whenidGivenis true,targetTypeis always defined from line 143). The literal"transform"duplicatesDEFAULT_TARGET_TYPEfromsubgraph.ts(currently not exported). Exporting and reusing it would eliminate the repeated magic literal.♻️ Optional refactor
In
subgraph.ts:-const DEFAULT_TARGET_TYPE: TargetType = "transform"; +export const DEFAULT_TARGET_TYPE: TargetType = "transform";In
run.ts:import { + DEFAULT_TARGET_TYPE, parseColumnList, parseInputPairs, parseTargetType, runSubgraph, type SubgraphRunArgs, targetLabels, targetTypeFlag, } from "./subgraph";Then:
- ? parseId(args.id, targetLabels(targetType ?? "transform").positionalLabel) + ? parseId(args.id, targetLabels(targetType ?? DEFAULT_TARGET_TYPE).positionalLabel)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/transform-test/run.ts` around lines 143 - 146, The `run.ts` target parsing uses an inline "transform" fallback in `targetLabels(targetType ?? "transform")`, but that fallback is unreachable and duplicates the default from `subgraph.ts`. Export `DEFAULT_TARGET_TYPE` from `subgraph.ts` and reuse it in `run.ts` for both `parseTargetType` defaults and `targetLabels`, replacing the magic string and keeping `parseId`/`targetLabels` aligned with the shared constant.Source: Coding guidelines
src/commands/transform-test/index.ts (1)
1-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
defineCommandGroupfor this dispatcher.src/commands/transform-test/index.tsshould follow the other parent command indexes so the shared augment metadata — including explicitcapabilities: null— is attached automatically.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/transform-test/index.ts` around lines 1 - 13, The transform-test dispatcher is using the wrong command helper, so the parent command metadata augmentation is not being applied. Update the default export in transform-test’s index module to use defineCommandGroup instead of defineCommand, matching the other parent command indexes so shared augment metadata such as explicit capabilities: null is attached automatically.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/transform-test/suite.test.ts`:
- Line 101: The tests in parseSuite should assert the exact ConfigError message
instead of only matching the error type. Update the failing rejection checks in
suite.test.ts around parseSuite so they verify the known stable error string for
each case, using the parseSuite helper and ConfigError as the reference points.
In `@src/core/http/errors.ts`:
- Around line 41-43: parseEnvelopeMessage is still treating envelope.error as an
opaque value, so object-shaped errors like { message: "..." } fall through to
later fallbacks. Update the error extraction logic in the envelope parsing flow
to detect when envelope.error is an object and prefer its message field before
checking error-message or generic fallback text, while keeping the existing
z.unknown().optional() schema intact.
---
Nitpick comments:
In `@src/commands/transform-test/assert.ts`:
- Line 15: The AssertToken union in assert.ts uses inline object members, which
violates the naming guideline. Extract each union member into separately named
interfaces or type aliases, then update AssertToken to reference those named
members while keeping the same discriminant field (`kind`) and existing
properties (`path`, `pattern`).
- Line 14: Remove the WHAT-style comments in assert.ts that simply restate the
code, including the top-level note about --assert inputs and the similar comment
near the assert handling logic; keep only comments that explain non-obvious WHY,
and use the surrounding symbols in this file (such as the assert
parsing/validation code and related helpers) to preserve readability without
redundant commentary.
In `@src/commands/transform-test/index.ts`:
- Around line 1-13: The transform-test dispatcher is using the wrong command
helper, so the parent command metadata augmentation is not being applied. Update
the default export in transform-test’s index module to use defineCommandGroup
instead of defineCommand, matching the other parent command indexes so shared
augment metadata such as explicit capabilities: null is attached automatically.
In `@src/commands/transform-test/run.ts`:
- Around line 143-146: The `run.ts` target parsing uses an inline "transform"
fallback in `targetLabels(targetType ?? "transform")`, but that fallback is
unreachable and duplicates the default from `subgraph.ts`. Export
`DEFAULT_TARGET_TYPE` from `subgraph.ts` and reuse it in `run.ts` for both
`parseTargetType` defaults and `targetLabels`, replacing the magic string and
keeping `parseId`/`targetLabels` aligned with the shared constant.
In `@src/domain/transform-test-run.test.ts`:
- Around line 13-30: The schema parse tests currently rely on `.success` and
single-property checks instead of full object comparisons, so update the
assertions in the `TestRunInput` and `TestRunInputCompact` cases to use deep
equality with the parsed object where applicable. In the
`TestRunInput.safeParse` and `TestRunInputCompact.parse` tests, assert the
entire parsed result/object rather than checking `success` or only `schema`.
Also add similar full-equality coverage for `AssertionResult`, `TestRunResult`,
and their compact variants to match the same validation depth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 721583b8-8d38-4c56-85f6-2e53e93d5f87
📒 Files selected for processing (19)
skill-data/transform-test/SKILL.mdsrc/commands/parse-id.tssrc/commands/runtime.tssrc/commands/transform-test/assert.test.tssrc/commands/transform-test/assert.tssrc/commands/transform-test/index.tssrc/commands/transform-test/inputs.tssrc/commands/transform-test/run.tssrc/commands/transform-test/subgraph.test.tssrc/commands/transform-test/subgraph.tssrc/commands/transform-test/suite.test.tssrc/commands/transform-test/suite.tssrc/core/http/errors.tssrc/domain/transform-test-run.test.tssrc/domain/transform-test-run.tssrc/main.tssrc/runtime/citty.test.tssrc/runtime/citty.tssrc/runtime/upload.ts
Comment on lines
+87
to
+99
| it("expands a glob to one assertion per matching .sql file, sorted by name", async () => { | ||
| const out = await resolveAssertions([{ kind: "glob", pattern: join(dir, "*.sql") }]); | ||
| expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]); | ||
| expect(out.every((a) => a.severity === "error")).toBe(true); | ||
| }); | ||
|
|
||
| it("resolves multiple file tokens preserving order, each named by basename", async () => { | ||
| const out = await resolveAssertions([ | ||
| { kind: "file", path: join(dir, "has_rows.sql") }, | ||
| { kind: "file", path: join(dir, "no_negatives.sql") }, | ||
| ]); | ||
| expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use full deep equality assertions per coding guidelines.
The glob expansion and multiple-file tests check only a.name and a.severity piecemeal. The guideline requires full deep equality with toEqual where possible. The SQL content is known from the temp files, so full assertions are feasible.
💚 Proposed fix for glob expansion test
it("expands a glob to one assertion per matching .sql file, sorted by name", async () => {
const out = await resolveAssertions([{ kind: "glob", pattern: join(dir, "*.sql") }]);
- expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]);
- expect(out.every((a) => a.severity === "error")).toBe(true);
+ expect(out).toEqual([
+ {
+ name: "has_rows",
+ sql: "SELECT * FROM test_output WHERE 1=0",
+ severity: "error",
+ },
+ {
+ name: "no_negatives",
+ sql: "SELECT * FROM test_output WHERE revenue < 0",
+ severity: "error",
+ },
+ ]);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("expands a glob to one assertion per matching .sql file, sorted by name", async () => { | |
| const out = await resolveAssertions([{ kind: "glob", pattern: join(dir, "*.sql") }]); | |
| expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]); | |
| expect(out.every((a) => a.severity === "error")).toBe(true); | |
| }); | |
| it("resolves multiple file tokens preserving order, each named by basename", async () => { | |
| const out = await resolveAssertions([ | |
| { kind: "file", path: join(dir, "has_rows.sql") }, | |
| { kind: "file", path: join(dir, "no_negatives.sql") }, | |
| ]); | |
| expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]); | |
| }); | |
| it("expands a glob to one assertion per matching .sql file, sorted by name", async () => { | |
| const out = await resolveAssertions([{ kind: "glob", pattern: join(dir, "*.sql") }]); | |
| expect(out).toEqual([ | |
| { | |
| name: "has_rows", | |
| sql: "SELECT * FROM test_output WHERE 1=0", | |
| severity: "error", | |
| }, | |
| { | |
| name: "no_negatives", | |
| sql: "SELECT * FROM test_output WHERE revenue < 0", | |
| severity: "error", | |
| }, | |
| ]); | |
| }); | |
| it("resolves multiple file tokens preserving order, each named by basename", async () => { | |
| const out = await resolveAssertions([ | |
| { kind: "file", path: join(dir, "has_rows.sql") }, | |
| { kind: "file", path: join(dir, "no_negatives.sql") }, | |
| ]); | |
| expect(out.map((a) => a.name)).toEqual(["has_rows", "no_negatives"]); | |
| }); |
Source: Coding guidelines
| assertions: | ||
| - name: bad | ||
| `; | ||
| await expect(parseSuite(yaml, "suite.yaml")).rejects.toThrow(ConfigError); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert exact error messages per coding guidelines.
The guideline requires exact error strings when checking failures. Both rejection cases produce a known, stable ConfigError message — assert it.
💚 Proposed fix
it("rejects an assertion with neither sql nor file", async () => {
...
- await expect(parseSuite(yaml, "suite.yaml")).rejects.toThrow(ConfigError);
+ await expect(parseSuite(yaml, "suite.yaml")).rejects.toThrow(
+ "Suite assertion 'bad' must set exactly one of 'sql' or 'file'.",
+ );
});
it("rejects an assertion with both sql and file", async () => {
...
- await expect(parseSuite(yaml, "suite.yaml")).rejects.toThrow(ConfigError);
+ await expect(parseSuite(yaml, "suite.yaml")).rejects.toThrow(
+ "Suite assertion 'bad' must set exactly one of 'sql' or 'file'.",
+ );
});Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/transform-test/suite.test.ts` at line 101, The tests in
parseSuite should assert the exact ConfigError message instead of only matching
the error type. Update the failing rejection checks in suite.test.ts around
parseSuite so they verify the known stable error string for each case, using the
parseSuite helper and ConfigError as the reference points.
Source: Coding guidelines
Comment on lines
+41
to
+43
| // `error` may be a structured object, not a string; accept any shape so a non-string | ||
| // doesn't break parsing the whole envelope. | ||
| error: z.unknown().optional(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test cases or API documentation showing structured error shapes
rg -n "error.*message" --type=ts src/core/http/ -g '!*.test.ts' -C3
rg -n "envelope\.error" --type=ts src/core/http/ -C3Repository: metabase/metabase-cli
Length of output: 2015
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,280p' src/core/http/errors.ts
printf '\n---\n'
rg -n "parseEnvelopeMessage|formatErrorTree|error-message|specific-errors|via|errors:" src -C 2
printf '\n---\n'
rg -n "structured error|error-message|specific-errors|parseEnvelopeMessage" src -g '!*.snap' -C 3Repository: metabase/metabase-cli
Length of output: 34950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for tests or fixtures that exercise error envelopes and structured error payloads.
fd -a -t f "errors" src test . | sed -n '1,120p'
printf '\n---\n'
rg -n "parseEnvelopeMessage|formatErrorTree|error-message|specific-errors|envelope\.error|via:" src test -g '!*.snap' -C 2Repository: metabase/metabase-cli
Length of output: 7026
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the envelope schema and message extraction logic.
sed -n '1,260p' src/core/http/errors.tsRepository: metabase/metabase-cli
Length of output: 7889
Unwrap object-shaped envelope.error values
parseEnvelopeMessage still drops structured envelope.error payloads like { message: "..." }. Extract error.message here before the fallback chain so those messages surface instead of falling through to error-message or generic text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/http/errors.ts` around lines 41 - 43, parseEnvelopeMessage is still
treating envelope.error as an opaque value, so object-shaped errors like {
message: "..." } fall through to later fallbacks. Update the error extraction
logic in the envelope parsing flow to detect when envelope.error is an object
and prefer its message field before checking error-message or generic fallback
text, while keeping the existing z.unknown().optional() schema intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
transform-testcommand withinputsandrunsubcommands for validating transform/card behavior.--ignore-columns.transform-testnow appears in the bundled visible skills list and command help.Bug Fixes
Documentation
transform-testskill/command documentation, including supported workflow and limitations.