feat: add test for combine tools script #3136
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3136--asyncapi-website.netlify.app/ |
| @@ -106,7 +105,7 @@ const getFinalTool = async (toolObject) => { | |||
|
|
|||
| // Combine the automated tools and manual tools list into single JSON object file, and | |||
| // lists down all the language and technology tags in one JSON file. | |||
| const combineTools = async (automatedTools, manualTools) => { | |||
| const combineTools = async (automatedTools, manualTools, toolsPath, tagsPath) => { | |||
There was a problem hiding this comment.
what does tagspath signify??
| JSON.stringify({ languages: languageList, technologies: technologyList }), | ||
| ) | ||
| fs.writeFileSync(toolsPath,JSON.stringify(finalTools)); | ||
| fs.writeFileSync(tagsPath,JSON.stringify({ languages: languageList, technologies: technologyList }),) | ||
| } | ||
|
|
||
| module.exports = { combineTools } |
tests/tools/combine-tools.test.js
Outdated
| const tagsData = readJSON(tagsPath); | ||
| expect(tagsData.languages).toContainEqual(expect.objectContaining({ name: 'NewLanguage' })); | ||
| }); | ||
| }); |
anshgoyalevil
added
the
gsoc
scripts/build-tools.js
Outdated
| @@ -14,7 +14,7 @@ const buildTools = async () => { | |||
| resolve(__dirname, '../config', 'tools-automated.json'), | |||
| JSON.stringify(automatedTools, null, ' ') | |||
| ); | |||
| await combineTools(automatedTools, manualTools); | |||
| await combineTools(automatedTools, manualTools, resolve(__dirname, '../../config', 'tools.json'), resolve(__dirname, '../../config', 'all-tags.json')); | |||
There was a problem hiding this comment.
Can we do it in a more better way, instead of passing filePath here? Try to refactor tools scripts instead of manually passing filePaths for tags file.
| }); | ||
|
|
||
| it('should combine tools and create correct JSON files', async () => { | ||
| await combineTools(automatedTools, manualTools, toolsPath, tagsPath); |
There was a problem hiding this comment.
try to add this function call in a try/catch block here to monitor if we are receiving any error or not.
| }); | ||
|
|
||
|
|
||
| it('should log validation errors to console.error', async () => { |
There was a problem hiding this comment.
Errors shouldn't be validated via console messages. You should formally return a error using Promise.reject.
scripts/build-tools.js
Outdated
| @@ -6,15 +6,19 @@ const manualTools = require('../config/tools-manual.json') | |||
| const fs = require('fs'); | |||
| const { resolve } = require('path'); | |||
|
|
|||
| let toolsPath = resolve(__dirname, '../../config', 'tools.json') | |||
There was a problem hiding this comment.
is using let a good option here?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scripts/build-tools.js (1)
Line range hint
9-11: Consider using 'const' instead of 'let'The variables
githubExtractDataandautomatedToolsare declared usinglet, but they are not reassigned after their initial assignment. To improve code clarity and prevent accidental reassignment, consider usingconstinstead:- let githubExtractData = await getData(); - let automatedTools = await convertTools(githubExtractData); + const githubExtractData = await getData(); + const automatedTools = await convertTools(githubExtractData);This change aligns with best practices for variable declaration in JavaScript and makes the code's intent clearer.
tests/build-tools.test.js (2)
Line range hint
1-89: Consider enhancing test coverageWhile the existing tests cover the main scenarios, consider adding the following to improve coverage:
- Test with empty input files.
- Test with malformed JSON in input files.
- Test the behavior when the
categoryListis empty or contains unexpected data.- Verify the structure and content of the output files more thoroughly.
These additional tests would help ensure the robustness of the
buildToolsfunction across various edge cases.
3-5: Improve visibility of mock dataWhile using mock data from external files is a good practice for keeping tests clean, it can sometimes make it harder to understand the tests at a glance. Consider adding brief comments or examples of the mock data structure directly in this test file. This would make it easier for developers to understand the test scenarios without having to switch between multiple files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/build-tools.js (2 hunks)
- tests/build-tools.test.js (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
scripts/build-tools.js (1)
25-28: 🛠️ Refactor suggestionConsider the implications of changing from
path.resolve()topath.join()The switch from
path.resolve()topath.join()might alter how paths are constructed. Whilepath.resolve()always returns an absolute path,path.join()simply concatenates path segments. This change could make the paths relative to the current working directory instead of absolute paths.Please verify that this change doesn't negatively impact how these paths are used in other parts of the codebase. If absolute paths are required, consider using
path.resolve(__dirname, ...)instead.To verify the impact of this change, run the following script:
Additionally, as per a previous review comment, consider refactoring the tools scripts to avoid manually passing file paths. This could involve creating a configuration file or using environment variables to manage these paths centrally.
✅ Verification successful
Path resolution change verified; no impact detected
The change from
path.resolve()topath.join()inscripts/build-tools.jshas been verified. Searches across the codebase show thatautomatedToolsPath,manualToolsPath,toolsPath, andtagsPathare used consistently within their modules and tests, with no negative impacts detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of these file paths in other parts of the codebase # Test: Search for usage of these file paths echo "Searching for usage of automatedToolsPath:" rg "automatedToolsPath" --type js echo "Searching for usage of manualToolsPath:" rg "manualToolsPath" --type js echo "Searching for usage of toolsPath:" rg "toolsPath" --type js echo "Searching for usage of tagsPath:" rg "tagsPath" --type jsLength of output: 9115
tests/build-tools.test.js (3)
27-31: Improved path handling withpath.join()The change from
path.resolve()topath.join()for constructing file paths is a good improvement.path.join()is more predictable and safer for cross-platform compatibility, as it doesn't resolve to an absolute path likeresolve()might in some cases. This change aligns with best practices for path manipulation in Node.js.
81-81: Consistent path handling for invalid pathThe update to use
path.join()for constructing theinvalidPathis consistent with the earlier changes. This maintains the test's intention of creating an invalid path while aligning with the improved path handling approach.
Line range hint
1-89: Overall assessment: Good improvements with room for enhancementThe changes to path handling using
path.join()are positive and improve cross-platform compatibility. The overall test structure remains robust, covering main scenarios effectively. To further enhance this test suite, consider:
- Implementing the suggested additional test cases for better coverage.
- Improving the visibility of mock data structures within the test file.
- Adding more detailed assertions to verify the structure and content of output files.
These enhancements would contribute to a more comprehensive and maintainable test suite.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/tools/combine-tools.test.js (1)
30-32: Remove unused mock for 'ajv-formats'The mock for
'ajv-formats'is defined but not utilized in the tests. Removing this unused mock will clean up the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- tests/build-tools.test.js (1 hunks)
- tests/tools/combine-tools.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-tools.test.js
🧰 Additional context used
🔇 Additional comments (18)
tests/tools/combine-tools.test.js (18)
1-22: Module imports and data setups are well-structuredThe required modules and fixtures are correctly imported, providing a solid foundation for the tests.
34-43: Mocks for 'tags-color' are correctly set upThe color configurations for languages and technologies are well-defined, aiding in the consistency of test data.
45-50: 'categoryList' mock is properly configuredThe mocked categories provide a clear structure for testing category-related functionalities.
52-52: Efficient JSON reading withreadJSONhelperThe
readJSONfunction enhances code readability and reuse by encapsulating JSON file reading.
54-76: Test initialization and cleanup are properly handledUsing
beforeAllandafterAllensures that tests are isolated and the environment is clean, preventing side effects between tests.
78-88: Comprehensive test for combining tools and generating JSON filesThe test correctly verifies that the
combineToolsfunction creates the expected files with the right properties.
90-96: Correct handling of tools with missing dataThe test ensures that tools lacking language or technology information are still processed appropriately.
98-105: Tools are sorted alphabetically as expectedVerifying the alphabetical sorting of tools by title confirms that the sorting functionality works correctly.
107-120: Validation errors are effectively loggedThe test successfully checks that validation errors are captured and logged, aiding in debugging and error tracking.
122-136: Proper handling of multiple and new languagesThe test confirms that tools with multiple languages, including new ones, are processed, and tags are updated accordingly.
138-162: New languages and technologies are added correctlyThe test ensures that unfamiliar languages and technologies are added to the tags, maintaining the integrity of the tag system.
164-180: Addition of new languages is handled properlyBy testing with a new language, the code demonstrates flexibility in accommodating growth in the list of supported languages.
182-198: Valid tool objects are processed accuratelyThe test validates that tools with complete and correct data are handled as expected, reinforcing confidence in the normal operation of
combineTools.
200-208: Handling tools withoutrepoUrlgracefullyThe absence of
repoUrldoesn't cause issues, and the test confirms that theisAsyncAPIrepoproperty is managed correctly.
210-221: Refactor error handling using Jest's.rejects.toThrow()The previous review comment suggesting refactoring this test to use
.rejects.toThrow()instead of a try/catch block is still applicable.
223-233: Simplify error assertions with Jest's.rejects.toThrow()The prior suggestion to use
.rejects.toThrow()for cleaner error handling remains valid for this test case.
235-245: Use.rejects.toThrow()for clearer asynchronous error testsThe earlier recommendation to refactor this test using Jest's
.rejects.toThrow()method is still pertinent.
247-258: Refactor to simplify error handling with JestThe test can be streamlined by adopting
.rejects.toThrow()as previously suggested.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
scripts/build-newsroom-videos.js (2)
7-10: LGTM: Directory creation logic added.The new code block effectively ensures that the target directory exists before attempting to write the file. This prevents potential errors and improves the robustness of the script.
Consider adding error handling for the
mkdirSynccall:if (!existsSync(dir)) { try { mkdirSync(dir, { recursive: true }); } catch (err) { throw new Error(`Failed to create directory: ${err.message}`); } }This will provide more specific error information if directory creation fails.
1-10: Overall assessment: Improved robustness without affecting core functionality.The changes in this file focus on ensuring the target directory exists before writing the output file. This improvement adds robustness to the script without altering its core functionality of fetching and processing YouTube video data.
The error handling for the YouTube API interaction remains intact, maintaining the script's reliability for its primary purpose.
Consider adding a config file or environment variable for the output directory path. This would make the script more flexible and easier to configure in different environments.
tests/build-newsroom-videos.test.js (2)
18-20: LGTM: Improved error handling in cleanup.The addition of the existence check before removing the test directory is a good practice. It prevents potential errors if the directory doesn't exist.
Consider using the
fs.promisesAPI for asynchronous file system operations, which can lead to more consistent and easier-to-maintain code:const fs = require('fs').promises; afterAll(async () => { try { await fs.rm(testDir, { recursive: true, force: true }); } catch (error) { if (error.code !== 'ENOENT') { throw error; } } });This approach handles the directory removal asynchronously and catches any errors, only re-throwing if it's not a "directory not found" error.
24-26: LGTM: Improved directory handling in test setup.The addition of the existence check before creating the test directory is a good practice. It prevents unnecessary directory creation attempts and potential errors.
Consider using the
fs.promisesAPI for asynchronous file system operations:const fs = require('fs').promises; beforeEach(async () => { await fs.mkdir(testDir, { recursive: true }); fetch.mockClear(); });This approach handles the directory creation asynchronously and the
recursive: trueoption automatically creates parent directories if they don't exist, eliminating the need for an explicit existence check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- scripts/build-newsroom-videos.js (1 hunks)
- scripts/build-tools.js (1 hunks)
- tests/build-newsroom-videos.test.js (4 hunks)
- tests/build-tools.test.js (2 hunks)
- tests/tools/combine-tools.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/build-tools.js
- tests/build-tools.test.js
- tests/tools/combine-tools.test.js
🧰 Additional context used
🔇 Additional comments (3)
scripts/build-newsroom-videos.js (1)
1-2: LGTM: Necessary imports added for new functionality.The additional imports (
mkdirSync,existsSync, anddirname) are correctly added and align with the new directory creation logic introduced in the file.tests/build-newsroom-videos.test.js (2)
1-1: LGTM: Appropriate import added for directory existence check.The addition of
existsSyncto the imports is necessary and aligns with the new directory existence checks implemented in the test suite. This change improves the robustness of the tests.
Line range hint
1-95: Overall assessment: Good improvements with minor suggestions.The changes in this file generally improve the robustness of the test suite by adding necessary checks for directory existence. Here's a summary of the review:
- The addition of
existsSyncimport is appropriate.- The
afterAllandbeforeEachhooks now handle directory operations more safely.- There's a redundant directory check in one of the test cases that should be addressed.
Consider the suggestions for using
fs.promisesAPI for more consistent asynchronous file operations. These changes will further enhance the quality and maintainability of the test suite.
…ebsite into combineToolsTest
| languages: [ | ||
| { | ||
| name: 'JavaScript', | ||
| color: 'bg-[#57f281]', | ||
| borderColor: 'border-[#37f069]' | ||
| }, | ||
| { | ||
| name: 'Python', | ||
| color: 'bg-[#3572A5]', | ||
| borderColor: 'border-[#3572A5]' | ||
| } | ||
| ], |
There was a problem hiding this comment.
Can we directly use tagsData instead of creating this duplicate data?
|
|
||
| consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Why aren't we using resolveAllMocks() function here in beforeEach function?
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (3)
tests/fixtures/tools/automated-tools.json (1)
1-17: LGTM! Consider enhancing test data coverage.The JSON structure is well-formed and provides a good basic test case. However, since this is test data, consider adding more diverse scenarios to improve test coverage:
- Multiple tools in the category
- Tools with multiple technologies
- Tools with missing optional fields
Example addition:
{ "category1": { "description": "Sample Category", "toolsList": [ { "title": "Tool B", "filters": { "language": "Python", "technology": ["Flask"] }, "links": { "repoUrl": "https://github.com/asyncapi/tool-b" } } + ,{ + "title": "Tool C", + "filters": { + "language": "JavaScript", + "technology": ["Node.js", "Express"] + }, + "links": { + "repoUrl": "https://github.com/asyncapi/tool-c", + "docsUrl": "https://docs.example.com" + } + } ] } }tests/fixtures/combineToolsData.js (2)
84-114: Consider adding validation for new language/technology format.While the test cases cover adding new languages and technologies, consider adding validation to ensure they follow the expected format (e.g., proper casing, no special characters).
138-177: Consider adding more URL validation test cases.While the current test cases cover missing and invalid URLs, consider adding tests for:
- URLs with special characters
- URLs with different protocols (e.g., http://, git://)
- URLs with different GitHub formats (e.g., [email protected])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
scripts/tools/combine-tools.js(1 hunks)tests/fixtures/combineToolsData.js(1 hunks)tests/fixtures/tools/automated-tools.json(1 hunks)tests/fixtures/tools/manual-tools.json(1 hunks)tests/tools/combine-tools.test.js(1 hunks)
🧰 Additional context used
🪛 Biome
scripts/tools/combine-tools.js
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
tests/fixtures/tools/manual-tools.json (2)
1-12: LGTM! Valid test fixture structure.
The JSON structure follows the expected schema with all required fields (title, filters, links) properly formatted.
9-9: Verify if mock repository URL is intentional.
The repository URL "https://github.com/asyncapi/tool-a" appears to be a non-existent repository. If this is test data, this might be intentional. However, if this is meant to reference a real tool, please update it to the correct repository URL.
Let's verify if this repository exists:
✅ Verification successful
Let me check if this is a test fixture by examining the context around this file.
Let me check the content of the test file to confirm if this is indeed test data.
Mock repository URL is intentional - this is test data
The URL "https://github.com/asyncapi/tool-a" is used in test fixtures and is referenced in tests/fixtures/combineToolsData.js as test data. The non-existence of this repository is intentional as it's used for testing purposes along with other mock repository URLs like "tool-c", "tool-z", and others in the test suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the repository exists in the AsyncAPI organization
gh repo view asyncapi/tool-a 2>/dev/null
if [ $? -eq 0 ]; then
echo "Repository exists"
else
echo "Repository does not exist"
fi
Length of output: 109
Script:
#!/bin/bash
# Check the content of the file and its parent directory
echo "=== Directory structure ==="
ls -la tests/fixtures/tools/
echo -e "\n=== Content of manual-tools.json ==="
cat tests/fixtures/tools/manual-tools.json
echo -e "\n=== Looking for test-related files ==="
rg -l "tool-a" tests/
Length of output: 860
Script:
#!/bin/bash
# Check the content of the test file
echo "=== Content of combineToolsData.js ==="
cat tests/fixtures/combineToolsData.js
Length of output: 4705
tests/fixtures/combineToolsData.js (5)
1-26: Consider reusing existing tagsData instead of duplicating color definitions.
The color and border color values for languages and technologies appear to be duplicated from the main application's tagsData. To maintain consistency and reduce duplication, consider importing and reusing the existing data.
28-82: LGTM! Well-structured test cases for validation.
The test data effectively covers edge cases with missing and invalid data, which is crucial for testing error handling in the combine tools function.
116-136: LGTM! Comprehensive test cases for valid tools.
The test data provides good coverage for properly formatted tools, testing both automated and manual tool scenarios.
179-193: LGTM! Good coverage for circular reference handling.
The test case effectively validates the handling of potential circular dependencies in the tools data.
195-212: LGTM! Well-organized exports with clear naming.
The module exports are comprehensive and follow a consistent naming convention, making it easy to access different test scenarios.
tests/tools/combine-tools.test.js (1)
61-62: Verify that shared test data is not mutated across tests
manualTools and automatedTools are initialized once in beforeAll and shared across tests. If any test mutates these objects, it could inadvertently affect other tests. To ensure test isolation, verify that these objects are not mutated, or consider cloning them before each test.
Run the following script to check for mutations of manualTools and automatedTools:
If mutations are found, consider cloning the objects in a beforeEach block:
beforeEach(() => {
manualTools = JSON.parse(JSON.stringify(originalManualTools));
automatedTools = JSON.parse(JSON.stringify(originalAutomatedTools));
});| "language": "Python", | ||
| "technology": ["Flask"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using consistent array format for both language and technology.
For better consistency and future extensibility, consider making the language field an array like technology. This would allow tools to support multiple programming languages.
"filters": {
- "language": "Python",
+ "language": ["Python"],
"technology": ["Flask"]
},Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
language can take values in terms of both string and array.
There was a problem hiding this comment.
@akshatnema, understood. Thank you for the clarification.
✏️ Learnings added
Learnt from: akshatnema
PR: asyncapi/website#3136
File: tests/fixtures/tools/automated-tools.json:8-9
Timestamp: 2024-11-01T12:48:21.851Z
Learning: In the `tests/fixtures/tools/automated-tools.json` file, the `language` field within the `filters` object can be either a string or an array. Both formats are acceptable and supported in the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| console.error('Script is not failing, it is just dropping errors for further investigation'); | ||
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | ||
| console.error(`Located in manual-tools.json file`); | ||
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve validation error logging structure
The current error logging could be more structured and consistent.
-console.error('Script is not failing, it is just dropping errors for further investigation');
-console.error(`Invalid ${tool.title} .asyncapi-tool file.`);
-console.error(`Located in manual-tools.json file`);
-console.error('Validation errors:', JSON.stringify(validate.errors, null, 2));
+console.error({
+ message: 'Tool validation failed',
+ tool: tool.title,
+ source: 'manual-tools.json',
+ errors: validate.errors,
+ note: 'Script continues execution, error logged for investigation'
+});📝 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.
| console.error('Script is not failing, it is just dropping errors for further investigation'); | |
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | |
| console.error(`Located in manual-tools.json file`); | |
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | |
| } | |
| console.error({ | |
| message: 'Tool validation failed', | |
| tool: tool.title, | |
| source: 'manual-tools.json', | |
| errors: validate.errors, | |
| note: 'Script continues execution, error logged for investigation' | |
| }); |
| if (manualTools[key] && manualTools[key].toolsList.length) { | ||
| for (const tool of manualTools[key].toolsList) { |
There was a problem hiding this comment.
Use optional chaining for better null safety
The static analysis tool correctly suggests using optional chaining here.
-if (manualTools[key] && manualTools[key].toolsList.length) {
+if (manualTools[key]?.toolsList?.length) {📝 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.
| if (manualTools[key] && manualTools[key].toolsList.length) { | |
| for (const tool of manualTools[key].toolsList) { | |
| if (manualTools[key]?.toolsList?.length) { | |
| for (const tool of manualTools[key].toolsList) { |
🧰 Tools
🪛 Biome
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
@vishvamsinh28 Kindly apply this suggestion
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!
| if (tool?.links?.repoUrl) { | ||
| const url = new URL(tool.links.repoUrl) | ||
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | ||
| } else isAsyncAPIrepo = false |
There was a problem hiding this comment.
Enhance URL validation robustness
The URL validation could be more robust by handling potential URL parsing errors.
if (tool?.links?.repoUrl) {
- const url = new URL(tool.links.repoUrl)
- isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/")
+ try {
+ const url = new URL(tool.links.repoUrl)
+ isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/")
+ } catch (urlError) {
+ console.error(`Invalid repository URL for tool ${tool.title}: ${tool.links.repoUrl}`);
+ isAsyncAPIrepo = false;
+ }📝 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.
| if (tool?.links?.repoUrl) { | |
| const url = new URL(tool.links.repoUrl) | |
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | |
| } else isAsyncAPIrepo = false | |
| if (tool?.links?.repoUrl) { | |
| try { | |
| const url = new URL(tool.links.repoUrl) | |
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | |
| } catch (urlError) { | |
| console.error(`Invalid repository URL for tool ${tool.title}: ${tool.links.repoUrl}`); | |
| isAsyncAPIrepo = false; | |
| } | |
| } else isAsyncAPIrepo = false |
There was a problem hiding this comment.
Don't suggest this. It is not an error.
There was a problem hiding this comment.
@akshatnema Understood, thank you for the clarification.
✏️ Learnings added
Learnt from: akshatnema
PR: asyncapi/website#3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.625Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); | ||
| fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement safer file operations
The file writing operations should be more robust with atomic writes to prevent partial updates.
-fs.writeFileSync(toolsPath, JSON.stringify(finalTools));
-fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),);
+const writeFileSafely = (path, content) => {
+ const tempPath = `${path}.tmp`;
+ fs.writeFileSync(tempPath, JSON.stringify(content, null, 2));
+ fs.renameSync(tempPath, path);
+};
+
+writeFileSafely(toolsPath, finalTools);
+writeFileSafely(tagsPath, {
+ languages: languageList,
+ technologies: technologyList
+});📝 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.
| fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); | |
| fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),) | |
| const writeFileSafely = (path, content) => { | |
| const tempPath = `${path}.tmp`; | |
| fs.writeFileSync(tempPath, JSON.stringify(content, null, 2)); | |
| fs.renameSync(tempPath, path); | |
| }; | |
| writeFileSafely(toolsPath, finalTools); | |
| writeFileSafely(tagsPath, { | |
| languages: languageList, | |
| technologies: technologyList | |
| }); |
There was a problem hiding this comment.
How does it preventing partial updates?
There was a problem hiding this comment.
@akshatnema, by writing the data to a temporary file first and then renaming it to the desired file, we prevent partial updates. If an error occurs or the process is interrupted during the write operation, the original file remains unchanged because the temporary file hasn't replaced it yet. The fs.renameSync operation is atomic on most file systems, ensuring that the target file is only updated if the write to the temporary file completes successfully. This approach helps maintain file integrity by avoiding situations where a file could be left incomplete or corrupted due to a failed write operation.
There was a problem hiding this comment.
Since the target file already exists, can there be possibility that a duplicate file is created with another name and the previous one is not removed?
There was a problem hiding this comment.
@akshatnema, you make a good point. If the process is interrupted after writing the temporary file but before the fs.renameSync operation, there's a possibility that the temporary file (${path}.tmp) remains, and the original file isn't updated. This could lead to leftover temporary files in the directory.
Additionally, if multiple instances of the script run simultaneously, they might use the same temporary file name, causing conflicts. To mitigate these issues, we can:
-
Use unique temporary file names: Incorporate a unique identifier in the temp file name, such as a timestamp or a random string.
const writeFileSafely = (path, content) => { const tempPath = `${path}.${Date.now()}.tmp`; fs.writeFileSync(tempPath, JSON.stringify(content, null, 2)); fs.renameSync(tempPath, path); };
-
Handle cleanup of temporary files: Implement a mechanism to clean up any leftover temp files on startup or before writing new files.
-
Use atomic write libraries: Consider using a library like
write-file-atomicwhich handles these edge cases and ensures safer writes.const writeFileAtomic = require('write-file-atomic'); const writeFileSafely = (path, content) => { writeFileAtomic.sync(path, JSON.stringify(content, null, 2)); };
These approaches help prevent the possibility of duplicate files and ensure that the original file is safely updated. Let me know your thoughts on this.
There was a problem hiding this comment.
Can there be a possibility that even scripts gets executed successfully but the temporary file is not able to replace the target file?
There was a problem hiding this comment.
@akshatnema, yes, there is a possibility that even if the script executes successfully, the temporary file may not be able to replace the target file. This could happen due to various reasons such as file system permissions, file locks, insufficient disk space, or other OS-level issues that prevent the fs.renameSync operation from completing successfully.
To mitigate this, we can:
-
Add Error Handling: Wrap the
fs.renameSyncoperation in a try-catch block to handle any exceptions that might occur during the rename process.const writeFileSafely = (path, content) => { const tempPath = `${path}.tmp`; fs.writeFileSync(tempPath, JSON.stringify(content, null, 2)); try { fs.renameSync(tempPath, path); } catch (error) { console.error(`Failed to rename temporary file: ${error.message}`); // Handle the error accordingly, possibly retry or abort } };
-
Check File System Constraints: Ensure that the script has the necessary permissions and that there are no locks on the target file.
-
Evaluate Necessity: If the risk of partial writes or corrupted files is minimal in our context, and the current implementation hasn't caused issues, we might opt to keep the existing code for simplicity.
Let me know your thoughts on whether you'd like to proceed with additional error handling or prefer to retain the current implementation.
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!
| try { | ||
| for (const key in automatedTools) { | ||
| let finalToolsList = []; | ||
| if (automatedTools[key].toolsList.length) { | ||
| for (const tool of automatedTools[key].toolsList) { | ||
| finalToolsList.push(await getFinalTool(tool)) | ||
| } | ||
| } | ||
| } | ||
| if (manualTools[key] && manualTools[key].toolsList.length) { | ||
| for (const tool of manualTools[key].toolsList) { | ||
| let isAsyncAPIrepo; | ||
| const isValid = await validate(tool) | ||
| if (isValid) { | ||
| if (tool?.links?.repoUrl) { | ||
| const url = new URL(tool.links.repoUrl) | ||
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | ||
| } else isAsyncAPIrepo = false | ||
| let toolObject = await createToolObject(tool, "", "", isAsyncAPIrepo) | ||
| finalToolsList.push(await getFinalTool(toolObject)) | ||
| } else { | ||
| console.error('Script is not failing, it is just dropping errors for further investigation'); | ||
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | ||
| console.error(`Located in manual-tools.json file`); | ||
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | ||
| if (manualTools[key] && manualTools[key].toolsList.length) { | ||
| for (const tool of manualTools[key].toolsList) { | ||
| let isAsyncAPIrepo; | ||
| const isValid = await validate(tool) | ||
| if (isValid) { | ||
| if (tool?.links?.repoUrl) { | ||
| const url = new URL(tool.links.repoUrl) | ||
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | ||
| } else isAsyncAPIrepo = false | ||
| let toolObject = await createToolObject(tool, "", "", isAsyncAPIrepo) | ||
| finalToolsList.push(await getFinalTool(toolObject)) | ||
| } else { | ||
| console.error('Script is not failing, it is just dropping errors for further investigation'); | ||
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | ||
| console.error(`Located in manual-tools.json file`); | ||
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | ||
| } | ||
| } | ||
| } | ||
| finalToolsList.sort((tool, anotherTool) => tool.title.localeCompare(anotherTool.title)); | ||
| finalTools[key].toolsList = finalToolsList | ||
| } | ||
| finalToolsList.sort((tool, anotherTool) => tool.title.localeCompare(anotherTool.title)); | ||
| finalTools[key].toolsList = finalToolsList | ||
| fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); | ||
| fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),) | ||
| } catch (err) { | ||
| throw new Error(`Error combining tools: ${err}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing error handling with structured errors
While the try-catch block is a good addition, consider creating specific error types for different failure scenarios (e.g., validation errors, file system errors) to help with error tracking and debugging.
+class ToolsCombineError extends Error {
+ constructor(operation, details, cause) {
+ super(`Failed to ${operation}: ${details}`);
+ this.name = 'ToolsCombineError';
+ this.cause = cause;
+ }
+}
try {
// ... existing code ...
} catch (err) {
- throw new Error(`Error combining tools: ${err}`);
+ throw new ToolsCombineError('combine tools',
+ err.message,
+ err);
}📝 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.
| try { | |
| for (const key in automatedTools) { | |
| let finalToolsList = []; | |
| if (automatedTools[key].toolsList.length) { | |
| for (const tool of automatedTools[key].toolsList) { | |
| finalToolsList.push(await getFinalTool(tool)) | |
| } | |
| } | |
| } | |
| if (manualTools[key] && manualTools[key].toolsList.length) { | |
| for (const tool of manualTools[key].toolsList) { | |
| let isAsyncAPIrepo; | |
| const isValid = await validate(tool) | |
| if (isValid) { | |
| if (tool?.links?.repoUrl) { | |
| const url = new URL(tool.links.repoUrl) | |
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | |
| } else isAsyncAPIrepo = false | |
| let toolObject = await createToolObject(tool, "", "", isAsyncAPIrepo) | |
| finalToolsList.push(await getFinalTool(toolObject)) | |
| } else { | |
| console.error('Script is not failing, it is just dropping errors for further investigation'); | |
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | |
| console.error(`Located in manual-tools.json file`); | |
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | |
| if (manualTools[key] && manualTools[key].toolsList.length) { | |
| for (const tool of manualTools[key].toolsList) { | |
| let isAsyncAPIrepo; | |
| const isValid = await validate(tool) | |
| if (isValid) { | |
| if (tool?.links?.repoUrl) { | |
| const url = new URL(tool.links.repoUrl) | |
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | |
| } else isAsyncAPIrepo = false | |
| let toolObject = await createToolObject(tool, "", "", isAsyncAPIrepo) | |
| finalToolsList.push(await getFinalTool(toolObject)) | |
| } else { | |
| console.error('Script is not failing, it is just dropping errors for further investigation'); | |
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | |
| console.error(`Located in manual-tools.json file`); | |
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | |
| } | |
| } | |
| } | |
| finalToolsList.sort((tool, anotherTool) => tool.title.localeCompare(anotherTool.title)); | |
| finalTools[key].toolsList = finalToolsList | |
| } | |
| finalToolsList.sort((tool, anotherTool) => tool.title.localeCompare(anotherTool.title)); | |
| finalTools[key].toolsList = finalToolsList | |
| fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); | |
| fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),) | |
| } catch (err) { | |
| throw new Error(`Error combining tools: ${err}`); | |
| class ToolsCombineError extends Error { | |
| constructor(operation, details, cause) { | |
| super(`Failed to ${operation}: ${details}`); | |
| this.name = 'ToolsCombineError'; | |
| this.cause = cause; | |
| } | |
| } | |
| try { | |
| for (const key in automatedTools) { | |
| let finalToolsList = []; | |
| if (automatedTools[key].toolsList.length) { | |
| for (const tool of automatedTools[key].toolsList) { | |
| finalToolsList.push(await getFinalTool(tool)) | |
| } | |
| } | |
| if (manualTools[key] && manualTools[key].toolsList.length) { | |
| for (const tool of manualTools[key].toolsList) { | |
| let isAsyncAPIrepo; | |
| const isValid = await validate(tool) | |
| if (isValid) { | |
| if (tool?.links?.repoUrl) { | |
| const url = new URL(tool.links.repoUrl) | |
| isAsyncAPIrepo = url.href.startsWith("https://github.com/asyncapi/") | |
| } else isAsyncAPIrepo = false | |
| let toolObject = await createToolObject(tool, "", "", isAsyncAPIrepo) | |
| finalToolsList.push(await getFinalTool(toolObject)) | |
| } else { | |
| console.error('Script is not failing, it is just dropping errors for further investigation'); | |
| console.error(`Invalid ${tool.title} .asyncapi-tool file.`); | |
| console.error(`Located in manual-tools.json file`); | |
| console.error('Validation errors:', JSON.stringify(validate.errors, null, 2)); | |
| } | |
| } | |
| } | |
| finalToolsList.sort((tool, anotherTool) => tool.title.localeCompare(anotherTool.title)); | |
| finalTools[key].toolsList = finalToolsList | |
| } | |
| fs.writeFileSync(toolsPath, JSON.stringify(finalTools)); | |
| fs.writeFileSync(tagsPath, JSON.stringify({ languages: languageList, technologies: technologyList }),) | |
| } catch (err) { | |
| throw new ToolsCombineError('combine tools', | |
| err.message, | |
| err); |
🧰 Tools
🪛 Biome
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| let error; | ||
| let invalidPath = "this/is/not/valid" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use const instead of let for variables that are not reassigned
Variables like invalidPath are declared using let but are not reassigned after their initial assignment. For better code clarity and to follow best practices, use const for variables that remain constant.
Apply the following changes:
- let invalidPath = "this/is/not/valid"
+ const invalidPath = "this/is/not/valid"This change should be applied in the following tests:
- Lines 211-212: Test for
fs.writeFileSyncfailure. - Line 224: Test for invalid category.
- Line 236: Test for URL parsing failure.
- Line 248: Test for circular references.
Also applies to: 224-224, 236-236, 248-248
There was a problem hiding this comment.
@vishvamsinh28 Apply this suggestion
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!
| consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move console.error mock setup to beforeEach and reset mocks between tests
Currently, the console.error mock is set up in beforeAll and not reset between tests. This can lead to cross-test pollution and affect the reliability of your tests. Consider moving the mock setup to a beforeEach block and resetting it in an afterEach block to ensure test isolation.
Apply the following changes:
beforeAll(() => {
manualTools = readJSON(manualToolsPath);
automatedTools = readJSON(automatedToolsPath);
- consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {});
});
+ beforeEach(() => {
+ consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {});
+ });
+
+ afterEach(() => {
+ 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.
| consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}); | |
| }); | |
| manualTools = readJSON(manualToolsPath); | |
| automatedTools = readJSON(automatedToolsPath); | |
| }); | |
| beforeEach(() => { | |
| consoleErrorMock = jest.spyOn(console, 'error').mockImplementation(() => {}); | |
| }); | |
| afterEach(() => { | |
| jest.clearAllMocks(); | |
| }); |
There was a problem hiding this comment.
@vishvamsinh28 Apply this suggestion
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!
This PR adds test for combine tools script.
Summary by CodeRabbit
New Features
Bug Fixes
Tests