test: consolidate 5 test PRs — 139 tests, deduplicated, with bug fixes#709
Open
anandgupta42 wants to merge 2 commits intomainfrom
Open
test: consolidate 5 test PRs — 139 tests, deduplicated, with bug fixes#709anandgupta42 wants to merge 2 commits intomainfrom
anandgupta42 wants to merge 2 commits intomainfrom
Conversation
Consolidates PRs #694, #690, #689, #695, #620. Changes: - 4 files changed, ~577 lines of new test coverage - Deduplicated `dbt-helpers.test.ts`: merged #694 (superset) with unique tests from #690 (symlink caching, seed/test node exclusion, JSON array edge case, combined fallbacks) - Bug fixes: fixed `loadRawManifest` cache invalidation test (explicit `utimesSync` for filesystem mtime granularity) - Removed 2 broken tests from #695: `discoverExternalMcp` env-var interpolation tests assumed `${VAR}` resolution in `command` field, but `resolveServerEnvVars` only resolves `env` and `headers` fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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.
📝 WalkthroughWalkthroughAdds four new or updated Bun test suites under packages/opencode to validate dbt helper utilities, file status detection, MCP discovery comments, and ClickHouse environment detection across multiple input forms. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
This was referenced Apr 15, 2026
![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 (2)
packages/opencode/test/tool/project-scan.test.ts (1)
550-560: Also assert masking forclickhouse+http/clickhouse+httpsvariants.Lines 551-560 verify detection/type, but not secret masking for these scheme variants. Add the same
connection_stringmasking check used in the other ClickHouse tests.Suggested test assertion update
test("detects ClickHouse via DATABASE_URL with clickhouse+http and clickhouse+https schemes", async () => { for (const scheme of ["clickhouse+http", "clickhouse+https"]) { clearWarehouseEnvVars() process.env.DATABASE_URL = `${scheme}://default:pass@clickhouse.example.com:8443/analytics` const result = await detectEnvVars() const ch = result.find((r) => r.type === "clickhouse") expect(ch).toBeDefined() expect(ch!.signal).toBe("DATABASE_URL") expect(ch!.type).toBe("clickhouse") + expect(ch!.config.connection_string).toBe("***") } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/project-scan.test.ts` around lines 550 - 560, The ClickHouse scheme-variant test ("detects ClickHouse via DATABASE_URL with clickhouse+http and clickhouse+https schemes") currently asserts detection and type but not that secrets are masked; update the test that iterates over ["clickhouse+http","clickhouse+https"] to also assert the same masked connection_string check used in the other ClickHouse tests: after locating ch via result.find(r => r.type === "clickhouse"), add an assertion that ch!.metadata.connection_string equals the expected masked value (the same masking pattern used elsewhere), ensuring you keep calls to clearWarehouseEnvVars() and detectEnvVars() and reuse the existing variable names (scheme, result, ch) so the test structure remains consistent.packages/opencode/test/file/status.test.ts (1)
114-118: Prevent a vacuous pass in the relative-path test.The loop only checks
isAbsolutefor existing entries; ifchangedis unexpectedly empty, this test still passes. Add a presence assertion first.✅ Tighten assertion
const changed = await File.status() + expect(changed.length).toBeGreaterThan(0) + expect(changed.some((f) => f.path === "src/app.ts")).toBe(true) for (const file of changed) { // All paths should be relative, not absolute expect(path.isAbsolute(file.path)).toBe(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/file/status.test.ts` around lines 114 - 118, The test for relative paths can vacuously pass if File.status() returns an empty array; before iterating over changed, add a presence assertion (e.g., assert changed is non-empty or check changed.length > 0 via expect) so the test fails when no entries are returned, then keep the existing loop that uses path.isAbsolute(file.path) to verify each path is not absolute.
🤖 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 355-358: The test currently uses toEqual which only checks deep
equality; instead assert identity to ensure symlinks are resolved and the same
cached object is returned — replace the deep-equality check on the two manifest
loads (viaReal and viaSym) with a reference-equality assertion
(expect(viaSym).toBe(viaReal)) while keeping the existing check for nodes.sym to
be true; locate this in the test around loadRawManifest usages to tighten the
assertion.
- Around line 280-288: Replace the manual temp-dir lifecycle (the tmpDir
variable plus beforeEach using fs.mkdtempSync and afterEach fs.rmSync) with the
shared tmpdir fixture: remove tmpDir, beforeEach and afterEach blocks and
instead call the tmpdir() fixture via "await using tmp = tmpdir()" at the start
of each test (or in a beforeEach helper) so the fixture provides a realpath-safe
temporary directory and handles automatic cleanup; update any references to
tmpDir to use tmp.path (or the fixture's API) and ensure imports include tmpdir
from the test fixture module.
---
Nitpick comments:
In `@packages/opencode/test/file/status.test.ts`:
- Around line 114-118: The test for relative paths can vacuously pass if
File.status() returns an empty array; before iterating over changed, add a
presence assertion (e.g., assert changed is non-empty or check changed.length >
0 via expect) so the test fails when no entries are returned, then keep the
existing loop that uses path.isAbsolute(file.path) to verify each path is not
absolute.
In `@packages/opencode/test/tool/project-scan.test.ts`:
- Around line 550-560: The ClickHouse scheme-variant test ("detects ClickHouse
via DATABASE_URL with clickhouse+http and clickhouse+https schemes") currently
asserts detection and type but not that secrets are masked; update the test that
iterates over ["clickhouse+http","clickhouse+https"] to also assert the same
masked connection_string check used in the other ClickHouse tests: after
locating ch via result.find(r => r.type === "clickhouse"), add an assertion that
ch!.metadata.connection_string equals the expected masked value (the same
masking pattern used elsewhere), ensuring you keep calls to
clearWarehouseEnvVars() and detectEnvVars() and reuse the existing variable
names (scheme, result, ch) so the test structure remains consistent.
🪄 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: 43b7998a-43ba-4ac0-a96a-f25467777884
📒 Files selected for processing (4)
packages/opencode/test/altimate/dbt-helpers.test.tspackages/opencode/test/file/status.test.tspackages/opencode/test/mcp/discover.test.tspackages/opencode/test/tool/project-scan.test.ts
Comment on lines
+280
to
+288
| let tmpDir: string | ||
|
|
||
| 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.
Use tmpdir() + await using instead of manual mkdtempSync/rmSync.
This test file is under packages/opencode/test/**/*.test.ts, so manual temp-dir lifecycle should be replaced with the shared fixture pattern for automatic cleanup and realpath-safe temp paths.
♻️ Suggested 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..." and "Always use await using syntax with tmpdir()...".
🤖 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 280 - 288,
Replace the manual temp-dir lifecycle (the tmpDir variable plus beforeEach using
fs.mkdtempSync and afterEach fs.rmSync) with the shared tmpdir fixture: remove
tmpDir, beforeEach and afterEach blocks and instead call the tmpdir() fixture
via "await using tmp = tmpdir()" at the start of each test (or in a beforeEach
helper) so the fixture provides a realpath-safe temporary directory and handles
automatic cleanup; update any references to tmpDir to use tmp.path (or the
fixture's API) and ensure imports include tmpdir from the test fixture module.
Comment on lines
+62
to
+66
| // Deleted files appear in diff --numstat (as "modified") AND diff --diff-filter=D (as "deleted") | ||
| const deleted = changed.find((f) => f.path === "to-delete.txt" && f.status === "deleted") | ||
| expect(deleted).toBeDefined() | ||
| expect(deleted!.status).toBe("deleted") | ||
| }, |
There was a problem hiding this comment.
Deleted-file assertion conflicts with current File.status() behavior.
Line 62 assumes a separate deleted-file pass (--diff-filter=D), but File.status() (in packages/opencode/src/file/index.ts) currently classifies diff entries as "modified" from --numstat and doesn’t emit "deleted" in the shown implementation. This test will fail unless runtime code is updated too.
💡 Suggested adjustment (if keeping current runtime behavior)
- // Deleted files appear in diff --numstat (as "modified") AND diff --diff-filter=D (as "deleted")
- const deleted = changed.find((f) => f.path === "to-delete.txt" && f.status === "deleted")
- expect(deleted).toBeDefined()
- expect(deleted!.status).toBe("deleted")
+ // Current File.status() reports deletions via --numstat as "modified"
+ const deleted = changed.find((f) => f.path === "to-delete.txt")
+ expect(deleted).toBeDefined()
+ expect(deleted!.status).toBe("modified")
+ expect(deleted!.removed).toBeGreaterThan(0)📝 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
| // Deleted files appear in diff --numstat (as "modified") AND diff --diff-filter=D (as "deleted") | |
| const deleted = changed.find((f) => f.path === "to-delete.txt" && f.status === "deleted") | |
| expect(deleted).toBeDefined() | |
| expect(deleted!.status).toBe("deleted") | |
| }, | |
| // Current File.status() reports deletions via --numstat as "modified" | |
| const deleted = changed.find((f) => f.path === "to-delete.txt") | |
| expect(deleted).toBeDefined() | |
| expect(deleted!.status).toBe("modified") | |
| expect(deleted!.removed).toBeGreaterThan(0) | |
| }, |
- `dbt-helpers.test.ts`: use `toBe` (reference equality) for symlink cache test - `status.test.ts`: add presence assertion before relative-path loop - `project-scan.test.ts`: add `connection_string` masking assertion for `clickhouse+http/https` schemes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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
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?
Consolidates 5 open test PRs (#694, #690, #689, #695, #620) into a single reviewed PR with deduplication, bug fixes, and flaky test removal.
Test coverage added (139 tests across 4 files):
dbt-helpers.test.ts(38 tests) —findModel,getUniqueId,detectDialect,buildSchemaContext,extractColumns,listModelNames,loadRawManifestwith cache invalidation, symlink resolution, JSON array edge casestatus.test.ts(6 tests) —File.status()git changed-file detection: modified, untracked, deleted, binary, clean tree, path normalizationproject-scan.test.ts(+4 tests) — ClickHousedetectEnvVarscoverage:CLICKHOUSE_HOST,CLICKHOUSE_URL,DATABASE_URLwithclickhouse://andclickhouse+http://schemesdiscover.test.ts(+0 tests, comment only) — Removed 2 broken env-var interpolation tests from test: mcp discover — env-var interpolation in external configs #695 that assumed${VAR}resolution incommandfield, butresolveServerEnvVarsonly resolvesenvandheadersfieldsDeduplication:
dbt-helpers.test.ts. Merged test: dbt helpers — direct unit tests for shared manifest utilities #694 (32 tests, superset) with 6 unique tests from test: dbt helpers — loadRawManifest, getUniqueId, extractColumns, listModelNames #690 (symlink caching, seed/test node exclusion, non-model resource_type by unique_id, JSON array typeof guard, combined name+type fallbacks, empty nodes)Bug fixes:
loadRawManifestcache invalidation test: added explicitutimesSyncto bump mtime, avoiding failures on filesystems with 1-second mtime granularityType of change
Issue for this PR
Closes #708
How did you verify your code works?
Checklist
Summary by cubic
Consolidates five test PRs into one, adding 139 deduplicated unit tests and removing flaky/invalid cases. Expands coverage for dbt helpers, git changed-file detection, and ClickHouse env var detection; addresses review feedback to harden assertions.
Refactors
File.status()(modified/added/deleted/binary/clean/relative paths); ClickHouse detection viaCLICKHOUSE_*andDATABASE_URLschemes. Removed invalid discover command interpolation tests.toBefor symlink cache reference equality; presence check before relative-path loop;connection_stringmasking forclickhouse+http/https.Bug Fixes
loadRawManifestcache invalidation test by bumping mtime withfs.utimesSync.Written for commit a0c1918. Summary will update on new commits.
Summary by CodeRabbit