feat: update base toolset testcases #1206
Conversation
abhishekpatil4
requested review from
himanshu-dixit,
plxity and
utkarsh-dixit
as code owners
|
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}
| const res = await executeRequest({ | ||
| endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | ||
| method: "PUT", | ||
| parameters: [], | ||
| }); | ||
| return res; |
There was a problem hiding this comment.
Lack of error handling for non-200 responses may lead to silent failures. Implement error handling for robustness.
📝 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.
| const res = await executeRequest({ | |
| endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | |
| method: "PUT", | |
| parameters: [], | |
| }); | |
| return res; | |
| const res = await executeRequest({ | |
| endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | |
| method: "PUT", | |
| parameters: [], | |
| }); | |
| if (!res.ok) { | |
| throw new Error(`Failed to star repository: ${res.status} ${res.statusText}`); | |
| } | |
| return res; |
js/src/sdk/base.toolset.spec.ts
Outdated
| const appId = "01e22f33-dc3f-46ae-b58d-050e4d2d1909"; | ||
| const integration = await toolset.integrations.create({ | ||
| name: "testIntegration", | ||
| authScheme: "OAUTH2", | ||
| appId: appId, | ||
| useComposioAuth: true | ||
| }); |
There was a problem hiding this comment.
Hard-coded app ID reduces maintainability. Move to config or constants for better management.
📝 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.
| const appId = "01e22f33-dc3f-46ae-b58d-050e4d2d1909"; | |
| const integration = await toolset.integrations.create({ | |
| name: "testIntegration", | |
| authScheme: "OAUTH2", | |
| appId: appId, | |
| useComposioAuth: true | |
| }); | |
| const appId = process.env.APP_ID || require('../config/constants').APP_ID; | |
| const integration = await toolset.integrations.create({ | |
| name: "testIntegration", | |
| authScheme: "OAUTH2", | |
| appId: appId, | |
| useComposioAuth: true | |
| }); |
![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 59e7483 in 48 seconds
More details
- Looked at
225lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/base.toolset.spec.ts:243
- Draft comment:
Typo: The propertysuccessfullis misspelled. It should besuccessful. This typo is present in multiple places (lines 184, 243). - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_Ap09DIJWxi1WIZuZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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: |
js/src/sdk/base.toolset.spec.ts
Outdated
| ).toBeDefined(); | ||
| const imageResponse = await fetch("https://composio.dev/wp-content/uploads/2024/07/Composio-Logo.webp"); | ||
| const arrayBuffer = await imageResponse.arrayBuffer(); | ||
| const base64Image = Buffer.from(arrayBuffer).toString('base64'); |
There was a problem hiding this comment.
Consider adding error handling for the fetch and arrayBuffer operations. If the image fetch fails or the buffer conversion fails, the test will fail without a clear error message. Consider wrapping this in a try-catch block with appropriate error handling.
js/src/sdk/base.toolset.spec.ts
Outdated
|
|
||
| it("should setup a trigger", async () => { | ||
| const trigger = await toolset.triggers.setup({ | ||
| connectedAccountId: "d7bb2a05-22c2-41d7-bf3c-b4c4e7c6c46e", |
There was a problem hiding this comment.
Consider moving hardcoded IDs like this to a test constants file. This would make the tests more maintainable and easier to update if these IDs change.
js/src/sdk/base.toolset.spec.ts
Outdated
| it("should throw error if connected account id is invalid in get method", async () => { | ||
| await expect(toolset.connectedAccounts.get({ | ||
| connectedAccountId: "invalid-id" | ||
| })).rejects.toThrow(); |
There was a problem hiding this comment.
The error handling here could be more specific. Instead of just expecting any error to be thrown, consider checking for a specific error type or message that would be thrown for an invalid account ID. This would make the test more robust against unintended errors.
| // Integration tests | ||
| let createdIntegrationId: string; | ||
|
|
||
| it("should create an integration", async () => { |
There was a problem hiding this comment.
Consider adding a timeout to this test case. Integration tests that involve external services should have reasonable timeouts to prevent the test suite from hanging if the service is slow or unresponsive.
Code Review SummaryOverall, this is a solid PR that significantly improves the test coverage of the codebase. The changes are well-structured and add important test cases for core functionality. Strengths:
Areas for Improvement:
The changes represent a significant improvement in test coverage and quality. With the suggested improvements implemented, this would be an excellent addition to the codebase. Rating: 8/10 - Strong improvements with minor enhancements needed. |
|
@abhishekpatil4 Some of the tests are failing. Please check that |
| // recipient_email: "[email protected]", | ||
| // subject: "Test email from himanshu", |
There was a problem hiding this comment.
Hardcoded email addresses in test cases pose a security risk and could lead to spam/abuse. Should use test email domains or environment variables.
📝 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.
| // recipient_email: "[email protected]", | |
| // subject: "Test email from himanshu", | |
| recipient_email: "[email protected]" |
| // "https://composio.dev/wp-content/uploads/2024/07/Composio-Logo.webp", | ||
| "/Users/abhishek/Desktop/randomimage.png", |
There was a problem hiding this comment.
Using local file path in tests makes them environment-dependent and likely to fail on other systems. Should use a test fixture or mock file.
js/src/sdk/base.toolset.spec.ts
Outdated
| owner: "abhishekpatil4", | ||
| repo: "tweetify", |
There was a problem hiding this comment.
Hardcoded repository owner/name in trigger test makes it brittle. Should use test config values or environment variables.
📝 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.
| owner: "abhishekpatil4", | |
| repo: "tweetify", | |
| owner: process.env.GITHUB_OWNER || "abhishekpatil4", | |
| repo: process.env.GITHUB_REPO || "tweetify" |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8e8a521 in 42 seconds
More details
- Looked at
255lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/src/sdk/base.toolset.spec.ts:2
- Draft comment:
Duplicate import of 'zod'. Remove the redundant import statement. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. js/src/sdk/base.toolset.spec.ts:160
- Draft comment:
Typo in property name 'successfull'. It should be 'successful'. This issue is also present on lines 218 and 277. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_U9njrn2IXPZ4tIN3
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Co-authored-by: Himanshu Dixit <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c63357e in 52 seconds
More details
- Looked at
86lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js/src/sdk/base.toolset.spec.ts:417
- Draft comment:
Ensure that the correct parameter 'triggerInstanceId' is used instead of 'triggerId' if required by the method. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_sPM6akt4zqusDVBj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fce27db in 53 seconds
More details
- Looked at
56lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. js/src/sdk/base.toolset.spec.ts:443
- Draft comment:
Consider logging the error before rethrowing it for better debugging. - Reason this comment was not posted:
Confidence changes required:50%
The use ofthrow errorin the catch blocks is not ideal for logging purposes. It would be better to log the error before rethrowing it.
2. js/src/sdk/base.toolset.spec.ts:446
- Draft comment:
Consider logging the error before rethrowing it for better debugging. - Reason this comment was not posted:
Confidence changes required:50%
The use ofthrow errorin the catch blocks is not ideal for logging purposes. It would be better to log the error before rethrowing it.
3. js/src/sdk/base.toolset.spec.ts:454
- Draft comment:
Consider logging the error before rethrowing it for better debugging. - Reason this comment was not posted:
Confidence changes required:50%
The use ofthrow errorin the catch blocks is not ideal for logging purposes. It would be better to log the error before rethrowing it.
Workflow ID: wflow_S1u3TaSjqgK7QlQy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7fe5dc7 in 2 minutes and 42 seconds
More details
- Looked at
55lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. js/config/getTestConfig.ts:1
- Draft comment:
Changing the default environment from "staging" to "prod" can have significant implications. Ensure this change is intentional and that all tests are safe to run in a production environment. - Reason this comment was not posted:
Comment did not seem useful.
2. js/src/sdk/base.toolset.spec.ts:438
- Draft comment:
Consider logging the error in the catch block to aid in debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
This is a test cleanup section where errors are intentionally ignored for individual items. The code continues trying to clean up other resources even if one fails. There's a second catch block that will throw any other errors. The empty catch is a deliberate pattern here, not an oversight. Adding logging would just create noise in test output.
The comment could be valid if this was production code where we'd want visibility into failures. Test cleanup is a special case.
While logging errors is generally good practice, in test cleanup code it's common and acceptable to silently ignore certain failures since they don't affect the test results.
The comment should be deleted. The empty catch block is intentional in test cleanup code and adding error logging would not improve anything.
Workflow ID: wflow_XkND2UKeCc39Gu6M
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Add new Python agents and setup scripts for Solana and Coinbase cryptocurrency operations, including wallet management and automated rewards.
coinbase_agent.mdx,community_reward_agent.mdx,github_contributor_reward_agent.mdxfor Coinbase and community reward agents.solana_kit.pyfor balance checks, token transfers, and transaction status.crypto_assistant.py,coder_reward_agent.py,community_reward_agent.pyfor automated reward distribution.setup.shscripts for environment setup in Coinbase and Solana examples.README.mdfor Solana agents with installation and usage instructions.examples_js.ymlandrun_examples_js.ymlto include new API keys and paths.package.jsondependencies for JavaScript examples.docs/cryptokit/for tools and agents.This description was created by
for 7fe5dc7. It will automatically update as commits are pushed.