test: registry env-var loading + dbt lineage helpers#495
Closed
anandgupta42 wants to merge 1 commit intomainfrom
Closed
test: registry env-var loading + dbt lineage helpers#495anandgupta42 wants to merge 1 commit intomainfrom
anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Cover two previously untested code paths: - ConnectionRegistry loadFromEnv() parsing for CI/CD environments - dbtLineage() model lookup, dialect detection, and error handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> https://claude.ai/code/session_011ZpwXLNnjySge6t7CBbcrW
![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.
📝 WalkthroughWalkthroughTwo new comprehensive test suites were added to verify existing functionality: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
🧹 Nitpick comments (1)
packages/opencode/test/altimate/dbt-lineage-helpers.test.ts (1)
154-202: Dialect tests are too weak to validate dialect selection logic.These cases only assert
model_name, sodetectDialect/ explicit override regressions could still pass. Please assert observable dialect-dependent behavior (or mock the lineage call and assert the dialect argument).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/dbt-lineage-helpers.test.ts` around lines 154 - 202, The tests currently only check result.model_name and thus don’t validate dialect selection; update the tests around dbtLineage (and detectDialect if accessible) to assert observable, dialect-dependent behavior or to spy/mock the internal call so the chosen dialect is verified: for example, in the "detects dialect from manifest metadata.adapter_type" test mock or spy the function detectDialect (or the internal function that constructs the SQL parser) and assert it was called with "bigquery", and in the "explicit dialect param overrides auto-detection" and "defaults to snowflake when adapter_type is missing" tests either assert a dialect-specific parsing outcome (e.g., BigQuery vs Postgres identifier handling) or verify the dialect argument passed into the lineage/parser call when invoking dbtLineage.
🤖 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-lineage-helpers.test.ts`:
- Around line 269-273: The test currently passes a fixed
"/tmp/definitely-not-a-manifest.json" to dbtLineage; replace that with a
guaranteed-missing path created inside a temporary directory (use fs.mkdtempSync
/ fs.promises.mkdtemp or os.tmpdir to build a unique temp dir and path, e.g.,
path.join(tempDir, "nonexistent-manifest.json")) and do not create the file
before calling dbtLineage({ manifest_path: <thatPath>, model: "orders" }) so the
test cannot collide with existing files on the host.
- Around line 13-40: Replace the manual temp-dir lifecycle (tmpDirs array,
makeTmpDir(), writeManifest(), and afterEach cleanup) with the shared tmpdir()
test fixture used via "await using". Specifically, remove tmpDirs, mkdtempSync,
rmSync and afterEach logic, and in tests call "await using dir = tmpdir()" to
obtain a directory; then use writeManifest(dir, ...) (or adapt writeManifest to
accept the fixture dir) to write manifest.json so cleanup and realpath
guarantees are handled by the fixture automatically. Ensure references to
makeTmpDir() are updated to use the fixture variable and that writeManifest
still writes to join(dir, "manifest.json").
In `@packages/opencode/test/altimate/registry-env-loading.test.ts`:
- Around line 107-117: The test in registry-env-loading.test.ts assumes load()
returns exactly three warehouses but Registry.load() merges env vars with file
configs, making exact count brittle; modify the test that calls Registry.load()
so it asserts that the env-defined connections exist rather than asserting total
length—e.g., after calling Registry.list().warehouses, extract the types and
assert that the set includes "duckdb", "postgres", and "snowflake" (or assert
that the filtered array for those types has length 3), keeping the checks
focused on the env entries created by setEnvVar rather than exact total
warehouse count.
- Around line 23-34: The cleanup currently unconditionally deletes keys set by
setEnvVar, which can remove pre-existing environment variables; change setEnvVar
and cleanEnvVars so setEnvVar records the previous value (e.g., store a map
previousEnv: Record<string, string | undefined> keyed by `${ENV_PREFIX}${name}`)
and envVarsToClean holds the keys; then have cleanEnvVars iterate those keys and
if previousEnv[key] is undefined delete process.env[key] else restore
process.env[key] = previousEnv[key]; update references to envVarsToClean and
ensure previousEnv is reset after cleanup.
---
Nitpick comments:
In `@packages/opencode/test/altimate/dbt-lineage-helpers.test.ts`:
- Around line 154-202: The tests currently only check result.model_name and thus
don’t validate dialect selection; update the tests around dbtLineage (and
detectDialect if accessible) to assert observable, dialect-dependent behavior or
to spy/mock the internal call so the chosen dialect is verified: for example, in
the "detects dialect from manifest metadata.adapter_type" test mock or spy the
function detectDialect (or the internal function that constructs the SQL parser)
and assert it was called with "bigquery", and in the "explicit dialect param
overrides auto-detection" and "defaults to snowflake when adapter_type is
missing" tests either assert a dialect-specific parsing outcome (e.g., BigQuery
vs Postgres identifier handling) or verify the dialect argument passed into the
lineage/parser call when invoking dbtLineage.
🪄 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: efac7734-4cce-4f20-ba56-28b69eb958f7
📒 Files selected for processing (2)
packages/opencode/test/altimate/dbt-lineage-helpers.test.tspackages/opencode/test/altimate/registry-env-loading.test.ts
Comment on lines
+13
to
+40
| import { writeFileSync, mkdtempSync, rmSync } from "fs" | ||
| import { tmpdir } from "os" | ||
| import { join } from "path" | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Helpers | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| let tmpDirs: string[] = [] | ||
|
|
||
| function makeTmpDir(): string { | ||
| const dir = mkdtempSync(join(tmpdir(), "dbt-lineage-test-")) | ||
| tmpDirs.push(dir) | ||
| return dir | ||
| } | ||
|
|
||
| function writeManifest(dir: string, manifest: Record<string, any>): string { | ||
| const manifestPath = join(dir, "manifest.json") | ||
| writeFileSync(manifestPath, JSON.stringify(manifest)) | ||
| return manifestPath | ||
| } | ||
|
|
||
| afterEach(() => { | ||
| for (const dir of tmpDirs) { | ||
| rmSync(dir, { recursive: true, force: true }) | ||
| } | ||
| tmpDirs = [] | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the shared tmpdir() fixture with await using instead of manual temp-dir lifecycle.
This manual mkdtempSync + array tracking + afterEach cleanup pattern bypasses the required test fixture conventions and realpath guarantees.
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-lineage-helpers.test.ts` around lines 13
- 40, Replace the manual temp-dir lifecycle (tmpDirs array, makeTmpDir(),
writeManifest(), and afterEach cleanup) with the shared tmpdir() test fixture
used via "await using". Specifically, remove tmpDirs, mkdtempSync, rmSync and
afterEach logic, and in tests call "await using dir = tmpdir()" to obtain a
directory; then use writeManifest(dir, ...) (or adapt writeManifest to accept
the fixture dir) to write manifest.json so cleanup and realpath guarantees are
handled by the fixture automatically. Ensure references to makeTmpDir() are
updated to use the fixture variable and that writeManifest still writes to
join(dir, "manifest.json").
Comment on lines
+269
to
+273
| test("returns low confidence for non-existent manifest", () => { | ||
| const result = dbtLineage({ | ||
| manifest_path: "/tmp/definitely-not-a-manifest.json", | ||
| model: "orders", | ||
| }) |
There was a problem hiding this comment.
Use a guaranteed-missing path instead of a hardcoded /tmp filename.
A fixed path can collide on some hosts. Build a unique path in a test temp dir and intentionally do not create the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/dbt-lineage-helpers.test.ts` around lines 269
- 273, The test currently passes a fixed "/tmp/definitely-not-a-manifest.json"
to dbtLineage; replace that with a guaranteed-missing path created inside a
temporary directory (use fs.mkdtempSync / fs.promises.mkdtemp or os.tmpdir to
build a unique temp dir and path, e.g., path.join(tempDir,
"nonexistent-manifest.json")) and do not create the file before calling
dbtLineage({ manifest_path: <thatPath>, model: "orders" }) so the test cannot
collide with existing files on the host.
Comment on lines
+23
to
+34
| function setEnvVar(name: string, value: string): void { | ||
| const key = `${ENV_PREFIX}${name}` | ||
| process.env[key] = value | ||
| envVarsToClean.push(key) | ||
| } | ||
|
|
||
| function cleanEnvVars(): void { | ||
| for (const key of envVarsToClean) { | ||
| delete process.env[key] | ||
| } | ||
| envVarsToClean.length = 0 | ||
| } |
There was a problem hiding this comment.
Preserve pre-existing env values during cleanup.
cleanEnvVars() unconditionally deletes keys, so if a key already existed before the test run, this suite permanently removes it for downstream tests.
Suggested fix
const ENV_PREFIX = "ALTIMATE_CODE_CONN_"
const envVarsToClean: string[] = []
+const originalEnv = new Map<string, string | undefined>()
function setEnvVar(name: string, value: string): void {
const key = `${ENV_PREFIX}${name}`
+ if (!originalEnv.has(key)) {
+ originalEnv.set(key, process.env[key])
+ }
process.env[key] = value
- envVarsToClean.push(key)
+ if (!envVarsToClean.includes(key)) envVarsToClean.push(key)
}
function cleanEnvVars(): void {
for (const key of envVarsToClean) {
- delete process.env[key]
+ const prev = originalEnv.get(key)
+ if (prev === undefined) delete process.env[key]
+ else process.env[key] = prev
+ originalEnv.delete(key)
}
envVarsToClean.length = 0
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/registry-env-loading.test.ts` around lines 23
- 34, The cleanup currently unconditionally deletes keys set by setEnvVar, which
can remove pre-existing environment variables; change setEnvVar and cleanEnvVars
so setEnvVar records the previous value (e.g., store a map previousEnv:
Record<string, string | undefined> keyed by `${ENV_PREFIX}${name}`) and
envVarsToClean holds the keys; then have cleanEnvVars iterate those keys and if
previousEnv[key] is undefined delete process.env[key] else restore
process.env[key] = previousEnv[key]; update references to envVarsToClean and
ensure previousEnv is reset after cleanup.
Comment on lines
+107
to
+117
| test("loads multiple connections from env vars", () => { | ||
| setEnvVar("PG", JSON.stringify({ type: "postgres", host: "pg.local" })) | ||
| setEnvVar("SF", JSON.stringify({ type: "snowflake", account: "xyz" })) | ||
| setEnvVar("DDB", JSON.stringify({ type: "duckdb", path: ":memory:" })) | ||
|
|
||
| Registry.load() | ||
|
|
||
| const warehouses = Registry.list().warehouses | ||
| expect(warehouses).toHaveLength(3) | ||
| expect(warehouses.map((w) => w.type).sort()).toEqual(["duckdb", "postgres", "snowflake"]) | ||
| }) |
There was a problem hiding this comment.
Avoid exact warehouse-count assertions in merged-source loading tests.
This test assumes only env-defined connections exist. Since load() also includes file configs, asserting exactly 3 can be flaky; assert expected env entries instead.
Suggested fix
const warehouses = Registry.list().warehouses
- expect(warehouses).toHaveLength(3)
- expect(warehouses.map((w) => w.type).sort()).toEqual(["duckdb", "postgres", "snowflake"])
+ const byName = new Map(warehouses.map((w) => [w.name, w]))
+ expect(byName.get("pg")?.type).toBe("postgres")
+ expect(byName.get("sf")?.type).toBe("snowflake")
+ expect(byName.get("ddb")?.type).toBe("duckdb")📝 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("loads multiple connections from env vars", () => { | |
| setEnvVar("PG", JSON.stringify({ type: "postgres", host: "pg.local" })) | |
| setEnvVar("SF", JSON.stringify({ type: "snowflake", account: "xyz" })) | |
| setEnvVar("DDB", JSON.stringify({ type: "duckdb", path: ":memory:" })) | |
| Registry.load() | |
| const warehouses = Registry.list().warehouses | |
| expect(warehouses).toHaveLength(3) | |
| expect(warehouses.map((w) => w.type).sort()).toEqual(["duckdb", "postgres", "snowflake"]) | |
| }) | |
| test("loads multiple connections from env vars", () => { | |
| setEnvVar("PG", JSON.stringify({ type: "postgres", host: "pg.local" })) | |
| setEnvVar("SF", JSON.stringify({ type: "snowflake", account: "xyz" })) | |
| setEnvVar("DDB", JSON.stringify({ type: "duckdb", path: ":memory:" })) | |
| Registry.load() | |
| const warehouses = Registry.list().warehouses | |
| const byName = new Map(warehouses.map((w) => [w.name, w])) | |
| expect(byName.get("pg")?.type).toBe("postgres") | |
| expect(byName.get("sf")?.type).toBe("snowflake") | |
| expect(byName.get("ddb")?.type).toBe("duckdb") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/registry-env-loading.test.ts` around lines
107 - 117, The test in registry-env-loading.test.ts assumes load() returns
exactly three warehouses but Registry.load() merges env vars with file configs,
making exact count brittle; modify the test that calls Registry.load() so it
asserts that the env-defined connections exist rather than asserting total
length—e.g., after calling Registry.list().warehouses, extract the types and
assert that the set includes "duckdb", "postgres", and "snowflake" (or assert
that the filtered array for those types has length 3), keeping the checks
focused on the env entries created by setEnvVar rather than exact total
warehouse count.
anandgupta42
added a commit
that referenced
this pull request
Mar 27, 2026
Consolidates tests from PRs #440, #441, #479, #483, #484, #485, #490, #491, #492, #495, #496. Deduplicates overlapping MongoDB normalize tests (3 PRs → 1), `keybind` tests (2 PRs → 1), and `connections.test.ts` additions (4 PRs → 1 merged file). New test files: - `sql-analyze-tool.test.ts` — `SqlAnalyzeTool` formatting (6 tests) - `sql-analyze-format.test.ts` — `sql.analyze` success semantics + dispatcher (5 tests) - `builtin-commands.test.ts` — altimate builtin command registration (10 tests) - `hints-discover-mcps.test.ts` — `Command.hints()` + discover-and-add-mcps (11 tests) - `fingerprint-detect.test.ts` — file-based project detection rules (11 tests) - `dbt-lineage-helpers.test.ts` — dbt lineage helper functions (13 tests) - `registry-env-loading.test.ts` — `ALTIMATE_CODE_CONN_*` env var loading (8 tests) - `warehouse-telemetry.test.ts` — MongoDB auth detection telemetry (4 tests) - `bus-event.test.ts` — bus event registry payloads (5 tests) - `git.test.ts` — git utility functions (5 tests) - `keybind.test.ts` — keybind parsing, matching, `fromParsedKey` (22 tests) Merged into existing files: - `connections.test.ts` — `loadFromEnv` (5), `detectAuthMethod` (14), credential stripping (1), `containerToConfig` (1), dbt profiles edge cases (3) - `driver-normalize.test.ts` — MongoDB alias resolution (13 tests) Fixes: - `fixture.ts` — disable `commit.gpgsign` in test tmpdir to prevent CI failures - `hints-discover-mcps.test.ts` — corrected sort order expectation - `registry-env-loading.test.ts` — fixed flaky assertion that assumed isolated env Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
Contributor
Author
|
Consolidated into #498. Tests deduplicated and merged into a single PR. |
anandgupta42
added a commit
that referenced
this pull request
Mar 27, 2026
* test: consolidate 11 test PRs into single suite — 305 new tests Consolidates tests from PRs #440, #441, #479, #483, #484, #485, #490, #491, #492, #495, #496. Deduplicates overlapping MongoDB normalize tests (3 PRs → 1), `keybind` tests (2 PRs → 1), and `connections.test.ts` additions (4 PRs → 1 merged file). New test files: - `sql-analyze-tool.test.ts` — `SqlAnalyzeTool` formatting (6 tests) - `sql-analyze-format.test.ts` — `sql.analyze` success semantics + dispatcher (5 tests) - `builtin-commands.test.ts` — altimate builtin command registration (10 tests) - `hints-discover-mcps.test.ts` — `Command.hints()` + discover-and-add-mcps (11 tests) - `fingerprint-detect.test.ts` — file-based project detection rules (11 tests) - `dbt-lineage-helpers.test.ts` — dbt lineage helper functions (13 tests) - `registry-env-loading.test.ts` — `ALTIMATE_CODE_CONN_*` env var loading (8 tests) - `warehouse-telemetry.test.ts` — MongoDB auth detection telemetry (4 tests) - `bus-event.test.ts` — bus event registry payloads (5 tests) - `git.test.ts` — git utility functions (5 tests) - `keybind.test.ts` — keybind parsing, matching, `fromParsedKey` (22 tests) Merged into existing files: - `connections.test.ts` — `loadFromEnv` (5), `detectAuthMethod` (14), credential stripping (1), `containerToConfig` (1), dbt profiles edge cases (3) - `driver-normalize.test.ts` — MongoDB alias resolution (13 tests) Fixes: - `fixture.ts` — disable `commit.gpgsign` in test tmpdir to prevent CI failures - `hints-discover-mcps.test.ts` — corrected sort order expectation - `registry-env-loading.test.ts` — fixed flaky assertion that assumed isolated env Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address CodeRabbit review comments - `bus-event.test.ts`: move `BusEvent.define` into tests instead of module scope - `builtin-commands.test.ts`: fix misleading "lexicographic" → "numeric" sort label - `git.test.ts`: use `tmpdir({ git: true })` fixture instead of manual `git init` - `registry-env-loading.test.ts`: remove unused `fs`, `os`, `path` imports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
What does this PR do?
Adds 21 new tests covering two previously untested code paths that affect real users.
1.
ConnectionRegistry.loadFromEnv()—src/altimate/native/connections/registry.ts(8 new tests)CI/CD users configure warehouse connections via
ALTIMATE_CODE_CONN_*environment variables. The entireloadFromEnv()pipeline — prefix stripping, JSON parsing, type validation, and name lowercasing — had zero test coverage. Existing tests inconnections.test.tsusesetConfigs()which bypasses the loading pipeline entirely.New coverage includes:
MY_PROD_DB→my_prod_db)typefieldload()replacing priorsetConfigs()state entirelylist()reflecting env-loaded connections (with.find()for host-filesystem safety)2.
dbtLineage()+ helper functions —src/altimate/native/dbt/lineage.ts(13 new tests)The
dbtLineage()function and its helpers (findModel,detectDialect,buildSchemaContext) power column-level lineage analysis. All had zero test coverage —schema-finops-dbt.test.tsonly coversparseManifest(). A bug infindModelcauses lineage to silently return "no lineage available" to users.New coverage includes:
findModel)metadata.adapter_typewith snowflake default fallbackType of change
Issue for this PR
N/A — proactive test coverage from automated test discovery
How did you verify your code works?
Tests were reviewed by a critic agent that validated each test for:
Critic rejected 2 tests (MongoDB env var = redundant, empty nodes = duplicate coverage) and revised 3 others (misleading comments, fragile index-based assertions). All feedback was incorporated.
Checklist
https://claude.ai/code/session_011ZpwXLNnjySge6t7CBbcrW
Summary by CodeRabbit