chore: onboard a utility for getting the test data for manual testing #3092
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent update introduces a more flexible testing environment by allowing the selection of operation modes and improving file management. Specifically, it adds a new command-line option to specify the operation mode for integration tests, enabling conditional logic for test data handling. Additionally, the update enhances the project's organization by ignoring temporary files in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai revie |
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3092/test-report.html |
|
@utsabc: It seems like you're requesting a review of the pull request. I'll proceed with reviewing the code changes. Please note that I cannot directly access external URLs, such as the test report you mentioned, but I will do my best to provide a thorough review based on the code changes. If there's anything specific you'd like me to focus on, please let me know. Action performedReview triggered.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3092 +/- ##
========================================
Coverage 87.21% 87.21%
========================================
Files 537 537
Lines 29243 29243
Branches 6962 6962
========================================
Hits 25503 25503
Misses 3393 3393
Partials 347 347 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
test/integrations/component.test.ts
Outdated
| @@ -98,6 +99,7 @@ | |||
| registerAxiosMocks(allAxiosRequests); | |||
| } | |||
|
|
|||
| const mode = opts.mode || 'active'; | |||
There was a problem hiding this comment.
The default value for mode is set here, but it could be more efficiently handled directly in the command option definition to ensure opts.mode always has a valid value.
Consider moving the default value assignment to the command option definition to simplify the code and avoid redundant checks.
test/integrations/component.test.ts
Outdated
| if (mode === 'passive') { | ||
| // print the test data | ||
| const result: any = []; | ||
| testData.forEach((tcData) => { | ||
| let events; | ||
| switch (tcData.feature) { | ||
| case tags.FEATURES.PROCESSOR: | ||
| events = tcData.input.request.body; | ||
| break; | ||
| case tags.MODULES.SOURCE: | ||
| await sourceTestHandler(tcData); | ||
| case tags.FEATURES.BATCH: | ||
| events = tcData.input.request.body.input; | ||
| break; | ||
| default: | ||
| console.log('Invalid module'); | ||
| // Intentionally fail the test case | ||
| expect(true).toEqual(false); | ||
| case tags.FEATURES.ROUTER: | ||
| events = tcData.input.request.body.input; | ||
| break; | ||
| } | ||
|
|
||
| events.map((event) => { | ||
| result.push(event.message); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('data generated successfully', () => { | ||
| // write the data to a file | ||
|
|
||
| // create directory if not exists | ||
| const dir = join(__dirname, '../../temp'); | ||
| if (!require('fs').existsSync(dir)) { | ||
| require('fs').mkdirSync(dir); | ||
| } | ||
| appendFileSync(join(__dirname, '../../temp/test_data.json'), JSON.stringify(result, null, 2)); | ||
| console.log('Data generated successfully at temp/test_data.json'); | ||
| expect(true).toEqual(true); // Intentionally pass the test case | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
The passive mode block generates test data and writes it to a file. However, there's a potential issue with the handling of the events variable. It's declared inside a switch statement and used outside of it, which could lead to reference errors if none of the cases match.
Ensure events is initialized before the switch statement to avoid reference errors. Also, consider handling the case where tcData.feature does not match any case, to avoid pushing undefined into result.
test/integrations/component.test.ts
Outdated
| // write the data to a file | ||
|
|
||
| // create directory if not exists | ||
| const dir = join(__dirname, '../../temp'); | ||
| if (!require('fs').existsSync(dir)) { | ||
| require('fs').mkdirSync(dir); | ||
| } | ||
| appendFileSync(join(__dirname, '../../temp/test_data.json'), JSON.stringify(result, null, 2)); | ||
| console.log('Data generated successfully at temp/test_data.json'); | ||
| expect(true).toEqual(true); // Intentionally pass the test case | ||
| }); |
There was a problem hiding this comment.
Writing test data to a file is done within a test case. This approach mixes test data generation with test execution, which might not be ideal for readability and maintainability.
Consider separating test data generation from test assertions. Use a separate function or script for generating and writing test data to ensure clear separation of concerns.
There was a problem hiding this comment.
should we write some other util for generation based on this comment? @utsabc
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
test/integrations/component.test.ts
Outdated
| if (!require('fs').existsSync(dir)) { | ||
| require('fs').mkdirSync(dir); |
There was a problem hiding this comment.
Using require('fs') directly within the test case for checking directory existence and creating a directory. This approach is not efficient as it does not leverage asynchronous file system operations and repeats the require('fs') call.
Extract fs module usage to the top of the file and use asynchronous versions of existsSync and mkdirSync to improve efficiency and code readability.
There was a problem hiding this comment.
I was talking about this in my earlier comment
There was a problem hiding this comment.
@koladilip: I understand there might have been a misunderstanding or a missed context regarding the use of require('fs') within the test case. Could you please provide more details or clarify the concern you mentioned in your earlier comment? I'm here to help and ensure the review is as accurate and helpful as possible.
There was a problem hiding this comment.
basically, I was also saying that @utsabc should put existsSync and mkdirSync in top imports
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
test/integrations/component.test.ts
Outdated
| describe(`${testData[0].name} ${testData[0].module}`, () => { | ||
| test.each(testData)('$feature -> $description', async (tcData) => { | ||
| tcData?.mockFns?.(mockAdapter); | ||
|
|
||
| switch (tcData.module) { | ||
| case tags.MODULES.DESTINATION: | ||
| await destinationTestHandler(tcData); | ||
| break; | ||
| case tags.MODULES.SOURCE: | ||
| await sourceTestHandler(tcData); | ||
| break; | ||
| default: | ||
| console.log('Invalid module'); | ||
| // Intentionally fail the test case | ||
| expect(true).toEqual(false); | ||
| break; | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The active mode block executes test handlers based on the module type. However, there's a lack of error handling for asynchronous operations within the test cases.
Add try-catch blocks around asynchronous operations to handle potential errors gracefully and provide meaningful error messages for test failures.
|
Test report for this run is available at: https://test-integrations-dev.s3.amazonaws.com/integrations-test-reports/rudder-transformer/3092/test-report.html |
|
What are the changes introduced in this PR?
Added utility to get test data for manual testing
What is the related Linear task?
Resolves INT-1567
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
.gitignoreto exclude temporary files, improving file management.