test: consolidate 11 test PRs — 305 new tests, deduplicated#498
Open
anandgupta42 wants to merge 2 commits intomainfrom
Open
test: consolidate 11 test PRs — 305 new tests, deduplicated#498anandgupta42 wants to merge 2 commits intomainfrom
anandgupta42 wants to merge 2 commits intomainfrom
Conversation
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>
![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.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 29254673 | Triggered | Generic Password | e1f0321 | packages/opencode/test/altimate/connections.test.ts | View secret |
| 29254674 | Triggered | Generic Password | e1f0321 | packages/opencode/test/altimate/connections.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
This was referenced Mar 27, 2026
This was referenced Mar 27, 2026
📝 WalkthroughWalkthroughThis PR adds many new and expanded unit tests across the repo (connections/registry, dbt lineage, SQL analysis, commands, utilities, event bus, fingerprinting) and a minor fixture change disabling git commit signing for temp repos. Tests exercise parsing, normalization, detection, formatting, and error paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
This was referenced Mar 27, 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 (5)
packages/opencode/test/util/keybind.test.ts (1)
214-232: Consider using the actualParsedKeytype instead ofas any.The tests use
as anyto cast the parsed key objects. IfKeybindexports or exposes theParsedKeytype from its dependency, using it would provide better type safety and make the test data more self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/util/keybind.test.ts` around lines 214 - 232, The tests currently cast parsed key objects with "as any", losing type safety; update the tests to import and use the actual ParsedKey type (or the exported type alias) and cast the test objects to ParsedKey when calling Keybind.fromParsedKey; locate the test file references to Keybind.fromParsedKey and replace "as any" with the imported ParsedKey type (e.g., import { ParsedKey } from the module that exports Keybind) so the literal objects conform to the ParsedKey shape and the compiler validates the test fixtures.packages/opencode/test/altimate/fingerprint-detect.test.ts (1)
12-63: Optional: consider table-driven cases to reduce repetition.The marker-detection tests follow the same setup/write/detect/assert pattern; parameterizing them would make future additions cheaper and keep this file shorter.
♻️ Example refactor shape
+const cases = [ + { name: "dbt_project.yml", files: [["dbt_project.yml", "name: my_project\n"]], tags: ["dbt", "data-engineering"] }, + { name: "profiles.yml snowflake", files: [["profiles.yml", "my_profile:\n outputs:\n dev:\n type: snowflake\n"]], tags: ["snowflake"] }, + // ... +] + +for (const c of cases) { + test(`detects tags for ${c.name}`, async () => { + await using tmp = await tmpdir() + for (const [file, content] of c.files) { + await fs.writeFile(path.join(tmp.path, file), content) + } + const result = await Fingerprint.detect(tmp.path) + for (const tag of c.tags) expect(result.tags).toContain(tag) + }) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/altimate/fingerprint-detect.test.ts` around lines 12 - 63, Multiple tests repeat the same setup/write/detect/assert pattern around Fingerprint.detect; replace them with a table-driven approach (e.g., test.each or a loop over an array of cases) where each case includes the marker (filename or directory name like "dbt_project.yml", "profiles.yml", ".sqlfluff", "query.sql", "airflow.cfg", "dags", "databricks.yml"), the file contents or an indicator to mkdir, and the expected tag(s) (e.g., "dbt", "data-engineering", "snowflake", "sql", "airflow", "databricks"); in the test body create a tmpdir (using the existing tmpdir()/using tmp pattern), apply the case-specific setup (write file or mkdir), call Fingerprint.detect(tmp.path), and assert expected tags with expect(result.tags).toContain(...). This keeps the logic in one parametrized test and removes repeated test blocks like the individual "detects ..." tests.packages/opencode/test/altimate/dbt-lineage-helpers.test.ts (2)
269-278: Hardcoded/tmppath may cause issues on non-Unix platforms.The path
/tmp/definitely-not-a-manifest.jsonis Unix-specific. While this test likely runs only on Unix-like systems, usingos.tmpdir()would be more portable.♻️ Suggested fix for cross-platform compatibility
+import { tmpdir as osTmpdir } from "os" + // ... test("returns low confidence for non-existent manifest", () => { + const nonExistentPath = join(osTmpdir(), "definitely-not-a-manifest-" + Date.now() + ".json") const result = dbtLineage({ - manifest_path: "/tmp/definitely-not-a-manifest.json", + manifest_path: nonExistentPath, model: "orders", })🤖 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 - 278, The test uses a hardcoded Unix-only path "/tmp/definitely-not-a-manifest.json"; update the test that calls dbtLineage (test "returns low confidence for non-existent manifest") to build the non-existent manifest path using the platform temp directory (e.g., path.join(os.tmpdir(), "definitely-not-a-manifest.json")) so it works cross-platform—import the Node 'os' (and 'path' if needed) in the test file and replace the literal string with the constructed tmp path when calling dbtLineage.
13-14: Consider using the project'stmpdirutility fromfixture/fixture.ts.The coding guidelines specify using the
tmpdirfunction fromfixture/fixture.tswithawait usingsyntax for automatic cleanup. The current implementation manually manages temporary directories withmkdtempSyncandafterEachcleanup.Refactoring to use the project's utility would improve consistency across the test suite.
♻️ Suggested refactor using fixture tmpdir
import { describe, test, expect, afterEach } from "bun:test" import { dbtLineage } from "../../src/altimate/native/dbt/lineage" -import { writeFileSync, mkdtempSync, rmSync } from "fs" -import { tmpdir } from "os" +import { writeFileSync } from "fs" import { join } from "path" +import { tmpdir } from "../fixture/fixture"Then update each test to use
await using:describe("dbtLineage: model lookup", () => { - test("finds model by unique_id", () => { - const dir = makeTmpDir() + test("finds model by unique_id", async () => { + await using tmp = await tmpdir() + const dir = tmp.path const manifestPath = writeManifest(dir, BASE_MANIFEST) // ... }) })This eliminates the need for the
tmpDirsarray,makeTmpDir()helper, andafterEachcleanup block entirely.As per coding guidelines: "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup" and "Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope".Also applies to: 21-40
🤖 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 - 14, Replace manual temp-dir handling (mkdtempSync, tmpDirs array, makeTmpDir helper, and the afterEach cleanup) by importing the project's tmpdir utility from fixture/fixture.ts and using it with the `await using` syntax inside each test; update tests that call makeTmpDir() to instead `await using` the tmpdir(...) returned resource and remove the tmpDirs collection and afterEach block entirely so cleanup is automatic. Ensure you remove the top-level import of tmpdir from "os" and the fs/mkdtempSync usage, and update any references to temporary directory paths to use the variable provided by the tmpdir fixture in the test bodies.packages/opencode/test/altimate/registry-env-loading.test.ts (1)
12-14: Remove unused imports.The
mkdtempSync,writeFileSync,mkdirSync,rmSyncfromfs,tmpdirfromos, andjoinfrompathare imported but never used in this test file. The tests only manipulateprocess.envwithout creating temporary files.🧹 Remove dead imports
import { describe, test, expect, beforeEach, afterEach } from "bun:test" import * as Registry from "../../src/altimate/native/connections/registry" -import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from "fs" -import { tmpdir } from "os" -import { join } from "path"🤖 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 12 - 14, Remove the unused imports mkdtempSync, writeFileSync, mkdirSync, rmSync (from fs), tmpdir (from os), and join (from path) from the top of the test file; update the import statements to only include any actually used symbols (or remove the import lines entirely if nothing is used) and run the linter/tests to confirm no references remain (look for these symbols in registry-env-loading.test.ts to ensure no further usage).
🤖 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/bus/bus-event.test.ts`:
- Around line 20-23: The test currently calls
BusEvent.define("__test_payloads_registered", ...) at module scope which mutates
the global BusEvent registry during import; move this registration into each
test (e.g., inside the payloads() tests) or generate a unique event name per
test and/or call a BusEvent.reset() helper before/after each test so
registrations do not leak across tests—update the tests that reference
BusEvent.define (including the instances around lines with payloads() and the
block at 52-58) to perform define/reset within their test lifecycle to ensure
isolation.
In `@packages/opencode/test/command/builtin-commands.test.ts`:
- Around line 111-114: The test's title and inline comment incorrectly state
lexicographic ordering; update the test description and comment to reflect that
Command.hints(...) performs numeric sorting of placeholders. Locate the test
using the symbol Command.hints in builtin-commands.test.ts and change the test
name and the note to indicate numeric (numeric-aware) ordering (e.g., "sorts
numbered placeholders numerically") and remove or replace the misleading remark
about .sort() lexicographic behavior so the test intent matches the
implementation.
In `@packages/opencode/test/util/git.test.ts`:
- Around line 7-10: Tests are manually running git init via Bun.spawn; replace
those manual setups by using the test fixture tmpdir({ git: true }) so the temp
dir is created with a git root commit and consistent fixture behavior—update the
three places that call tmpdir() and then Bun.spawn(["git","init"], ...) (and the
accompanying Bun.spawn(["git","config","core.fsmonitor", "false"], ...)) to
instead call tmpdir({ git: true }) and remove the manual Bun.spawn invocations;
ensure you make the same change for the other occurrences in this file that use
tmpdir + Bun.spawn for git init.
---
Nitpick comments:
In `@packages/opencode/test/altimate/dbt-lineage-helpers.test.ts`:
- Around line 269-278: The test uses a hardcoded Unix-only path
"/tmp/definitely-not-a-manifest.json"; update the test that calls dbtLineage
(test "returns low confidence for non-existent manifest") to build the
non-existent manifest path using the platform temp directory (e.g.,
path.join(os.tmpdir(), "definitely-not-a-manifest.json")) so it works
cross-platform—import the Node 'os' (and 'path' if needed) in the test file and
replace the literal string with the constructed tmp path when calling
dbtLineage.
- Around line 13-14: Replace manual temp-dir handling (mkdtempSync, tmpDirs
array, makeTmpDir helper, and the afterEach cleanup) by importing the project's
tmpdir utility from fixture/fixture.ts and using it with the `await using`
syntax inside each test; update tests that call makeTmpDir() to instead `await
using` the tmpdir(...) returned resource and remove the tmpDirs collection and
afterEach block entirely so cleanup is automatic. Ensure you remove the
top-level import of tmpdir from "os" and the fs/mkdtempSync usage, and update
any references to temporary directory paths to use the variable provided by the
tmpdir fixture in the test bodies.
In `@packages/opencode/test/altimate/fingerprint-detect.test.ts`:
- Around line 12-63: Multiple tests repeat the same setup/write/detect/assert
pattern around Fingerprint.detect; replace them with a table-driven approach
(e.g., test.each or a loop over an array of cases) where each case includes the
marker (filename or directory name like "dbt_project.yml", "profiles.yml",
".sqlfluff", "query.sql", "airflow.cfg", "dags", "databricks.yml"), the file
contents or an indicator to mkdir, and the expected tag(s) (e.g., "dbt",
"data-engineering", "snowflake", "sql", "airflow", "databricks"); in the test
body create a tmpdir (using the existing tmpdir()/using tmp pattern), apply the
case-specific setup (write file or mkdir), call Fingerprint.detect(tmp.path),
and assert expected tags with expect(result.tags).toContain(...). This keeps the
logic in one parametrized test and removes repeated test blocks like the
individual "detects ..." tests.
In `@packages/opencode/test/altimate/registry-env-loading.test.ts`:
- Around line 12-14: Remove the unused imports mkdtempSync, writeFileSync,
mkdirSync, rmSync (from fs), tmpdir (from os), and join (from path) from the top
of the test file; update the import statements to only include any actually used
symbols (or remove the import lines entirely if nothing is used) and run the
linter/tests to confirm no references remain (look for these symbols in
registry-env-loading.test.ts to ensure no further usage).
In `@packages/opencode/test/util/keybind.test.ts`:
- Around line 214-232: The tests currently cast parsed key objects with "as
any", losing type safety; update the tests to import and use the actual
ParsedKey type (or the exported type alias) and cast the test objects to
ParsedKey when calling Keybind.fromParsedKey; locate the test file references to
Keybind.fromParsedKey and replace "as any" with the imported ParsedKey type
(e.g., import { ParsedKey } from the module that exports Keybind) so the literal
objects conform to the ParsedKey shape and the compiler validates the test
fixtures.
🪄 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: 40eb57ed-af9b-44d1-a62a-b24dbccdb7dd
📒 Files selected for processing (14)
packages/opencode/test/altimate/connections.test.tspackages/opencode/test/altimate/dbt-lineage-helpers.test.tspackages/opencode/test/altimate/driver-normalize.test.tspackages/opencode/test/altimate/fingerprint-detect.test.tspackages/opencode/test/altimate/registry-env-loading.test.tspackages/opencode/test/altimate/sql-analyze-format.test.tspackages/opencode/test/altimate/sql-analyze-tool.test.tspackages/opencode/test/altimate/warehouse-telemetry.test.tspackages/opencode/test/bus/bus-event.test.tspackages/opencode/test/command/builtin-commands.test.tspackages/opencode/test/command/hints-discover-mcps.test.tspackages/opencode/test/fixture/fixture.tspackages/opencode/test/util/git.test.tspackages/opencode/test/util/keybind.test.ts
- `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>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/util/git.test.ts (1)
46-50: Consider aligning assertion with test name.The test name states "returns exitCode 1" but the assertion checks
not.toBe(0). Since thegit()implementation explicitly returnsexitCode: 1in its catch block (seepackages/opencode/src/util/git.ts), you could make the assertion more precise:- expect(result.exitCode).not.toBe(0) + expect(result.exitCode).toBe(1)This is a minor nitpick - the current test is functionally correct and will catch regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/util/git.test.ts` around lines 46 - 50, The test's assertion is more permissive than its name: update the test for the git(...) call (the test that checks behavior when cwd does not exist) to assert the exact exit code returned by the implementation; replace the loose expect(result.exitCode).not.toBe(0) with expect(result.exitCode).toBe(1) (keep the existing stdout length assertion) so the test aligns with the git function's catch behavior which returns exitCode 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/test/util/git.test.ts`:
- Around line 46-50: The test's assertion is more permissive than its name:
update the test for the git(...) call (the test that checks behavior when cwd
does not exist) to assert the exact exit code returned by the implementation;
replace the loose expect(result.exitCode).not.toBe(0) with
expect(result.exitCode).toBe(1) (keep the existing stdout length assertion) so
the test aligns with the git function's catch behavior which returns exitCode 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c14a09a-df47-4022-89fd-c6555913df10
📒 Files selected for processing (4)
packages/opencode/test/altimate/registry-env-loading.test.tspackages/opencode/test/bus/bus-event.test.tspackages/opencode/test/command/builtin-commands.test.tspackages/opencode/test/util/git.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/opencode/test/altimate/registry-env-loading.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/test/bus/bus-event.test.ts
- packages/opencode/test/command/builtin-commands.test.ts
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 11 scattered test PRs into a single, deduplicated test suite adding 305 new tests across 13 files.
PRs consolidated: #440, #441, #479, #483, #484, #485, #490, #491, #492, #495, #496
Deduplication performed:
fromParsedKey)connections.test.ts: 4 PRs (test: MongoDB normalization + detectAuthMethod coverage #491, test: warehouse connections — env-var loading and MongoDB auth detection #490, test: keybind parsing & credential multi-field stripping #492, test: bus events + dbt profiles parser — registry payloads and env_var defaults #483) → merged unique tests from eachNew test files (11):
sql-analyze-tool.test.tsSqlAnalyzeToolformattingsql-analyze-format.test.tssql.analyzesuccess semanticsbuiltin-commands.test.tshints-discover-mcps.test.tsCommand.hints()+ discover-and-add-mcpsfingerprint-detect.test.tsdbt-lineage-helpers.test.tsregistry-env-loading.test.tsALTIMATE_CODE_CONN_*env var loadingwarehouse-telemetry.test.tsbus-event.test.tsgit.test.tskeybind.test.tsfromParsedKeyMerged into existing files (3):
connections.test.ts— +24 tests (loadFromEnv,detectAuthMethod, credential stripping,containerToConfig, dbt profiles edge cases)driver-normalize.test.ts— +13 MongoDB alias resolution testsfixture.ts— disablecommit.gpgsignin test tmpdirBug fixes in tests:
registry-env-loadingassertion that assumed isolated envhints-discover-mcpssort order expectation to match actual behaviorNot included: PR #424 (bug fixes to source files) — kept separate as it requires updating existing error propagation tests. PR #494 (Docker/CI sanity suite) — kept separate as it's infrastructure changes.
Type of change
Issue for this PR
Closes #497
How did you verify your code works?
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit