Merged
Conversation
Contributor
WalkthroughRefactors test infrastructure by centralizing VS Code API mocks into Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to eed9f81 in 2 minutes and 13 seconds. Click for details.
- Reviewed
1307lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/test/mock/vscode.ts:182
- Draft comment:
Consider adding an explicit return type (e.g., void) to the 'resetMocks' utility function for better refactoring support. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/test/suite/dbtProject.test.ts:518
- Draft comment:
Before invoking the event callback (using onCall), ensure that the callback exists to avoid potential runtime errors.
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the suggestion promotes safer code by using optional chaining, this is in a test file where we have full control over the mock setup. The find() call is looking for a specific mock call that we know exists based on the test setup. If the handler wasn't found, that would indicate a test setup issue that we want to fail fast on. Using optional chaining here could mask test setup problems. The comment promotes a generally good practice of defensive programming. In production code, this would be a valid suggestion. However, in test code we often want to fail fast when our assumptions are violated. The non-null assertion here acts as an implicit assertion that our test setup is correct. The comment should be deleted because it suggests applying production code best practices to test code where different rules apply. We want test failures to be loud and immediate.
3. src/test/suite/dbtProject.test.ts:125
- Draft comment:
There is repeated instantiation of DBTProject in multiple beforeEach blocks. Consider extracting a helper function to reduce duplication and improve readability. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_eZo9oXObVFbXjWaM
You can customize 
| ); | ||
|
|
||
| // Wait for the async dbt loom check to complete | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); |
Contributor
There was a problem hiding this comment.
Avoid using arbitrary timeouts (setTimeout) in tests. Consider using async utilities or Jest’s fake timers for more reliable asynchronous testing.
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed fbace47 in 2 minutes and 50 seconds. Click for details.
- Reviewed
914lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/test/suite/dbtProjectContainer.test.ts:1
- Draft comment:
Imported type 'DataPilotHealtCheckParams' appears to have a potential typo. Consider renaming it to 'DataPilotHealthCheckParams' for clarity if not dictated by an external dependency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the type name does appear to have a typo missing the 'h' in 'Health', this is coming from an external package that we don't control. Suggesting a rename would be pointless since we can't change the external package's type names. This would violate the rule about not commenting on dependency-related issues. The typo is quite clear and fixing it would improve code clarity. Maybe the external package could be updated? Even if the typo could be fixed in the external package, that's outside the scope of this PR review. We should only comment on things that can be fixed within this codebase. Delete the comment since it suggests changing something in an external dependency which is outside our control.
2. jest.config.js:11
- Draft comment:
Added reporter 'summary' in the reporters array. Ensure that the 'summary' reporter package is installed and configured correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/test/suite/dbtProjectContainer.test.ts:470
- Draft comment:
Typo: 'DataPilotHealtCheckParams' appears to be misspelled. Did you mean 'DataPilotHealthCheckParams'? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this appears to be a legitimate typo, it's in an imported type name from an external package '@altimateai/dbt-integration'. The type is consistently used throughout the code. Changing it would require changes in the external package, not in this codebase. Comments about external package names/types are outside our scope. The typo does exist and fixing it would improve code clarity. Maybe there's a way to fix it in this codebase? No, since this is an imported type from an external package, we can't fix the spelling in this codebase. Any changes would need to be made in the '@altimateai/dbt-integration' package itself. Delete the comment since it refers to a type name from an external package that we can't modify in this codebase.
Workflow ID: wflow_Z5tAirMLMOhyj4lR
You can customize
| configType: "All" as const, | ||
| } as DataPilotHealtCheckParams; | ||
|
|
||
| expect(() => container.executeAltimateDatapilotHealthcheck(args)).toThrow( |
Contributor
There was a problem hiding this comment.
The test for throwing an error when a project is not found uses a synchronous expect(...).toThrow(), but the function is async. Use 'await expect(container.executeAltimateDatapilotHealthcheck(args)).rejects.toThrow(...)' instead.
| ): DBTWorkspaceFolder => { | ||
| return mockDbtWorkspaceFolder; | ||
| }; | ||
| let mockDbtClient: any; |
Contributor
There was a problem hiding this comment.
Mocks are declared with type 'any'. Consider using strongly typed mocks (e.g., jest.Mocked) to improve type safety and ease future refactoring.
|
|
||
| // Verify the container has the expected event emitters | ||
| expect( | ||
| (newContainer as any)._onProjectRegisteredUnregistered, |
Contributor
There was a problem hiding this comment.
Tests in 'Project Registration Events' are accessing private properties (e.g. _onProjectRegisteredUnregistered). Prefer using the public interface to verify behavior, if available.
Suggested change
| (newContainer as any)._onProjectRegisteredUnregistered, | |
| newContainer.onProjectRegisteredUnregistered, |
| @@ -1,4 +1,7 @@ | |||
| import { DBTTerminal, EnvironmentVariables } from "@altimateai/dbt-integration"; | |||
| import { | |||
| DataPilotHealtCheckParams, | |||
Contributor
There was a problem hiding this comment.
Typographical error: it appears 'DataPilotHealtCheckParams' is intended to be 'DataPilotHealthCheckParams'. Please fix the typo.
Suggested change
| DataPilotHealtCheckParams, | |
| DataPilotHealthCheckParams, |
Contributor
|
@saravmajestic : these tests don't run in CI, can you look into this? |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+115
to
+117
| withProgress: jest | ||
| .fn() | ||
| .mockImplementation((_options: any, task: any) => task()), |
There was a problem hiding this comment.
Bug: The window.withProgress mock is incomplete. It fails to pass the required token argument to the task function, which will cause a crash in production code that handles cancellation.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The mock implementation of window.withProgress in src/test/mock/vscode.ts calls the provided task function without the required arguments. Production code in src/dbt_client/dbtProject.ts expects this task to receive a token object as its second argument and subsequently calls token.onCancellationRequested. Because the mock passes undefined instead of a token object, any operation that uses this progress indicator (e.g., runModel, buildModel) will crash with a "Cannot read property 'onCancellationRequested' of undefined" error. This issue is not detected by the test suite because the specific code paths are not tested.
💡 Suggested Fix
Update the withProgress mock to call the task function with the two expected arguments: a mock progress object and a mock token object. The token object should have a mock onCancellationRequested function. For example: task({ report: jest.fn() }, { onCancellationRequested: jest.fn() }).
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/test/mock/vscode.ts#L115-L117
Potential issue: The mock implementation of `window.withProgress` in
`src/test/mock/vscode.ts` calls the provided `task` function without the required
arguments. Production code in `src/dbt_client/dbtProject.ts` expects this `task` to
receive a `token` object as its second argument and subsequently calls
`token.onCancellationRequested`. Because the mock passes `undefined` instead of a
`token` object, any operation that uses this progress indicator (e.g., `runModel`,
`buildModel`) will crash with a "Cannot read property 'onCancellationRequested' of
undefined" error. This issue is not detected by the test suite because the specific code
paths are not tested.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7737468
Collaborator
Author
I will take a loot early next week |
![linearb[bot]](https://avatars.githubusercontent.com/in/1658443?s=60&v=4){kind=small}
Adapts Sarav's unit-test additions from #1718 to the shape of master: - DBTProject now takes RunHistoryService and a manifest-changed emitter; tests pass matching mocks. - ParsedManifest gained functionMetaMap and RunResultEntry renamed unique_id -> uniqueId; fixtures updated. - Walkthrough tests point at openOnboarding (was openSetupWalkthrough). - Container model-ops tests stub fs.realpathSync.native so synthetic paths don't ENOENT. - Widen mock/vscode.ts with ThemeIcon/ThemeColor/CancellationToken*, a functional EventEmitter, and Location.range; these were provided by setup.ts's inline jest.mock before the cleanup commit removed it.
mdesmet
force-pushed
the
chore/dbt-integration-tests
branch
from
8c1ad2d to
51b7216
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/test/mock/vscode.ts (2)
157-162:getWorkspaceFoldermock ignores itsuriargument and has incorrect typing.Two issues here:
- The parameter type
uri: typeof Urirefers to the type of theUriobject (the namespace value), not aUriinstance. It should be typed as the instance type (orany), otherwise callers that pass a realUri-like value won't match.- The
uriargument is never consulted — the mock always returnsworkspaceFolders[0]. IndbtProjectContainer.test.tsthe tests exercise multi-workspace / added-folder scenarios, so a URI-unaware mock can produce misleading passes if production code later starts relying ongetWorkspaceFolderto actually map a URI → folder.Suggested refactor
- getWorkspaceFolder: jest.fn((uri: typeof Uri) => { - if (workspace.workspaceFolders && workspace.workspaceFolders.length > 0) { - return workspace.workspaceFolders[0]; - } - return undefined; - }), + getWorkspaceFolder: jest.fn((uri: { fsPath: string }) => { + const folders = workspace.workspaceFolders ?? []; + return ( + folders.find((f: any) => uri?.fsPath?.startsWith(f.uri.fsPath)) ?? + folders[0] + ); + }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/mock/vscode.ts` around lines 157 - 162, The getWorkspaceFolder mock currently types its parameter as "typeof Uri" and ignores the passed-in uri, always returning workspace.workspaceFolders[0]; change the mock for getWorkspaceFolder to accept an instance-like uri (use Uri or any as the parameter type instead of typeof Uri) and implement logic that returns the workspace folder matching the uri (e.g., iterate workspace.workspaceFolders and return the folder whose uri matches or contains the provided uri), falling back to undefined if none match—update the mock in src/test/mock/vscode.ts (function getWorkspaceFolder) so tests that rely on URI→folder mapping behave correctly.
197-219:EventEmitter.eventuses an arrow property instead of a getter — diverges from the real VS Code API shape.The real
vscode.EventEmitter<T>.eventis anEvent<T>accessed as a property (effectively a getter returning a subscribe function). Hereeventis an arrow-function instance property, which works for the current call patternemitter.event(listener)but means:
typeof emitter.event === "function"always (fine), but the reference is recreated per instance (fine in tests).- Any code that compares
emitter.eventidentity across calls, or that relies onEvent<T>being a value you can assign to a field and reuse, will behave differently than production.Minor, but worth either adding a brief comment explaining the simplification or aligning the shape:
- event = (listener: (e: T) => any) => { + // Simplified Event<T>: real vscode exposes this as a getter/property. + event = (listener: (e: T) => any) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/mock/vscode.ts` around lines 197 - 219, The mock EventEmitter.event should match the VS Code API shape (a getter returning an Event<T> value) instead of an instance arrow property; change the current event property to a getter that returns a single reusable Event<T> function (e.g., store a private _event value created once in the constructor and have get event(): Event<T> { return this._event; }) so listeners are registered the same way and emitter.event identity is stable; keep the existing listener push/dispose/fire/dispose logic but move the listener-registration logic into the stored _event function referenced by the getter.src/test/suite/dbtProject.test.ts (2)
94-107: Redundantjest.mock("vscode", ...)—moduleNameMapperinjest.config.jsalready routesvscodetosrc/test/mock/vscode.ts.
jest.config.jsalready maps"^vscode$"→<rootDir>/src/test/mock/vscode.ts, so everyimport * as vscode from "vscode"in this file already resolves to the mock. Callingjest.mock("vscode", () => jest.requireActual("../mock/vscode"))is a no-op at best and can subtly change module identity (the mock instance used viaimport * as vscodevs. viajest.requireActualcan diverge, causing state likeworkspace.workspaceFoldersset inbeforeEachto be seen by one consumer but not another).The same applies to
dbtProjectContainer.test.tslines 20-23.Consider removing the explicit
jest.mock("vscode", ...)block and relying on themoduleNameMapperalone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/suite/dbtProject.test.ts` around lines 94 - 107, Remove the explicit jest.mock("vscode", ...) call from the test file because jest.config.js's moduleNameMapper already routes "vscode" to the shared mock; delete the whole block starting with jest.mock("vscode", ...) to avoid creating a second module identity and potential state divergence, and keep the existing jest.mock("../../utils", ...) stubs; also remove the same explicit jest.mock("vscode", ...) block in dbtProjectContainer.test.ts (the one around lines ~20-23) so both tests rely solely on the moduleNameMapper-provided mock.
329-421: RepeatedDBTProjectconstructor calls across everydescribe— extract a factory.The 17-argument
new DBTProject(...)block is duplicated in ~6beforeEach/itblocks in this file, making changes to the constructor signature (which the PR description notes has already happened once:RunHistoryService+manifestChangedEmitter) painful and error-prone. A small helper would reduce noise and keep future signature changes to one site.Suggested helper
const createDbtProject = (overrides: Partial<{ uri: vscode.Uri; config: any }> = {}) => new DBTProject( mockPythonEnvironment, dbtProjectLogFactory as any, mockCommandFactory, mockTerminal, mockSharedStateService, mockTelemetry, mockExecutionInfrastructure, jest.fn().mockReturnValue(mockProjectIntegration) as any, mockAltimate, mockValidationProvider, mockAltimateAuthService, mockRunHistoryService, overrides.uri ?? vscode.Uri.file("/test/project"), overrides.config ?? {}, mockManifestChangedEmitter, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/suite/dbtProject.test.ts` around lines 329 - 421, The test file repeats the long DBTProject constructor call across multiple tests; extract a helper factory (e.g., createDbtProject) that returns new DBTProject(...) using existing mocks (mockPythonEnvironment, dbtProjectLogFactory, mockCommandFactory, mockTerminal, mockSharedStateService, mockTelemetry, mockExecutionInfrastructure, jest.fn().mockReturnValue(mockProjectIntegration), mockAltimate, mockValidationProvider, mockAltimateAuthService, mockRunHistoryService, mockManifestChangedEmitter) and accepts an overrides parameter for uri/config so tests can call createDbtProject({ uri, config }) instead of repeating the 17-arg constructor; replace each direct new DBTProject(...) usage in the file with the helper to centralize the signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/mock/vscode.ts`:
- Around line 9-12: The Uri mock exported as Uri should include the missing
members used by production code: ensure Uri.file and Uri.parse return objects
that include a scheme property (string), a with(changes) method that returns a
new Uri-like object merging provided properties (e.g., changing fsPath/scheme)
and a toString() method that returns a stable string representation; wrap these
behaviors with jest.fn where appropriate so tests can spy on them. Update the
Uri mock implementation referenced by production usages like validateSql,
sqlPreviewContentProvider, dbtProjectContainer and commands index so code using
uri.scheme, uri.with(...) and uri.toString() will receive objects with those
properties/methods.
In `@src/test/suite/dbtProject.test.ts`:
- Around line 739-753: The test currently uses await
expect(dbtProject.validateSql(request)).rejects.toBeDefined(), which accepts any
rejection and is too weak; change it to assert the specific error from
validateSql (e.g., use await expect(...).rejects.toThrow("expected validation
error message") or await expect(...).rejects.toMatchObject({ message: "..." })
so the test verifies the actual failure reason, while keeping the existing
assertions that mockExecutionInfrastructure.createPythonBridge and
.closePythonBridge were called; locate the call to validateSql in this test and
replace the generic rejects.toBeDefined() with a precise rejection assertion (or
assert on a specific Error instance) to ensure the wiring and error behavior are
validated.
- Around line 1078-1108: The test uses an arbitrary 100ms setTimeout to wait for
the async dbt-loom check in DBTProject, which is flaky; remove the fixed timeout
and instead deterministically wait for the work to finish by either (a)
polling/asserting until
mockTelemetry.setTelemetryCustomAttribute("dbtLoomInstalled", "true") is
observed (a waitFor-style loop that retries the expect), (b) use
jest.useFakeTimers() and advance timers/flush microtasks to let
mockExecutionInfrastructure.createPythonBridge().ex resolve, or (c) expose and
await a completion promise from DBTProject (e.g., a detection promise/property)
that the test can await; reference DBTProject,
mockExecutionInfrastructure.createPythonBridge, and
mockTelemetry.setTelemetryCustomAttribute when making the change.
- Around line 34-81: The mock currently uses || fallbacks (e.g.,
DBT_PROJECT_FILE, MANIFEST_FILE, RESOURCE_TYPE_MODEL, RESOURCE_TYPE_METRIC,
DEFAULT_CONFIGURATION_VALUES, DBTProjectIntegrationAdapterEvents, etc.), which
masks removed/renamed exports; replace those || fallbacks with nullish
coalescing checks that throw a clear error when the upstream export is undefined
(use actualModule.X ?? throw new Error("Missing export: X")) so tests fail
loudly if the module contract changes; apply this pattern for every constant
defined in the mock (including DBT_PROJECT_FILE, MANIFEST_FILE,
RUN_RESULTS_FILE, CATALOG_FILE, RESOURCE_TYPE_*, DEFAULT_CONFIGURATION_VALUES,
and DBTProjectIntegrationAdapterEvents).
In `@src/test/suite/dbtProjectContainer.test.ts`:
- Around line 81-90: The mock's onDBTInstallationVerification currently
references an unbound method (new EventEmitter().on) which will throw when
called; replace it with a properly bound subscriber or a function-shaped Event:
create an EventEmitter instance (e.g., const emitter = new EventEmitter()) and
set onDBTInstallationVerification to emitter.on.bind(emitter) or to a wrapper
function that accepts a listener, registers it on the emitter, and returns a
disposable (with dispose removing the listener); update the mockDbtClient object
to use that bound/wrapper function so calls to
onDBTInstallationVerification(listener) work safely.
- Around line 868-870: Make the test async and await the async initialization to
ensure event-handler registration completes: change the test callback to async
and replace the call with await newContainer.initializeDBTProjects(); then stop
silently masking missing registration by removing or tightening the "if
(statusChangeHandler)" guard so the test fails if
onRebuildManifestStatusChange's statusChangeHandler is still undefined (refer to
initializeDBTProjects, onRebuildManifestStatusChange, and statusChangeHandler).
---
Nitpick comments:
In `@src/test/mock/vscode.ts`:
- Around line 157-162: The getWorkspaceFolder mock currently types its parameter
as "typeof Uri" and ignores the passed-in uri, always returning
workspace.workspaceFolders[0]; change the mock for getWorkspaceFolder to accept
an instance-like uri (use Uri or any as the parameter type instead of typeof
Uri) and implement logic that returns the workspace folder matching the uri
(e.g., iterate workspace.workspaceFolders and return the folder whose uri
matches or contains the provided uri), falling back to undefined if none
match—update the mock in src/test/mock/vscode.ts (function getWorkspaceFolder)
so tests that rely on URI→folder mapping behave correctly.
- Around line 197-219: The mock EventEmitter.event should match the VS Code API
shape (a getter returning an Event<T> value) instead of an instance arrow
property; change the current event property to a getter that returns a single
reusable Event<T> function (e.g., store a private _event value created once in
the constructor and have get event(): Event<T> { return this._event; }) so
listeners are registered the same way and emitter.event identity is stable; keep
the existing listener push/dispose/fire/dispose logic but move the
listener-registration logic into the stored _event function referenced by the
getter.
In `@src/test/suite/dbtProject.test.ts`:
- Around line 94-107: Remove the explicit jest.mock("vscode", ...) call from the
test file because jest.config.js's moduleNameMapper already routes "vscode" to
the shared mock; delete the whole block starting with jest.mock("vscode", ...)
to avoid creating a second module identity and potential state divergence, and
keep the existing jest.mock("../../utils", ...) stubs; also remove the same
explicit jest.mock("vscode", ...) block in dbtProjectContainer.test.ts (the one
around lines ~20-23) so both tests rely solely on the moduleNameMapper-provided
mock.
- Around line 329-421: The test file repeats the long DBTProject constructor
call across multiple tests; extract a helper factory (e.g., createDbtProject)
that returns new DBTProject(...) using existing mocks (mockPythonEnvironment,
dbtProjectLogFactory, mockCommandFactory, mockTerminal, mockSharedStateService,
mockTelemetry, mockExecutionInfrastructure,
jest.fn().mockReturnValue(mockProjectIntegration), mockAltimate,
mockValidationProvider, mockAltimateAuthService, mockRunHistoryService,
mockManifestChangedEmitter) and accepts an overrides parameter for uri/config so
tests can call createDbtProject({ uri, config }) instead of repeating the 17-arg
constructor; replace each direct new DBTProject(...) usage in the file with the
helper to centralize the signature.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: affc1a0e-dd42-4063-be94-5c4a37188092
📒 Files selected for processing (7)
jest.config.jssrc/test/mock/vscode.tssrc/test/setup.tssrc/test/suite/conversationProvider.test.tssrc/test/suite/dbtProject.test.tssrc/test/suite/dbtProjectContainer.test.tssrc/test/suite/dbtWorkspaceFolder.test.ts
💤 Files with no reviewable changes (2)
- src/test/suite/dbtWorkspaceFolder.test.ts
- src/test/setup.ts
Comment on lines
+9
to
+12
| export const Uri = { | ||
| file: jest.fn((f: string) => ({ fsPath: f })), | ||
| parse: jest.fn(), | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for production/test usages that may break with the thinner Uri mock.
rg -nP '\binstanceof\s+Uri\b' --type=ts -C2
echo '---'
rg -nP '\.scheme\b|\.toString\(\)|\.with\(' --type=ts -g 'src/**' -g '!src/test/**' -C1 | rg -n 'Uri' -C1Repository: AltimateAI/vscode-dbt-power-user
Length of output: 3876
Uri mock is incomplete — .scheme, .with(), and .toString() are used by production code but not mocked.
The mock currently only provides file and parse methods. However, production code extensively uses:
uri.schemeproperty (e.g.,src/commands/validateSql.ts:195,src/dbt_client/dbtProjectContainer.ts:243)uri.with()method (e.g.,src/content_provider/sqlPreviewContentProvider.ts:40,src/commands/validateSql.ts:199)uri.toString()method (e.g.,src/content_provider/sqlPreviewContentProvider.ts:35,src/commands/index.ts:148)
Tests exercising these code paths will fail with the current mock. Add the missing properties and methods to the Uri mock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/mock/vscode.ts` around lines 9 - 12, The Uri mock exported as Uri
should include the missing members used by production code: ensure Uri.file and
Uri.parse return objects that include a scheme property (string), a
with(changes) method that returns a new Uri-like object merging provided
properties (e.g., changing fsPath/scheme) and a toString() method that returns a
stable string representation; wrap these behaviors with jest.fn where
appropriate so tests can spy on them. Update the Uri mock implementation
referenced by production usages like validateSql, sqlPreviewContentProvider,
dbtProjectContainer and commands index so code using uri.scheme, uri.with(...)
and uri.toString() will receive objects with those properties/methods.
Comment on lines
+34
to
+81
| jest.mock("@altimateai/dbt-integration", () => { | ||
| // Get the actual module to inherit constants | ||
| const actualModule = jest.requireActual("@altimateai/dbt-integration") as any; | ||
|
|
||
| return { | ||
| // First, include all the constants that should be inherited | ||
| DBT_PROJECT_FILE: actualModule.DBT_PROJECT_FILE || "dbt_project.yml", | ||
| MANIFEST_FILE: actualModule.MANIFEST_FILE || "manifest.json", | ||
| RUN_RESULTS_FILE: actualModule.RUN_RESULTS_FILE || "run_results.json", | ||
| CATALOG_FILE: actualModule.CATALOG_FILE || "catalog.json", | ||
| RESOURCE_TYPE_MODEL: actualModule.RESOURCE_TYPE_MODEL || "model", | ||
| RESOURCE_TYPE_MACRO: actualModule.RESOURCE_TYPE_MACRO || "macro", | ||
| RESOURCE_TYPE_ANALYSIS: actualModule.RESOURCE_TYPE_ANALYSIS || "analysis", | ||
| RESOURCE_TYPE_SOURCE: actualModule.RESOURCE_TYPE_SOURCE || "source", | ||
| RESOURCE_TYPE_EXPOSURE: actualModule.RESOURCE_TYPE_EXPOSURE || "exposure", | ||
| RESOURCE_TYPE_SEED: actualModule.RESOURCE_TYPE_SEED || "seed", | ||
| RESOURCE_TYPE_SNAPSHOT: actualModule.RESOURCE_TYPE_SNAPSHOT || "snapshot", | ||
| RESOURCE_TYPE_TEST: actualModule.RESOURCE_TYPE_TEST || "test", | ||
| RESOURCE_TYPE_METRIC: actualModule.RESOURCE_TYPE_METRIC || "semantic_model", | ||
|
|
||
| // Include any other constants from the actual module | ||
| DEFAULT_CONFIGURATION_VALUES: | ||
| actualModule.DEFAULT_CONFIGURATION_VALUES || {}, | ||
|
|
||
| // Mock functions | ||
| validateSQLUsingSqlGlot: jest.fn(), | ||
| validateSQL: jest.fn(), | ||
|
|
||
| // Keep the actual event constants but ensure they're defined | ||
| DBTProjectIntegrationAdapterEvents: | ||
| actualModule.DBTProjectIntegrationAdapterEvents || { | ||
| SOURCE_FILE_CHANGED: "sourceFileChanged", | ||
| MANIFEST_PARSED: "manifestParsed", | ||
| RUN_RESULTS_PARSED: "runResultsParsed", | ||
| DIAGNOSTICS_CHANGED: "diagnosticsChanged", | ||
| PROJECT_CONFIG_CHANGED: "projectConfigChanged", | ||
| REBUILD_MANIFEST_STATUS_CHANGE: "rebuildManifestStatusChange", | ||
| }, | ||
|
|
||
| // Mock the error class but keep it extending Error | ||
| NoCredentialsError: class NoCredentialsError extends Error { | ||
| constructor(message?: string) { | ||
| super(message); | ||
| this.name = "NoCredentialsError"; | ||
| } | ||
| }, | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which of these constants are actually exported by the installed package.
rg -nP 'export\s+(const|enum)\s+(DBT_PROJECT_FILE|MANIFEST_FILE|RUN_RESULTS_FILE|CATALOG_FILE|RESOURCE_TYPE_(MODEL|MACRO|ANALYSIS|SOURCE|EXPOSURE|SEED|SNAPSHOT|TEST|METRIC)|DBTProjectIntegrationAdapterEvents)\b' -g 'node_modules/@altimateai/dbt-integration/**'Repository: AltimateAI/vscode-dbt-power-user
Length of output: 224
🏁 Script executed:
#!/bin/bash
# First, find if node_modules exists and check package structure
ls -la node_modules/@altimateai/ 2>/dev/null | head -20 || echo "node_modules path not accessible"
# Alternative: Check if there's a package.json in the repo referencing this package
cat package.json | grep -A 5 -B 5 "@altimateai/dbt-integration"Repository: AltimateAI/vscode-dbt-power-user
Length of output: 765
🏁 Script executed:
#!/bin/bash
# Check what the actual imports of `@altimateai/dbt-integration` look like in the codebase
rg "from ['\"]@altimateai/dbt-integration['\"]" --type ts --type tsx -A 5 | head -80Repository: AltimateAI/vscode-dbt-power-user
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Search for where these constants are actually used in the codebase
rg "DBT_PROJECT_FILE|MANIFEST_FILE|RUN_RESULTS_FILE|CATALOG_FILE|RESOURCE_TYPE_MODEL" --type ts --type tsx -lRepository: AltimateAI/vscode-dbt-power-user
Length of output: 103
🏁 Script executed:
#!/bin/bash
# Check the actual exports from the dbt-integration package
ls -la node_modules/@altimateai/dbt-integration/Repository: AltimateAI/vscode-dbt-power-user
Length of output: 423
🏁 Script executed:
#!/bin/bash
# Look at the main export file
cat node_modules/@altimateai/dbt-integration/package.json | head -30Repository: AltimateAI/vscode-dbt-power-user
Length of output: 1093
🏁 Script executed:
#!/bin/bash
# Search for imports of the package without tsx filter
rg "from ['\"]@altimateai/dbt-integration['\"]" -A 5 | head -100Repository: AltimateAI/vscode-dbt-power-user
Length of output: 6433
🏁 Script executed:
#!/bin/bash
# Check how these constants are actually used in the source code
rg "DBT_PROJECT_FILE|MANIFEST_FILE|RUN_RESULTS_FILE" -t ts -C 3 | head -150Repository: AltimateAI/vscode-dbt-power-user
Length of output: 6728
🏁 Script executed:
#!/bin/bash
# Check the TypeScript definitions to see what's actually exported
cat node_modules/@altimateai/dbt-integration/dist/index.d.ts | head -200Repository: AltimateAI/vscode-dbt-power-user
Length of output: 5244
🏁 Script executed:
#!/bin/bash
# Check if there's a source file we can examine
ls -la node_modules/@altimateai/dbt-integration/dist/Repository: AltimateAI/vscode-dbt-power-user
Length of output: 717
🏁 Script executed:
#!/bin/bash
# Search for the actual constant exports in the index.d.ts file
grep -n "export\s\+const\|export\s\+enum\|declare\s\+const" node_modules/@altimateai/dbt-integration/dist/index.d.ts | head -50Repository: AltimateAI/vscode-dbt-power-user
Length of output: 1205
🏁 Script executed:
#!/bin/bash
# Get more of the definition file to find exports
tail -400 node_modules/@altimateai/dbt-integration/dist/index.d.tsRepository: AltimateAI/vscode-dbt-power-user
Length of output: 23094
Use nullish coalescing (??) with thrown errors instead of fallback values to catch breaking upstream changes.
The fallback pattern (actualModule.CONSTANT || "literal") silently masks when constants are removed or renamed upstream. This allows tests to pass against stale literals while production code breaks. For example, if the package stops exporting RESOURCE_TYPE_METRIC, the mock will still return "semantic_model" without signaling the problem.
Use nullish coalescing and throw errors for missing exports instead:
- DBT_PROJECT_FILE: actualModule.DBT_PROJECT_FILE || "dbt_project.yml",
+ DBT_PROJECT_FILE: actualModule.DBT_PROJECT_FILE
+ ?? (() => { throw new Error("DBT_PROJECT_FILE missing from `@altimateai/dbt-integration`"); })(),Apply this pattern to all fallback constants to ensure tests fail loudly when the contract changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProject.test.ts` around lines 34 - 81, The mock currently
uses || fallbacks (e.g., DBT_PROJECT_FILE, MANIFEST_FILE, RESOURCE_TYPE_MODEL,
RESOURCE_TYPE_METRIC, DEFAULT_CONFIGURATION_VALUES,
DBTProjectIntegrationAdapterEvents, etc.), which masks removed/renamed exports;
replace those || fallbacks with nullish coalescing checks that throw a clear
error when the upstream export is undefined (use actualModule.X ?? throw new
Error("Missing export: X")) so tests fail loudly if the module contract changes;
apply this pattern for every constant defined in the mock (including
DBT_PROJECT_FILE, MANIFEST_FILE, RUN_RESULTS_FILE, CATALOG_FILE,
RESOURCE_TYPE_*, DEFAULT_CONFIGURATION_VALUES, and
DBTProjectIntegrationAdapterEvents).
Comment on lines
+739
to
+753
| it("should validate SQL", async () => { | ||
| const request = { | ||
| sql: "SELECT * FROM table", | ||
| dialect: "postgres", | ||
| models: [], | ||
| }; | ||
|
|
||
| // The real validateSQL requires a working python bridge; here we just | ||
| // verify the wiring: the python bridge is created and closed around the | ||
| // call, even when validation itself surfaces an error. | ||
| await expect(dbtProject.validateSql(request)).rejects.toBeDefined(); | ||
|
|
||
| expect(mockExecutionInfrastructure.createPythonBridge).toHaveBeenCalled(); | ||
| expect(mockExecutionInfrastructure.closePythonBridge).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
rejects.toBeDefined() is a near-useless assertion.
toBeDefined() on a rejection passes for virtually any thrown value (including new Error(""), strings, numbers). Combined with the comment above it, the intent is to verify the bridge is created/closed around validateSql — so assert on the cause directly (message/type) or on the exact wiring, not on "some rejection occurred". Otherwise a future refactor that breaks validateSql entirely (e.g., throws on wrong argument) will silently keep this test green.
- await expect(dbtProject.validateSql(request)).rejects.toBeDefined();
+ await expect(dbtProject.validateSql(request)).rejects.toThrow(
+ /* specific error matcher, e.g. /python bridge/i or an Error subclass */,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProject.test.ts` around lines 739 - 753, The test currently
uses await expect(dbtProject.validateSql(request)).rejects.toBeDefined(), which
accepts any rejection and is too weak; change it to assert the specific error
from validateSql (e.g., use await expect(...).rejects.toThrow("expected
validation error message") or await expect(...).rejects.toMatchObject({ message:
"..." }) so the test verifies the actual failure reason, while keeping the
existing assertions that mockExecutionInfrastructure.createPythonBridge and
.closePythonBridge were called; locate the call to validateSql in this test and
replace the generic rejects.toBeDefined() with a precise rejection assertion (or
assert on a specific Error instance) to ensure the wiring and error behavior are
validated.
Comment on lines
+1078
to
+1108
| it("should check if dbt loom is installed", async () => { | ||
| const pythonBridge = mockExecutionInfrastructure.createPythonBridge(); | ||
| (pythonBridge.ex as any).mockResolvedValue(undefined); | ||
|
|
||
| const projectUri = vscode.Uri.file("/test/project"); | ||
| dbtProject = new DBTProject( | ||
| mockPythonEnvironment, | ||
| dbtProjectLogFactory as any, | ||
| mockCommandFactory, | ||
| mockTerminal, | ||
| mockSharedStateService, | ||
| mockTelemetry, | ||
| mockExecutionInfrastructure, | ||
| jest.fn().mockReturnValue(mockProjectIntegration) as any, | ||
| mockAltimate, | ||
| mockValidationProvider, | ||
| mockAltimateAuthService, | ||
| mockRunHistoryService, | ||
| projectUri, | ||
| {}, | ||
| mockManifestChangedEmitter, | ||
| ); | ||
|
|
||
| // Wait for the async dbt loom check to complete | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| expect(() => mockValidationProvider.validateCredentialsSilently()).toThrow( | ||
| NoCredentialsError, | ||
| ); | ||
| expect(mockTelemetry.setTelemetryCustomAttribute).toHaveBeenCalledWith( | ||
| "dbtLoomInstalled", | ||
| "true", | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Flaky test: arbitrary 100ms setTimeout to wait for async work.
Waiting a fixed 100ms for the internal dbt-loom check to complete is racy — under load (slow CI runners, Node warmup) the promise chain may not have resolved yet, and on fast machines it wastes time. This is a common source of "passes locally, fails in CI" flake.
Prefer deterministic synchronization: expose/await the loom-check promise, use jest.useFakeTimers() + await Promise.resolve() flushing, or use waitFor-style polling on the assertion.
Example using a polling helper
- // Wait for the async dbt loom check to complete
- await new Promise((resolve) => setTimeout(resolve, 100));
-
- expect(mockTelemetry.setTelemetryCustomAttribute).toHaveBeenCalledWith(
- "dbtLoomInstalled",
- "true",
- );
+ // Poll until the telemetry attribute is recorded (bounded retry).
+ const deadline = Date.now() + 2000;
+ while (
+ !(mockTelemetry.setTelemetryCustomAttribute as jest.Mock).mock.calls
+ .some((c) => c[0] === "dbtLoomInstalled") &&
+ Date.now() < deadline
+ ) {
+ await new Promise((r) => setImmediate(r));
+ }
+ expect(mockTelemetry.setTelemetryCustomAttribute).toHaveBeenCalledWith(
+ "dbtLoomInstalled",
+ "true",
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProject.test.ts` around lines 1078 - 1108, The test uses an
arbitrary 100ms setTimeout to wait for the async dbt-loom check in DBTProject,
which is flaky; remove the fixed timeout and instead deterministically wait for
the work to finish by either (a) polling/asserting until
mockTelemetry.setTelemetryCustomAttribute("dbtLoomInstalled", "true") is
observed (a waitFor-style loop that retries the expect), (b) use
jest.useFakeTimers() and advance timers/flush microtasks to let
mockExecutionInfrastructure.createPythonBridge().ex resolve, or (c) expose and
await a completion promise from DBTProject (e.g., a detection promise/property)
that the test can await; reference DBTProject,
mockExecutionInfrastructure.createPythonBridge, and
mockTelemetry.setTelemetryCustomAttribute when making the change.
Comment on lines
81
to
+90
| mockDbtClient = { | ||
| onDBTInstallationVerification: new EventEmitter().event, | ||
| onDBTInstallationVerification: new EventEmitter().on, | ||
| dispose: jest.fn(), | ||
| } as unknown as jest.Mocked<DBTClient>; | ||
| showErrorIfDbtOrPythonNotInstalled: jest.fn(), | ||
| showErrorIfDbtIsNotInstalled: jest.fn(), | ||
| detectDBT: jest.fn(() => Promise.resolve()), | ||
| getPythonEnvironment: jest.fn(() => ({ | ||
| pythonPath: "/path/to/python", | ||
| })), | ||
| }; |
Contributor
There was a problem hiding this comment.
onDBTInstallationVerification: new EventEmitter().on captures an unbound method — will throw if ever invoked.
new EventEmitter().on is a reference to EventEmitter.prototype.on detached from the instance. If production code (or a future test) does container.onDBTInstallationVerification(listener), it'll execute on(...) with this === undefined and crash. The only assertion using this (line 150-152) is identity-based and therefore green by accident.
Either use a real Event<T>-shaped subscriber (a function taking a listener and returning a disposable), or bind the method:
- onDBTInstallationVerification: new EventEmitter().on,
+ onDBTInstallationVerification: jest.fn(() => ({ dispose: jest.fn() })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProjectContainer.test.ts` around lines 81 - 90, The mock's
onDBTInstallationVerification currently references an unbound method (new
EventEmitter().on) which will throw when called; replace it with a properly
bound subscriber or a function-shaped Event: create an EventEmitter instance
(e.g., const emitter = new EventEmitter()) and set onDBTInstallationVerification
to emitter.on.bind(emitter) or to a wrapper function that accepts a listener,
registers it on the emitter, and returns a disposable (with dispose removing the
listener); update the mockDbtClient object to use that bound/wrapper function so
calls to onDBTInstallationVerification(listener) work safely.
Comment on lines
+868
to
+870
| // Mock workspace folders | ||
| (workspace as any).workspaceFolders = [workspaceFolder]; | ||
| newContainer.initializeDBTProjects(); |
Contributor
There was a problem hiding this comment.
initializeDBTProjects() is not awaited — event-handler registration is racy.
initializeDBTProjects is async (used with await in the earlier test at line 517). Not awaiting it here means the onRebuildManifestStatusChange registration may not have run before the if (statusChangeHandler) check executes, which is silently bypassed when statusChangeHandler is still undefined — turning the whole test into a no-op. The if guard masks the problem.
- (workspace as any).workspaceFolders = [workspaceFolder];
- newContainer.initializeDBTProjects();
+ (workspace as any).workspaceFolders = [workspaceFolder];
+ await newContainer.initializeDBTProjects();
@@
- if (statusChangeHandler) {
- statusChangeHandler({ ... });
- ...
- }
+ expect(statusChangeHandler).toBeDefined();
+ statusChangeHandler({ project: mockDbtProject, inProgress: true });Also mark the it(...) callback async.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProjectContainer.test.ts` around lines 868 - 870, Make the
test async and await the async initialization to ensure event-handler
registration completes: change the test callback to async and replace the call
with await newContainer.initializeDBTProjects(); then stop silently masking
missing registration by removing or tightening the "if (statusChangeHandler)"
guard so the test fails if onRebuildManifestStatusChange's statusChangeHandler
is still undefined (refer to initializeDBTProjects,
onRebuildManifestStatusChange, and statusChangeHandler).
Bundle Size Reportdarwin-arm64: 70.0 MB
linux-x64: 71.7 MB
win32-x64: 72.7 MB
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/test/suite/dbtProject.test.ts (3)
742-756:⚠️ Potential issue | 🟡 MinorAssert the specific validation failure.
Line 752 still accepts any rejection, so unrelated breakage in
validateSql()keeps this test green. Configure the mocked validator/bridge to reject with a known error and assert that error.- await expect(dbtProject.validateSql(request)).rejects.toBeDefined(); + await expect(dbtProject.validateSql(request)).rejects.toThrow( + /expected validation error/i, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/suite/dbtProject.test.ts` around lines 742 - 756, Set the mock python validator to reject with a known error and assert that exact error from dbtProject.validateSql; specifically, configure the mock (via mockExecutionInfrastructure or the mocked bridge's validate method) to reject with a descriptive Error (e.g. new Error("validation-failed")) and replace the generic await expect(...).rejects.toBeDefined() with an assertion that the promise rejects with that specific error (e.g. await expect(dbtProject.validateSql(request)).rejects.toThrow("validation-failed")), while keeping the existing expectations that mockExecutionInfrastructure.createPythonBridge and mockExecutionInfrastructure.closePythonBridge were called to verify the bridge lifecycle.
41-72:⚠️ Potential issue | 🟡 MinorFail loudly when upstream dbt-integration exports disappear.
The
actualModule.X || "literal"fallbacks still mask removed/renamed exports, so tests can stay green while production imports break. Prefer explicitundefinedchecks and throw for missing contract values.Suggested pattern
+ const requiredExport = <T>(name: string): T => { + const value = actualModule[name]; + if (value === undefined) { + throw new Error(`Missing export from `@altimateai/dbt-integration`: ${name}`); + } + return value as T; + }; + return { - DBT_PROJECT_FILE: actualModule.DBT_PROJECT_FILE || "dbt_project.yml", - MANIFEST_FILE: actualModule.MANIFEST_FILE || "manifest.json", + DBT_PROJECT_FILE: requiredExport("DBT_PROJECT_FILE"), + MANIFEST_FILE: requiredExport("MANIFEST_FILE"), ... - DBTProjectIntegrationAdapterEvents: - actualModule.DBTProjectIntegrationAdapterEvents || { - SOURCE_FILE_CHANGED: "sourceFileChanged", - MANIFEST_PARSED: "manifestParsed", - RUN_RESULTS_PARSED: "runResultsParsed", - DIAGNOSTICS_CHANGED: "diagnosticsChanged", - PROJECT_CONFIG_CHANGED: "projectConfigChanged", - REBUILD_MANIFEST_STATUS_CHANGE: "rebuildManifestStatusChange", - }, + DBTProjectIntegrationAdapterEvents: requiredExport( + "DBTProjectIntegrationAdapterEvents", + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/suite/dbtProject.test.ts` around lines 41 - 72, Replace the permissive "actualModule.X || fallback" pattern with explicit undefined checks so missing/renamed exports fail loudly: for each exported constant (DBT_PROJECT_FILE, MANIFEST_FILE, RUN_RESULTS_FILE, CATALOG_FILE, RESOURCE_TYPE_*, RESOURCE_TYPE_METRIC, DEFAULT_CONFIGURATION_VALUES, DBTProjectIntegrationAdapterEvents) and the functions validateSQLUsingSqlGlot/validateSQL, assert actualModule.<NAME> !== undefined and throw a clear Error naming the missing export if undefined; otherwise assign the actualModule value (do not silently substitute the string/default), and for DBTProjectIntegrationAdapterEvents validate that it contains the required keys (SOURCE_FILE_CHANGED, MANIFEST_PARSED, RUN_RESULTS_PARSED, DIAGNOSTICS_CHANGED, PROJECT_CONFIG_CHANGED, REBUILD_MANIFEST_STATUS_CHANGE) before using it.
1081-1110:⚠️ Potential issue | 🟡 MinorConfigure the bridge used by
DBTProjectand replace the fixed sleep.Lines 1082-1083 configure a bridge instance before construction, but the constructor gets a new one from the mock implementation. The 100ms timeout is also racy in CI. Queue the configured bridge with
mockReturnValueOnce()and poll for the telemetry call.Suggested deterministic setup
const pythonBridge = mockExecutionInfrastructure.createPythonBridge(); (pythonBridge.ex as any).mockResolvedValue(undefined); + mockExecutionInfrastructure.createPythonBridge.mockReturnValueOnce( + pythonBridge, + ); @@ - // Wait for the async dbt loom check to complete - await new Promise((resolve) => setTimeout(resolve, 100)); + const deadline = Date.now() + 2000; + while ( + !(mockTelemetry.setTelemetryCustomAttribute as jest.Mock).mock.calls.some( + ([key, value]) => + key === "dbtLoomInstalled" && value === "true", + ) && + Date.now() < deadline + ) { + await new Promise((resolve) => setImmediate(resolve)); + } expect(mockTelemetry.setTelemetryCustomAttribute).toHaveBeenCalledWith( "dbtLoomInstalled",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/suite/dbtProject.test.ts` around lines 1081 - 1110, The test is using a configured python bridge but the DBTProject constructor receives a new one and the fixed 100ms sleep is racy; fix by having mockExecutionInfrastructure.createPythonBridge() (or the factory the constructor calls) return your configured bridge via mockReturnValueOnce(...) so DBTProject uses it, and replace the setTimeout with a deterministic poll/wait for the telemetry side effect (i.e., await until mockTelemetry.setTelemetryCustomAttribute("dbtLoomInstalled","true") is observed) instead of sleeping; reference DBTProject, mockExecutionInfrastructure.createPythonBridge, mockReturnValueOnce, and mockTelemetry.setTelemetryCustomAttribute when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/suite/dbtProject.test.ts`:
- Around line 304-309: The afterEach currently clears mocks before disposing
dbtProject which can leak dispose interactions into the next test; make the
afterEach async, await dbtProject.dispose() (if dbtProject exists) before
calling jest.clearAllMocks(), and ensure dispose() is awaited for the symbol
dbtProject.dispose() so mock history is cleared only after cleanup completes.
- Around line 530-615: The tests register listeners with
dbtProject.onSourceFileChanged and dbtProject.onRunResults but only assert debug
logs; update the two specs to assert the registered handlers are actually
invoked: after triggering the integration callback found via
mockProjectIntegration.on (the onCall![1]() and onCall
calls), add expectations that sourceFileChangedHandler was called (and
optionally called with no args or expected payload) and that runResultsHandler
was called with the runResultsData (or at least called once). This ensures the
code paths that call the internal _onSourceFileChanged.fire and
_onRunResults.fire (or the public on* dispatch) are exercised rather than
relying solely on mockTerminal.debug assertions.
---
Duplicate comments:
In `@src/test/suite/dbtProject.test.ts`:
- Around line 742-756: Set the mock python validator to reject with a known
error and assert that exact error from dbtProject.validateSql; specifically,
configure the mock (via mockExecutionInfrastructure or the mocked bridge's
validate method) to reject with a descriptive Error (e.g. new
Error("validation-failed")) and replace the generic await
expect(...).rejects.toBeDefined() with an assertion that the promise rejects
with that specific error (e.g. await
expect(dbtProject.validateSql(request)).rejects.toThrow("validation-failed")),
while keeping the existing expectations that
mockExecutionInfrastructure.createPythonBridge and
mockExecutionInfrastructure.closePythonBridge were called to verify the bridge
lifecycle.
- Around line 41-72: Replace the permissive "actualModule.X || fallback" pattern
with explicit undefined checks so missing/renamed exports fail loudly: for each
exported constant (DBT_PROJECT_FILE, MANIFEST_FILE, RUN_RESULTS_FILE,
CATALOG_FILE, RESOURCE_TYPE_*, RESOURCE_TYPE_METRIC,
DEFAULT_CONFIGURATION_VALUES, DBTProjectIntegrationAdapterEvents) and the
functions validateSQLUsingSqlGlot/validateSQL, assert actualModule.<NAME> !==
undefined and throw a clear Error naming the missing export if undefined;
otherwise assign the actualModule value (do not silently substitute the
string/default), and for DBTProjectIntegrationAdapterEvents validate that it
contains the required keys (SOURCE_FILE_CHANGED, MANIFEST_PARSED,
RUN_RESULTS_PARSED, DIAGNOSTICS_CHANGED, PROJECT_CONFIG_CHANGED,
REBUILD_MANIFEST_STATUS_CHANGE) before using it.
- Around line 1081-1110: The test is using a configured python bridge but the
DBTProject constructor receives a new one and the fixed 100ms sleep is racy; fix
by having mockExecutionInfrastructure.createPythonBridge() (or the factory the
constructor calls) return your configured bridge via mockReturnValueOnce(...) so
DBTProject uses it, and replace the setTimeout with a deterministic poll/wait
for the telemetry side effect (i.e., await until
mockTelemetry.setTelemetryCustomAttribute("dbtLoomInstalled","true") is
observed) instead of sleeping; reference DBTProject,
mockExecutionInfrastructure.createPythonBridge, mockReturnValueOnce, and
mockTelemetry.setTelemetryCustomAttribute when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62e6b6f8-53e9-421f-b4f6-60288fc99674
📒 Files selected for processing (1)
src/test/suite/dbtProject.test.ts
Comment on lines
304
to
+309
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| if (dbtProject) { | ||
| dbtProject.dispose(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Await project cleanup before clearing mocks.
Line 305 clears mocks before Line 307 disposes the project, so disposal calls can leak into the next test’s mock history. If dispose() is async, the current hook also races the next test.
Suggested cleanup order
- afterEach(() => {
- jest.clearAllMocks();
+ afterEach(async () => {
if (dbtProject) {
- dbtProject.dispose();
+ await dbtProject.dispose();
}
+ jest.clearAllMocks();
});📝 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
| afterEach(() => { | |
| jest.clearAllMocks(); | |
| if (dbtProject) { | |
| dbtProject.dispose(); | |
| } | |
| }); | |
| afterEach(async () => { | |
| if (dbtProject) { | |
| await dbtProject.dispose(); | |
| } | |
| jest.clearAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProject.test.ts` around lines 304 - 309, The afterEach
currently clears mocks before disposing dbtProject which can leak dispose
interactions into the next test; make the afterEach async, await
dbtProject.dispose() (if dbtProject exists) before calling jest.clearAllMocks(),
and ensure dispose() is awaited for the symbol dbtProject.dispose() so mock
history is cleared only after cleanup completes.
Comment on lines
+530
to
+615
| it("should handle source file changed events", () => { | ||
| const sourceFileChangedHandler = jest.fn(); | ||
| dbtProject.onSourceFileChanged(sourceFileChangedHandler); | ||
|
|
||
| // Trigger the event from the integration | ||
| const onCall = mockProjectIntegration.on.mock.calls.find( | ||
| (call: any) => | ||
| call[0] === DBTProjectIntegrationAdapterEvents.SOURCE_FILE_CHANGED, | ||
| ); | ||
| onCall![1](); // Call the handler | ||
|
|
||
| expect(mockTerminal.debug).toHaveBeenCalledWith( | ||
| "DBTProject", | ||
| "Received sourceFileChanged event from Node.js file watchers", | ||
| ); | ||
| }); | ||
|
|
||
| it("should handle manifest parsed events", () => { | ||
| const parsedManifest: ParsedManifest = { | ||
| nodeMetaMap: { | ||
| lookupByBaseName: (() => undefined) as any, | ||
| lookupByUniqueId: (() => undefined) as any, | ||
| nodes: (() => []) as any, | ||
| }, | ||
| macroMetaMap: new Map(), | ||
| metricMetaMap: new Map(), | ||
| sourceMetaMap: new Map(), | ||
| graphMetaMap: { | ||
| parents: new Map(), | ||
| children: new Map(), | ||
| tests: new Map(), | ||
| metrics: new Map(), | ||
| }, | ||
| testMetaMap: new Map(), | ||
| docMetaMap: new Map(), | ||
| exposureMetaMap: new Map(), | ||
| modelDepthMap: new Map(), | ||
| functionMetaMap: new Map(), | ||
| }; | ||
|
|
||
| // Trigger the event from the integration | ||
| const onCall = mockProjectIntegration.on.mock.calls.find( | ||
| (call: any) => | ||
| call[0] === DBTProjectIntegrationAdapterEvents.MANIFEST_PARSED, | ||
| ); | ||
| onCall; | ||
|
|
||
| expect(mockManifestChangedEmitter.fire).toHaveBeenCalledWith({ | ||
| added: [ | ||
| expect.objectContaining({ | ||
| project: dbtProject, | ||
| ...parsedManifest, | ||
| }), | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("should handle run results parsed events", () => { | ||
| const runResultsData: RunResultsEventData = { | ||
| id: "run-1", | ||
| command: "dbt run", | ||
| args: [], | ||
| completedAt: new Date(), | ||
| projectName: "test-project", | ||
| elapsedTime: 0, | ||
| results: [ | ||
| { uniqueId: "model.test.model1" } as any, | ||
| { uniqueId: "model.test.model2" } as any, | ||
| ], | ||
| }; | ||
|
|
||
| const runResultsHandler = jest.fn(); | ||
| dbtProject.onRunResults(runResultsHandler); | ||
|
|
||
| // Trigger the event from the integration | ||
| const onCall = mockProjectIntegration.on.mock.calls.find( | ||
| (call: any) => | ||
| call[0] === DBTProjectIntegrationAdapterEvents.RUN_RESULTS_PARSED, | ||
| ); | ||
| onCall; | ||
|
|
||
| expect(mockTerminal.debug).toHaveBeenCalledWith( | ||
| "DBTProject", | ||
| "Received runResultsParsed event from dbtIntegrationAdapter", | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Assert the event consumers, not just debug logs.
These tests register sourceFileChangedHandler and runResultsHandler, but they never assert those listeners fire. A regression that removes _onSourceFileChanged.fire() or _onRunResults.fire() would still pass.
Suggested assertions
onCall![1](); // Call the handler
+ expect(sourceFileChangedHandler).toHaveBeenCalledTimes(1);
expect(mockTerminal.debug).toHaveBeenCalledWith(
"DBTProject",
"Received sourceFileChanged event from Node.js file watchers",
);
@@
onCall;
+ expect(mockRunHistoryService.addEntry).toHaveBeenCalledWith(
+ runResultsData,
+ );
+ expect(runResultsHandler).toHaveBeenCalledWith(
+ expect.objectContaining({
+ project: dbtProject,
+ id: "run-1",
+ command: "dbt run",
+ }),
+ );
expect(mockTerminal.debug).toHaveBeenCalledWith(
"DBTProject",
"Received runResultsParsed event from dbtIntegrationAdapter",
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/suite/dbtProject.test.ts` around lines 530 - 615, The tests register
listeners with dbtProject.onSourceFileChanged and dbtProject.onRunResults but
only assert debug logs; update the two specs to assert the registered handlers
are actually invoked: after triggering the integration callback found via
mockProjectIntegration.on (the onCall![1]() and onCall
calls), add expectations that sourceFileChangedHandler was called (and
optionally called with no args or expected payload) and that runResultsHandler
was called with the runResultsData (or at least called once). This ensures the
code paths that call the internal _onSourceFileChanged.fire and
_onRunResults.fire (or the public on* dispatch) are exercised rather than
relying solely on mockTerminal.debug assertions.
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.
Overview
Problem
Describe the problem you are solving. Mention the ticket/issue if applicable.
Solution
Describe the implemented solution. Add external references if needed.
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Add comprehensive integration tests for DBT project and container with extensive mocking and validation of functionalities.
jest.config.jswith new reporter configuration.vscodeand@altimateai/dbt-integrationindbtProject.test.tsanddbtProjectContainer.test.ts.setup.ts.vscodeAPI inmock/vscode.tswith functions likeUri.file,window.withProgress, andworkspace.getConfiguration.DBTProjectIntegrationAdapterEventsandNoCredentialsErrorindbtProject.test.ts.DBTProjectindbtProject.test.tscovering initialization, configuration, event handling, diagnostics, model operations, query execution, healthcheck, catalog operations, and disposal.DBTProjectContainerindbtProjectContainer.test.tscovering initialization, error handling, project management, SQL operations, model operations, state management, Altimate Datapilot integration, workspace folder management, and edge cases.This description was created by
for fbace47. You can customize this summary. It will automatically update as commits are pushed.
✨ PR Description
Purpose: Add comprehensive integration test suite for DBT project functionality to improve test coverage and validate core DBT operations including project management, SQL execution, and diagnostics.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how
Summary by CodeRabbit