test: dbt helpers — direct unit tests for shared manifest utilities#694
Open
anandgupta42 wants to merge 1 commit intomainfrom
Open
test: dbt helpers — direct unit tests for shared manifest utilities#694anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
The 7 exported functions in src/altimate/native/dbt/helpers.ts power dbt-lineage, dbt-unit-test-gen, and dbt-manifest handlers but had zero direct unit tests. A silent regression in findModel or detectDialect cascades across multiple dbt tools. 32 new tests cover model lookup, dialect mapping, schema context building, column extraction, and manifest caching/invalidation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_01G3L4LeyQGJ2ox4ssaqb3Kb
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughA new test suite has been added for dbt helper functions, covering unit tests for model lookup, dialect detection, schema context generation, column extraction, model name listing, and raw manifest loading with caching behavior validation. Changes
Suggested labels
Suggested reviewers
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
🤖 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/dbt-helpers.test.ts`:
- Around line 294-304: The test "invalidates cache when file content is
rewritten" is flaky because it assumes a rewrite always changes mtimeMs; update
the test to force a deterministic mtime change using fs.utimesSync (or
fs.utimes) after the second write, then call loadRawManifest again and assert
the returned object differs (or that second.v === 2) and that the cache identity
changed; locate the test by its title and the use of
loadRawManifest/manifestPath to implement the utimes bump and the explicit
identity assertion.
- Around line 252-258: Replace the manual temp-dir lifecycle that uses
fs.mkdtempSync and fs.rmSync (tmpDir variable set in beforeEach/afterEach) with
the provided tmpdir() helper from fixture/fixture.ts and the `await using`
pattern; locate the setup that defines tmpDir and the beforeEach/afterEach
blocks, remove mkdtempSync/rmSync, import tmpdir, and in each test scope acquire
the temp directory via `await using (const tmpDir = await tmpdir(...))` so
cleanup happens automatically when the using block exits.
🪄 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: f1df5cfb-9d94-4641-ac5a-0b383b5a2fc0
📒 Files selected for processing (1)
packages/opencode/test/altimate/dbt-helpers.test.ts
Comment on lines
+252
to
+258
| beforeEach(() => { | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "dbt-helpers-test-")) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| fs.rmSync(tmpDir, { recursive: true, force: true }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use tmpdir() + await using instead of manual temp-dir lifecycle.
Line 252–258 manually manages temp directories with mkdtempSync/rmSync. In this test path, repo rules require tmpdir() from fixture/fixture.ts with await using.
♻️ Suggested refactor pattern
import { describe, test, expect, beforeEach, afterEach } from "bun:test"
import * as fs from "fs"
import * as path from "path"
import * as os from "os"
+import { tmpdir } from "../fixture/fixture"
describe("loadRawManifest", () => {
- let tmpDir: string
-
- beforeEach(() => {
- tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "dbt-helpers-test-"))
- })
-
- afterEach(() => {
- fs.rmSync(tmpDir, { recursive: true, force: true })
- })
-
- test("returns null for non-existent file", () => {
- expect(loadRawManifest(path.join(tmpDir, "nonexistent.json"))).toBeNull()
+ test("returns null for non-existent file", async () => {
+ await using tmp = await tmpdir()
+ expect(loadRawManifest(path.join(tmp.path, "nonexistent.json"))).toBeNull()
})As per coding guidelines: "Use the tmpdir function from fixture/fixture.ts to create temporary directories for tests with automatic cleanup in test files" and "Always use await using syntax with tmpdir() for automatic cleanup when the variable goes out of scope".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/dbt-helpers.test.ts` around lines 252 - 258,
Replace the manual temp-dir lifecycle that uses fs.mkdtempSync and fs.rmSync
(tmpDir variable set in beforeEach/afterEach) with the provided tmpdir() helper
from fixture/fixture.ts and the `await using` pattern; locate the setup that
defines tmpDir and the beforeEach/afterEach blocks, remove mkdtempSync/rmSync,
import tmpdir, and in each test scope acquire the temp directory via `await
using (const tmpDir = await tmpdir(...))` so cleanup happens automatically when
the using block exits.
Comment on lines
+294
to
+304
| test("invalidates cache when file content is rewritten", () => { | ||
| const manifestPath = path.join(tmpDir, "updated.json") | ||
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 1 })) | ||
| const first = loadRawManifest(manifestPath) | ||
|
|
||
| // Rewrite — OS updates mtime to current time, which differs from | ||
| // the first write's mtime (millisecond-resolution on modern Linux). | ||
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 2 })) | ||
| const second = loadRawManifest(manifestPath) | ||
| expect(second.v).toBe(2) | ||
| }) |
There was a problem hiding this comment.
Make the cache-invalidation test deterministic for mtime-based caching.
Line 299–301 assumes rewrite always changes mtimeMs. On fast filesystems/writes, this can be flaky. Force an mtime bump and assert identity changed.
🛠️ Deterministic assertion update
test("invalidates cache when file content is rewritten", () => {
const manifestPath = path.join(tmpDir, "updated.json")
fs.writeFileSync(manifestPath, JSON.stringify({ v: 1 }))
const first = loadRawManifest(manifestPath)
- // Rewrite — OS updates mtime to current time, which differs from
- // the first write's mtime (millisecond-resolution on modern Linux).
fs.writeFileSync(manifestPath, JSON.stringify({ v: 2 }))
+ const bumped = new Date(Date.now() + 1000)
+ fs.utimesSync(manifestPath, bumped, bumped)
const second = loadRawManifest(manifestPath)
expect(second.v).toBe(2)
+ expect(second).not.toBe(first)
})📝 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
| test("invalidates cache when file content is rewritten", () => { | |
| const manifestPath = path.join(tmpDir, "updated.json") | |
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 1 })) | |
| const first = loadRawManifest(manifestPath) | |
| // Rewrite — OS updates mtime to current time, which differs from | |
| // the first write's mtime (millisecond-resolution on modern Linux). | |
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 2 })) | |
| const second = loadRawManifest(manifestPath) | |
| expect(second.v).toBe(2) | |
| }) | |
| test("invalidates cache when file content is rewritten", () => { | |
| const manifestPath = path.join(tmpDir, "updated.json") | |
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 1 })) | |
| const first = loadRawManifest(manifestPath) | |
| fs.writeFileSync(manifestPath, JSON.stringify({ v: 2 })) | |
| const bumped = new Date(Date.now() + 1000) | |
| fs.utimesSync(manifestPath, bumped, bumped) | |
| const second = loadRawManifest(manifestPath) | |
| expect(second.v).toBe(2) | |
| expect(second).not.toBe(first) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/dbt-helpers.test.ts` around lines 294 - 304,
The test "invalidates cache when file content is rewritten" is flaky because it
assumes a rewrite always changes mtimeMs; update the test to force a
deterministic mtime change using fs.utimesSync (or fs.utimes) after the second
write, then call loadRawManifest again and assert the returned object differs
(or that second.v === 2) and that the cache identity changed; locate the test by
its title and the use of loadRawManifest/manifestPath to implement the utimes
bump and the explicit identity assertion.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
2 participants
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
What does this PR do?
1.
findModel,getUniqueId,detectDialect,buildSchemaContext,extractColumns,listModelNames,loadRawManifest—src/altimate/native/dbt/helpers.ts(32 new tests)These 7 exported pure functions are the shared foundation for dbt-lineage, dbt-unit-test-gen, and dbt-manifest handlers. Zero direct unit tests existed — they were only tested indirectly through
dbtLineage()indbt-lineage-helpers.test.ts, which cannot isolate individual function regressions.A silent bug in any of these helpers cascades across multiple dbt tools:
findModelreturningnullfor a valid model → lineage and unit-test generation fail silentlydetectDialectreturning wrong dialect → altimate-core SQL analysis produces garbagebuildSchemaContextdropping upstream columns → dbt unit-test generation creates empty schemasloadRawManifestcache corruption → stale manifest data used across requestsSpecific scenarios covered:
findModel(6 tests): lookup by unique_id, lookup by name, source/test node rejection, empty nodes, duplicate model namesgetUniqueId(4 tests): direct key match, name-based scan, source node rejection, missing modeldetectDialect(5 tests): all 11 known adapter→dialect mappings, unmapped adapter passthrough, empty/null/missing metadata defaults to snowflakebuildSchemaContext(5 tests): alias-over-name precedence (withtoBeUndefined()guard), empty column skip, source resolution, empty/unresolvable upstream IDsextractColumns(4 tests): data_type extraction,typefield fallback, dict-key-as-name fallback, empty dictlistModelNames(2 tests): model-only filtering, empty nodesloadRawManifest(6 tests): missing file returns null, valid parse, invalid JSON throws, primitive (non-object) throws, path+mtime caching (reference equality), cache invalidation on rewriteType of change
Issue for this PR
N/A — proactive test coverage for shared dbt utilities with zero direct tests
How did you verify your code works?
Checklist
https://claude.ai/code/session_01G3L4LeyQGJ2ox4ssaqb3Kb
Summary by cubic
Add 32 direct unit tests for shared dbt helpers in
src/altimate/native/dbt/helpers.tsto catch regressions and improve reliability across dbt-lineage, dbt-unit-test-gen, and manifest handlers. Tests cover model lookup, unique_id resolution, dialect detection, schema context building, column extraction, model name listing, and manifest load/caching (including invalidation).Written for commit ccac8f4. Summary will update on new commits.
Summary by CodeRabbit