Chore/dbt-integration-tests#1718
Chore/dbt-integration-tests#1718saravmajestic wants to merge 4 commits intofeat/extract-dbt-projectfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 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}
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)); |
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.
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( |
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; |
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, |
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.
| (newContainer as any)._onProjectRegisteredUnregistered, | |
| newContainer.onProjectRegisteredUnregistered, |
| @@ -1,4 +1,7 @@ | |||
| import { DBTTerminal, EnvironmentVariables } from "@altimateai/dbt-integration"; | |||
| import { | |||
| DataPilotHealtCheckParams, | |||
There was a problem hiding this comment.
Typographical error: it appears 'DataPilotHealtCheckParams' is intended to be 'DataPilotHealthCheckParams'. Please fix the typo.
| DataPilotHealtCheckParams, | |
| DataPilotHealthCheckParams, |
|
@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}
| 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
I will take a loot early next week |
![linearb[bot]](https://avatars.githubusercontent.com/in/1658443?s=60&v=4){kind=small}
2 participants
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