Transform graph testing & verification - #40
Conversation
📝 WalkthroughWalkthroughAdds the ChangesTransform-test command feature
E2E error expectation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as transform-test run
participant Config as Suite and assertion parsers
participant Runner as Subgraph execution
participant API as Metabase API
CLI->>Config: Load suite and resolve assertions
Config-->>CLI: Return normalized test arguments
CLI->>Runner: Execute target test with fixtures
Runner->>API: Submit subgraph test run
API-->>Runner: Return test result
Runner-->>CLI: Render result and exit on failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
| 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.
| 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
| // `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.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@skill-data/transform-test-plan/references/checklist.md`:
- Around line 41-45: Update
skill-data/transform-test-plan/references/checklist.md lines 41-45 to require
full-expression analysis of filters, projections, default mappings, and declared
key relationships before classifying orphan behavior as dropped, NULL-preserved,
or default-row. Update skill-data/transform-test-plan/references/checks.md lines
146-153 to detect unmatched keys with an explicit key-based predicate rather
than checking whether a copied attribute is NULL, so legitimate nullable
attributes are not misclassified.
In `@skill-data/transform-test-plan/references/checks.md`:
- Around line 44-50: Update the A2 Conservation reconciliation guidance to
require an independent COUNT(*) comparison alongside additive-measure SUM
comparisons for every declared tie. Ensure the count subqueries use the same
input, output, and declared exclusions, and report mismatched row counts using
the existing IS DISTINCT FROM assertion pattern.
In `@skill-data/transform-test-plan/SKILL.md`:
- Line 84: Update the fenced directory-tree block in SKILL.md by adding the text
language tag to its opening fence, changing the untyped fence to a text fence
while leaving the directory listing unchanged.
🪄 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 Plus
Run ID: 7c12c6af-2a71-4854-bf09-26f191cf4cea
📒 Files selected for processing (6)
skill-data/data-workflow/references/building-clean-tables.mdskill-data/transform-test-plan/SKILL.mdskill-data/transform-test-plan/references/checklist.mdskill-data/transform-test-plan/references/checks.mdskill-data/transform-test/SKILL.mdtests/e2e/skills.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- skill-data/transform-test/SKILL.md
| 6. **Orphan handling per join: drop / keep-with-NULLs / default row — and is | ||
| an orphan tolerated (warn) or forbidden (error)?** (C3, I5) | ||
| Detect: join type. INNER = drop, LEFT = keep-with-NULLs, COALESCE to a | ||
| sentinel = default row. Tolerated-vs-forbidden is the owner's call — | ||
| profiling says whether orphans exist today, not whether they're acceptable. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make orphan handling depend on explicit key semantics, not heuristics.
The checklist and catalog can derive incorrect tests when join type or nullable copied columns are treated as definitive. Analyze filters, projections, default mappings, and the declared key relationship.
skill-data/transform-test-plan/references/checklist.md#L41-L45: require full-expression analysis before classifying drop, NULL preservation, or default-row behavior.skill-data/transform-test-plan/references/checks.md#L146-L153: replacet.<copied attr> IS NULLwith an explicit unmatched-key predicate that cannot flag legitimate NULL attributes.
📍 Affects 2 files
skill-data/transform-test-plan/references/checklist.md#L41-L45(this comment)skill-data/transform-test-plan/references/checks.md#L146-L153
🤖 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 `@skill-data/transform-test-plan/references/checklist.md` around lines 41 - 45,
Update skill-data/transform-test-plan/references/checklist.md lines 41-45 to
require full-expression analysis of filters, projections, default mappings, and
declared key relationships before classifying orphan behavior as dropped,
NULL-preserved, or default-row. Update
skill-data/transform-test-plan/references/checks.md lines 146-153 to detect
unmatched keys with an explicit key-based predicate rather than checking whether
a copied attribute is NULL, so legitimate nullable attributes are not
misclassified.
| **A2 — Conservation reconciliation.** Row counts and additive-measure sums tie | ||
| from input to output; catches dropped rows and join double-counting at once. | ||
| - Applies: per declared tie (which input, which declared exclusions). | ||
| - Assert (error): independent scalar subqueries compared with | ||
| `IS DISTINCT FROM`, returning the mismatched pair: | ||
| `SELECT (SELECT SUM(t.m) FROM test_output t) AS output_sum, | ||
| (SELECT SUM(s.m) FROM <input> s) AS input_sum WHERE (…) IS DISTINCT FROM (…)`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include row-count reconciliation, not only SUM comparisons.
A2 claims that row counts and additive-measure sums tie, but the specified assertion only compares SUM(...). Dropping zero- or NULL-valued rows can therefore pass the check. Add an independent COUNT(*) comparison for each declared tie, honoring exclusions.
🤖 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 `@skill-data/transform-test-plan/references/checks.md` around lines 44 - 50,
Update the A2 Conservation reconciliation guidance to require an independent
COUNT(*) comparison alongside additive-measure SUM comparisons for every
declared tie. Ensure the count subqueries use the same input, output, and
declared exclusions, and report mismatched row counts using the existing IS
DISTINCT FROM assertion pattern.
| cell is the null policy's only enforcement. | ||
| 7. **Write the suites**, one directory per target: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced directory tree.
The untyped fence triggers markdownlint MD040. Use text for this directory listing.
Proposed fix
- ```
+ ```text📝 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.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@skill-data/transform-test-plan/SKILL.md` at line 84, Update the fenced
directory-tree block in SKILL.md by adding the text language tag to its opening
fence, changing the untyped fence to a text fence while leaving the directory
listing unchanged.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/skill-data/transform-test-plan/references/checks.md (1)
47-63: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd an explicit row-count reconciliation to A2.
A2 promises to tie row counts and additive sums, but the provided SQL shape compares only
SUM(...). Dropped zero-valued rows or offsetting changes can therefore pass. Define a separateCOUNT(*)scalar comparison, respecting declared exclusions, alongside the sum check.🤖 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 `@packages/cli/skill-data/transform-test-plan/references/checks.md` around lines 47 - 63, Update the A2 Conservation reconciliation guidance to require a separate scalar COUNT(*) comparison alongside the existing SUM(...) comparison, using the same declared exclusions and IS DISTINCT FROM mismatch pattern. Ensure the row-count check compares input and output independently rather than through a fact-to-fact join, while preserving the existing additive-sum reconciliation and authoritative aggregate guidance.packages/cli/src/commands/transform-test/assert.test.ts (1)
31-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the expected error text for failure paths.
These tests only check
ConfigError. Assert the stable message (or exact stable message slice for filesystem errors) too, so CLI diagnostics remain covered.As per coding guidelines, tests must “assert both error type and exact message slice.”
Also applies to: 57-59, 101-110
🤖 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 `@packages/cli/src/commands/transform-test/assert.test.ts` around lines 31 - 33, Update the failure-path tests for classifyAssertToken, including the cases around the bare name and referenced lines 57-59 and 101-110, to assert both ConfigError and the expected stable error-message text or exact stable message slice. Preserve the existing error-type assertions while adding precise diagnostic coverage for each filesystem-related and validation failure.Source: Coding guidelines
packages/cli/src/commands/transform-test/assert.ts (1)
82-87: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude directory entries from assertion globs.
readdir()includes non-directory.sqlentries, so a directory namedsomething.sqlmatches the current name-only filter and makes the glob fail whenreadFileis called. Map entries with file/stat checks and keep only regular files; add a regression covering this edge case.🤖 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 `@packages/cli/src/commands/transform-test/assert.ts` around lines 82 - 87, Update the assertion-glob matching logic around matches to inspect each readdir entry with the appropriate file/stat check, retaining only regular files before sorting and mapping paths. Exclude directories even when their names satisfy the prefix/suffix filter, and add a regression test covering a directory named with the matching .sql pattern.
🧹 Nitpick comments (1)
packages/cli/src/commands/runtime.ts (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep these comments focused on non-obvious rationale.
Both comments describe implementation details and internal paths. Replace them with concise WHY-only comments.
packages/cli/src/commands/runtime.ts#L46-L48: explain only that raw arguments preserve repeated flags.packages/cli/src/commands/runtime.ts#L97-L99: explain only that lazy loading avoids loading the client graph on paths that do not need it.As per coding guidelines, comments must “only” explain a non-obvious WHY and must not describe WHAT or paths.
🤖 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 `@packages/cli/src/commands/runtime.ts` around lines 46 - 48, In packages/cli/src/commands/runtime.ts lines 46-48, replace the comment above rawArgs with a concise WHY-only explanation that raw arguments preserve repeated flags, without mentioning implementation details or internal paths. In packages/cli/src/commands/runtime.ts lines 97-99, replace the lazy-loading comment with a concise WHY-only explanation that it avoids loading the client graph on paths that do not need it; do not describe what is loaded or reference paths.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 `@packages/cli/skill-data/transform-test/SKILL.md`:
- Line 75: Update the summary documentation in the transform-test SKILL.md to
reflect that runSubgraph uses targetLabels(args.targetType) for the summary
prefix. Replace the fixed “Transform <id>” wording with a target-neutral
placeholder, or explicitly document both “Transform” and “Card” labels while
preserving the existing assertion breakdown and table details.
- Line 50: Remove the WHAT/path comments from the Markdown examples in SKILL.md,
including the header/rows annotation and the referenced sections around the
other affected examples. Preserve the examples unchanged, and move any necessary
context into surrounding prose while retaining only comments that explain
non-obvious reasons.
In `@packages/cli/src/commands/transform-test/subgraph.ts`:
- Around line 99-115: Thread the global interrupt signal from runSubgraph
through buildRunParams into every readFixtureFile call, including input and
expected fixtures. Extend readFixtureFile to accept the signal and pass it to
its asynchronous filesystem read operation, preserving existing fixture paths
and error labels while allowing Ctrl-C to cancel pending I/O.
---
Outside diff comments:
In `@packages/cli/skill-data/transform-test-plan/references/checks.md`:
- Around line 47-63: Update the A2 Conservation reconciliation guidance to
require a separate scalar COUNT(*) comparison alongside the existing SUM(...)
comparison, using the same declared exclusions and IS DISTINCT FROM mismatch
pattern. Ensure the row-count check compares input and output independently
rather than through a fact-to-fact join, while preserving the existing
additive-sum reconciliation and authoritative aggregate guidance.
In `@packages/cli/src/commands/transform-test/assert.test.ts`:
- Around line 31-33: Update the failure-path tests for classifyAssertToken,
including the cases around the bare name and referenced lines 57-59 and 101-110,
to assert both ConfigError and the expected stable error-message text or exact
stable message slice. Preserve the existing error-type assertions while adding
precise diagnostic coverage for each filesystem-related and validation failure.
In `@packages/cli/src/commands/transform-test/assert.ts`:
- Around line 82-87: Update the assertion-glob matching logic around matches to
inspect each readdir entry with the appropriate file/stat check, retaining only
regular files before sorting and mapping paths. Exclude directories even when
their names satisfy the prefix/suffix filter, and add a regression test covering
a directory named with the matching .sql pattern.
---
Nitpick comments:
In `@packages/cli/src/commands/runtime.ts`:
- Around line 46-48: In packages/cli/src/commands/runtime.ts lines 46-48,
replace the comment above rawArgs with a concise WHY-only explanation that raw
arguments preserve repeated flags, without mentioning implementation details or
internal paths. In packages/cli/src/commands/runtime.ts lines 97-99, replace the
lazy-loading comment with a concise WHY-only explanation that it avoids loading
the client graph on paths that do not need it; do not describe what is loaded or
reference paths.
🪄 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 Plus
Run ID: d7ad83ec-c741-4cd9-9f62-fb835de49dbb
📒 Files selected for processing (16)
packages/cli/skill-data/data-workflow/references/building-clean-tables.mdpackages/cli/skill-data/transform-test-plan/SKILL.mdpackages/cli/skill-data/transform-test-plan/references/checklist.mdpackages/cli/skill-data/transform-test-plan/references/checks.mdpackages/cli/skill-data/transform-test/SKILL.mdpackages/cli/src/commands/parse-id.tspackages/cli/src/commands/runtime.tspackages/cli/src/commands/transform-test/assert.test.tspackages/cli/src/commands/transform-test/assert.tspackages/cli/src/commands/transform-test/index.tspackages/cli/src/commands/transform-test/inputs.tspackages/cli/src/commands/transform-test/run.tspackages/cli/src/commands/transform-test/subgraph.test.tspackages/cli/src/commands/transform-test/subgraph.tspackages/cli/src/commands/transform-test/suite.test.tspackages/cli/src/commands/transform-test/suite.ts
| 3,2024-01-03,3 | ||
| CSV | ||
|
|
||
| # The expected output: header = the columns your transform SELECTs, rows = what it should produce. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove WHAT/path comments from Markdown examples.
These comments annotate values, paths, or behavior rather than explaining a non-obvious reason. Move that information into surrounding prose or retain only WHY comments.
As per coding guidelines, **/*.{ts,tsx,md} comments must explain only non-obvious WHY and must not describe WHAT or paths.
Also applies to: 116-133, 151-152
🤖 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 `@packages/cli/skill-data/transform-test/SKILL.md` at line 50, Remove the
WHAT/path comments from the Markdown examples in SKILL.md, including the
header/rows annotation and the referenced sections around the other affected
examples. Preserve the examples unchanged, and move any necessary context into
surrounding prose while retaining only comments that explain non-obvious
reasons.
Source: Coding guidelines
|
|
||
| Reading the result: | ||
|
|
||
| - The plain (text) summary says `Transform <id> test run passed.` or `Transform <id> test run FAILED …`, followed — whenever assertions ran — by a one-line breakdown (`N assertions — A passed, B FAILED, C warn …`) and a per-assertion table (name / status / failing rows). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the target-specific summary label.
runSubgraph formats the summary through targetLabels(args.targetType), so card runs do not say Transform <id>. Use a target-neutral placeholder or document both Transform and Card.
🤖 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 `@packages/cli/skill-data/transform-test/SKILL.md` at line 75, Update the
summary documentation in the transform-test SKILL.md to reflect that runSubgraph
uses targetLabels(args.targetType) for the summary prefix. Replace the fixed
“Transform <id>” wording with a target-neutral placeholder, or explicitly
document both “Transform” and “Card” labels while preserving the existing
assertion breakdown and table details.
| export async function buildRunParams(args: SubgraphRunArgs): Promise<TestRunParams> { | ||
| const inputs = await Promise.all( | ||
| args.inputs.map(async ({ tableId, path }) => ({ | ||
| table_id: tableId, | ||
| file: await readFixtureFile(path, `--input ${tableId}`), | ||
| })), | ||
| ); | ||
| const params: TestRunParams = { | ||
| sources: args.sources, | ||
| inputs, | ||
| ignore_columns: args.ignoreColumns, | ||
| assertions: args.assertions, | ||
| }; | ||
| if (args.expected !== undefined) { | ||
| params.expected = await readFixtureFile(args.expected, "--expected"); | ||
| } | ||
| return params; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -i 'subgraph\.ts$|flags\.ts$|runtime\.ts$' packages/cli/src | sed -n '1,80p'
echo
echo "== subgraph outline =="
ast-grep outline packages/cli/src/commands/transform-test/subgraph.ts --view compact || true
echo
echo "== subgraph relevant lines =="
cat -n packages/cli/src/commands/transform-test/subgraph.ts | sed -n '1,180p'
echo
echo "== readFixtureFile definitions/usages =="
rg -n "readFixtureFile|buildRunParams|TestRunParams|interruptSignal" packages/cli/src -SRepository: metabase/metabase-cli
Length of output: 11309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime/upload.ts =="
cat -n packages/cli/src/runtime/upload.ts | sed -n '1,160p'
echo
echo "== runtime/interrupt.ts =="
cat -n packages/cli/src/runtime/interrupt.ts | sed -n '1,80p'
echo
echo "== transform-test subprocess command flow =="
cat -n packages/cli/src/commands/transform-test/subgraph.ts | sed -n '180,280p'
echo
echo "== CommonContext and create command signatures =="
rg -n "interface CommonContext|export interface CommonContext|type CommonContext|buildRunParams|run\\(ctx" packages/cli/src/commands/transform-test/subgraph.ts packages/cli/src/commands/runtime.ts packages/cli/src -S -g '*.ts' | sed -n '1,160p'Repository: metabase/metabase-cli
Length of output: 4693
Accept and pass interruptSignal into fixture reads.
readFixtureFile currently only uses readFile(path), so Ctrl-C during buildRunParams will not cancel pending fixture I/O before the API request starts. Thread an explicit signal from runSubgraph through buildRunParams to readFixtureFile, and pass it into the asynchronous filesystem operation so the command can exit promptly via the global interrupt signal.
🤖 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 `@packages/cli/src/commands/transform-test/subgraph.ts` around lines 99 - 115,
Thread the global interrupt signal from runSubgraph through buildRunParams into
every readFixtureFile call, including input and expected fixtures. Extend
readFixtureFile to accept the signal and pass it to its asynchronous filesystem
read operation, preserving existing fixture paths and error labels while
allowing Ctrl-C to cancel pending I/O.
Source: Coding guidelines
1 participant
Summary by CodeRabbit
New Features
transform-testCLI commands for discovering test inputs and running transform or card tests.Bug Fixes
Tests