test: impact analysis DAG traversal and training import markdown parser#374
Open
anandgupta42 wants to merge 1 commit intomainfrom
Open
test: impact analysis DAG traversal and training import markdown parser#374anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Add 34 tests for two newly-added tools (impact-analysis, training-import) that shipped with zero test coverage. These tests cover the core pure logic: findDownstream graph traversal, formatImpactReport output, parseMarkdownSections parsing, and slugify normalization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_011NpWhe2sbo52QP8Af9dA34
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdds two new Bun test files: one for impact analysis functionality testing DAG traversal and report generation, another for training import utilities testing slug generation and markdown section parsing with comprehensive unit test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
packages/opencode/test/altimate/tools/impact-analysis.test.ts (1)
160-183: Depth/path assertions are currently traversal-order brittle.At Line 169-Line 172 and Line 181-Line 182, assertions rely on array index order. A harmless traversal-order refactor could fail these tests even with correct graph results.
Make assertions order-independent
- expect(result[0]).toMatchObject({ name: "fct_orders", depth: 1 }) - expect(result[1]).toMatchObject({ name: "dim_orders", depth: 2 }) - expect(result[2]).toMatchObject({ name: "report_orders", depth: 3 }) + expect(result).toEqual( + expect.arrayContaining([ + expect.objectContaining({ name: "fct_orders", depth: 1 }), + expect.objectContaining({ name: "dim_orders", depth: 2 }), + expect.objectContaining({ name: "report_orders", depth: 3 }), + ]), + ) - expect(result[0].path).toEqual(["stg_orders", "fct_orders"]) - expect(result[1].path).toEqual(["stg_orders", "fct_orders", "report"]) + const byName = new Map(result.map((r) => [r.name, r])) + expect(byName.get("fct_orders")?.path).toEqual(["stg_orders", "fct_orders"]) + expect(byName.get("report")?.path).toEqual(["stg_orders", "fct_orders", "report"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts` around lines 160 - 183, Tests assert specific positions in the findDownstream result array which makes them brittle to traversal-order changes; update the two tests using result so they assert order-independently (e.g., verify length and that the array contains objects with the expected name/depth/path values using arrayContaining or by locating items by name before checking depth/path) while keeping references to findDownstream, result, and the specific expected entries (fct_orders, dim_orders, report_orders, and their paths) so the assertions validate content regardless of traversal order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts`:
- Around line 3-136: The test is using copied implementations of findDownstream
and formatImpactReport which can drift from production; modify the production
module to export a test-only hook object (e.g., add export const __test__ = {
findDownstream, formatImpactReport }) and update the test to import these
functions from that module instead of keeping the duplicated implementations in
the test file; then remove the duplicated findDownstream and formatImpactReport
code from the test so tests exercise the real production implementations.
In `@packages/opencode/test/altimate/tools/training-import.test.ts`:
- Around line 91-94: The test asserts that slugify truncates to 64 chars but
production slugify (in datamate.ts) doesn't; either remove the test in
training-import.test.ts or modify the slugify function to enforce
truncation—update datamate.ts's slugify to return the normalized slug.slice(0,
64) (and keep the test), or if truncation is not desired, delete the "truncates
to 64 characters" test; locate symbols slugify and datamate.ts to apply the
change and ensure tests reflect the chosen behavior.
---
Nitpick comments:
In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts`:
- Around line 160-183: Tests assert specific positions in the findDownstream
result array which makes them brittle to traversal-order changes; update the two
tests using result so they assert order-independently (e.g., verify length and
that the array contains objects with the expected name/depth/path values using
arrayContaining or by locating items by name before checking depth/path) while
keeping references to findDownstream, result, and the specific expected entries
(fct_orders, dim_orders, report_orders, and their paths) so the assertions
validate content regardless of traversal order.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3de7d166-2876-43ce-9927-ca7c7d14bd91
📒 Files selected for processing (2)
packages/opencode/test/altimate/tools/impact-analysis.test.tspackages/opencode/test/altimate/tools/training-import.test.ts
Comment on lines
+3
to
+136
| // Copy of findDownstream and formatImpactReport from | ||
| // src/altimate/tools/impact-analysis.ts (not exported, tested standalone) | ||
|
|
||
| interface DownstreamModel { | ||
| name: string | ||
| depth: number | ||
| materialized?: string | ||
| path: string[] | ||
| } | ||
|
|
||
| function findDownstream( | ||
| targetName: string, | ||
| models: Array<{ name: string; depends_on: string[]; materialized?: string }>, | ||
| ): DownstreamModel[] { | ||
| const results: DownstreamModel[] = [] | ||
| const visited = new Set<string>() | ||
|
|
||
| function walk(name: string, depth: number, path: string[]) { | ||
| for (const model of models) { | ||
| if (visited.has(model.name)) continue | ||
| const deps = model.depends_on.map((d) => d.split(".").pop()) | ||
| if (deps.includes(name)) { | ||
| visited.add(model.name) | ||
| const newPath = [...path, model.name] | ||
| results.push({ | ||
| name: model.name, | ||
| depth, | ||
| materialized: model.materialized, | ||
| path: newPath, | ||
| }) | ||
| walk(model.name, depth + 1, newPath) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| walk(targetName, 1, [targetName]) | ||
| return results | ||
| } | ||
|
|
||
| function formatImpactReport(data: { | ||
| model: string | ||
| column?: string | ||
| changeType: string | ||
| direct: DownstreamModel[] | ||
| transitive: DownstreamModel[] | ||
| affectedTestCount: number | ||
| columnImpact: string[] | ||
| totalModels: number | ||
| }): string { | ||
| const lines: string[] = [] | ||
| const target = data.column ? `${data.model}.${data.column}` : data.model | ||
| lines.push(`Impact Analysis: ${data.changeType.toUpperCase()} ${target}`) | ||
| lines.push("".padEnd(60, "=")) | ||
|
|
||
| const totalAffected = data.direct.length + data.transitive.length | ||
| const pct = data.totalModels > 0 ? ((totalAffected / data.totalModels) * 100).toFixed(1) : "0" | ||
| lines.push(`Blast radius: ${totalAffected}/${data.totalModels} models (${pct}%)`) | ||
| lines.push("") | ||
|
|
||
| if (data.changeType === "remove" && totalAffected > 0) { | ||
| lines.push("WARNING: This is a BREAKING change. All downstream models will fail.") | ||
| lines.push("") | ||
| } else if (data.changeType === "rename" && totalAffected > 0) { | ||
| lines.push("WARNING: Rename requires updating all downstream references.") | ||
| lines.push("") | ||
| } else if (data.changeType === "retype" && totalAffected > 0) { | ||
| lines.push("CAUTION: Type change may cause implicit casts or failures in downstream models.") | ||
| lines.push("") | ||
| } | ||
|
|
||
| if (data.direct.length > 0) { | ||
| lines.push(`Direct Dependents (${data.direct.length})`) | ||
| lines.push("".padEnd(40, "-")) | ||
| for (const d of data.direct) { | ||
| const mat = d.materialized ? ` [${d.materialized}]` : "" | ||
| lines.push(` ${d.name}${mat}`) | ||
| } | ||
| lines.push("") | ||
| } | ||
|
|
||
| if (data.transitive.length > 0) { | ||
| lines.push(`Transitive Dependents (${data.transitive.length})`) | ||
| lines.push("".padEnd(40, "-")) | ||
| for (const d of data.transitive) { | ||
| const mat = d.materialized ? ` [${d.materialized}]` : "" | ||
| const path = d.path.join(" \u2192 ") | ||
| lines.push(` ${d.name}${mat} (via: ${path})`) | ||
| } | ||
| lines.push("") | ||
| } | ||
|
|
||
| if (data.column && data.columnImpact.length > 0) { | ||
| lines.push(`Affected Output Columns (${data.columnImpact.length})`) | ||
| lines.push("".padEnd(40, "-")) | ||
| for (const col of data.columnImpact) { | ||
| lines.push(` ${col}`) | ||
| } | ||
| lines.push("") | ||
| } | ||
|
|
||
| if (data.affectedTestCount > 0) { | ||
| lines.push(`Tests in project: ${data.affectedTestCount}`) | ||
| lines.push("".padEnd(40, "-")) | ||
| lines.push(` Run \`dbt test\` to verify all ${data.affectedTestCount} tests still pass after this change.`) | ||
| lines.push("") | ||
| } | ||
|
|
||
| if (totalAffected === 0) { | ||
| lines.push("No downstream models depend on this. Change is safe to make.") | ||
| } | ||
|
|
||
| if (totalAffected > 0) { | ||
| lines.push("Recommended Actions") | ||
| lines.push("".padEnd(40, "-")) | ||
| if (data.changeType === "remove") { | ||
| lines.push("1. Update all downstream models to remove references") | ||
| lines.push("2. Run `dbt test` to verify no broken references") | ||
| lines.push("3. Consider deprecation period before removal") | ||
| } else if (data.changeType === "rename") { | ||
| lines.push("1. Update all downstream SQL references to new name") | ||
| lines.push("2. Run `dbt compile` to verify all models compile") | ||
| lines.push("3. Run `dbt test` to verify correctness") | ||
| } else if (data.changeType === "retype") { | ||
| lines.push("1. Check downstream models for implicit type casts") | ||
| lines.push("2. Verify aggregations and joins still work correctly") | ||
| lines.push("3. Run `dbt test` with data validation") | ||
| } else { | ||
| lines.push("1. Review downstream models for compatibility") | ||
| lines.push("2. Run `dbt compile` and `dbt test`") | ||
| } | ||
| } | ||
|
|
||
| return lines.join("\n") | ||
| } |
There was a problem hiding this comment.
Test-local function copies can drift from production behavior.
At Line 3-Line 5, this file explicitly tests copied implementations rather than the real functions in src/altimate/tools/impact-analysis.ts. That can let source regressions pass unnoticed if this copy is not updated in lockstep.
Suggested direction
-// Copy of findDownstream and formatImpactReport from
-// src/altimate/tools/impact-analysis.ts (not exported, tested standalone)
-// ...local implementations...
+// Import test hooks from source to ensure tests execute production logic.
+import { __test__ } from "../../../src/altimate/tools/impact-analysis"
+const { findDownstream, formatImpactReport } = __test__And in src/altimate/tools/impact-analysis.ts:
// Keep internal APIs private in app code, but expose test hooks intentionally.
export const __test__ = {
findDownstream,
formatImpactReport,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/tools/impact-analysis.test.ts` around lines 3
- 136, The test is using copied implementations of findDownstream and
formatImpactReport which can drift from production; modify the production module
to export a test-only hook object (e.g., add export const __test__ = {
findDownstream, formatImpactReport }) and update the test to import these
functions from that module instead of keeping the duplicated implementations in
the test file; then remove the duplicated findDownstream and formatImpactReport
code from the test so tests exercise the real production implementations.
Comment on lines
+91
to
+94
| test("truncates to 64 characters", () => { | ||
| const long = "a".repeat(100) | ||
| expect(slugify(long).length).toBe(64) | ||
| }) |
There was a problem hiding this comment.
Truncation test validates behavior that doesn't exist in production.
This test asserts 64-character truncation, but the production slugify in datamate.ts does not include .slice(0, 64). This test passes against the local copy but does not reflect actual production behavior.
If truncation is intentionally absent from production, remove this test. If truncation is required, update the production code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/tools/training-import.test.ts` around lines
91 - 94, The test asserts that slugify truncates to 64 chars but production
slugify (in datamate.ts) doesn't; either remove the test in
training-import.test.ts or modify the slugify function to enforce
truncation—update datamate.ts's slugify to return the normalized slug.slice(0,
64) (and keep the test), or if truncation is not desired, delete the "truncates
to 64 characters" test; locate symbols slugify and datamate.ts to apply the
change and ensure tests reflect the chosen behavior.
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.
What does this PR do?
Adds 34 unit tests for two newly-added tools (
impact-analysis,training-import) that shipped in commit edd58cd with zero test coverage. Both tools contain non-trivial pure logic (graph traversal, markdown parsing) that is critical to user-facing output.1.
findDownstream+formatImpactReport—src/altimate/tools/impact-analysis.ts(15 new tests)The
impact_analysistool helps users understand the blast radius of dbt model/column changes before making them. The corefindDownstreamfunction performs DAG traversal across dbt manifests, andformatImpactReportgenerates the user-facing impact report. Zero tests existed. New coverage includes:project.stg_orders→stg_orders)view,table,incremental)2.
parseMarkdownSections+slugify—src/altimate/tools/training-import.ts(19 new tests)The
training_importtool enables bulk-loading team standards from markdown documents into the training system.parseMarkdownSectionsis a state-machine parser that splits markdown by H2 headings and propagates H1 context.slugifynormalizes heading text into storage-safe keys. Zero tests existed. New coverage includes:v2.1→v21)Note: Both
findDownstream/formatImpactReportandparseMarkdownSections/slugifyare module-private (not exported). Tests use the established function-copy pattern (same astest/memory/prompt.test.ts) to test the pure logic in isolation.Type of change
Issue for this PR
N/A — proactive test coverage for newly shipped tools
How did you verify your code works?
All tests were reviewed by a critic agent and revised based on feedback (self-reference test renamed to behavior-documentation-only, percentage edge case uses NaN/Infinity guards, slugify tests use realistic inputs, unicode test covers multi-byte characters).
Checklist
https://claude.ai/code/session_011NpWhe2sbo52QP8Af9dA34
Summary by CodeRabbit