fix: langchain and remove failing test check #1187
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| @@ -34,7 +34,7 @@ export class LangchainToolSet extends BaseComposioToolSet { | |||
| super({ | |||
| apiKey: config.apiKey || null, | |||
| baseUrl: config.baseUrl || COMPOSIO_BASE_URL, | |||
| runtime: config?.runtime || null, | |||
| runtime: config?.runtime || LangchainToolSet.FRAMEWORK_NAME, | |||
There was a problem hiding this comment.
The default runtime should not be hardcoded to the framework name. This prevents users from omitting the runtime parameter when initializing the LangchainToolSet.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| runtime: config?.runtime || LangchainToolSet.FRAMEWORK_NAME, | |
| runtime: config?.runtime || this.FRAMEWORK_NAME, |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 0552109 in 9 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/base.toolset.spec.ts:159
- Draft comment:
Ensure that the test case is passing after removing the.failingmodifier. If the test is still failing, investigate the cause before removing the modifier. - Reason this comment was not posted:
Confidence changes required:50%
The change in the test file removes the 'failing' modifier from a test case. This suggests that the test is expected to pass now. However, the test case should be verified to ensure it is indeed passing and that the change is intentional.
Workflow ID: wflow_gG3Kv60OoxDHRwPo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| @@ -34,7 +34,7 @@ export class LangchainToolSet extends BaseComposioToolSet { | |||
| super({ | |||
| apiKey: config.apiKey || null, | |||
| baseUrl: config.baseUrl || COMPOSIO_BASE_URL, | |||
| runtime: config?.runtime || null, | |||
| runtime: config?.runtime || LangchainToolSet.FRAMEWORK_NAME, | |||
There was a problem hiding this comment.
Good change to set LangchainToolSet.FRAMEWORK_NAME as default runtime. Consider adding a comment explaining why this default is important for proper framework identification during tool initialization.
| @@ -156,7 +156,7 @@ describe("ComposioToolSet class tests", () => { | |||
| expect(executionResultAfterRemove.data.title).toBe("Test issue"); | |||
| }); | |||
|
|
|||
| it.failing("should execute an file upload", async () => { | |||
| it("should execute an file upload", async () => { | |||
There was a problem hiding this comment.
There's a grammatical error in the test description: "an file upload" should be "a file upload". Also, since this test is no longer marked as failing, it would be helpful to add a comment explaining what changes made it pass.
Code Review SummaryChanges Overview
Code Quality Assessment
Suggestions
Overall, the changes are well-justified and improve the codebase. The PR can be merged after addressing the minor documentation and grammar issues. |
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
Important
Update default
runtimeinLangchainToolSetand remove failing test check inbase.toolset.spec.ts.runtimetoLangchainToolSet.FRAMEWORK_NAMEinlangchain.ts..failingfrom "should execute an file upload" test inbase.toolset.spec.ts.This description was created by
for 0552109. It will automatically update as commits are pushed.