fix: file write errors for tools and newsroom video #3297
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3297 +/- ##
=======================================
Coverage 47.32% 47.32%
=======================================
Files 22 22
Lines 672 672
=======================================
Hits 318 318
Misses 354 354 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3297--asyncapi-website.netlify.app/ |
scripts/build-tools.js
Outdated
| const automatedToolsPath = resolve(__dirname, '../config', 'tools-automated.json'); | ||
| const manualToolsPath = resolve(__dirname, '../config', 'tools-manual.json'); | ||
| const toolsPath = resolve(__dirname, '../config', 'tools.json'); | ||
| const tagsPath = resolve(__dirname, '../config', 'all-tags.json'); |
There was a problem hiding this comment.
Why has resolve worked previously? I saw tests or scripts aren't failing even with resolve
There was a problem hiding this comment.
I thought the same we dont need this change, the workflow was initially failing on Windows, so I researched it and found that it might be due to path errors. I came across a solution suggesting the use of path.join, which should work across all OS and it passed all the checks. However, it's now failing on Ubuntu as well, and I think the issue might be with the fs module, which is causing the workflow to fail.
There was a problem hiding this comment.
Should i close this PR ?
There was a problem hiding this comment.
Don't close it but try to identify the root cause of that issue and push the changes
WalkthroughThe changes involve updating file handling in several scripts by switching from the native Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
tests/build-tools.test.js (2)
81-81: Good use ofpath.join()for the invalid path.The change to
path.join()is consistent with earlier modifications and appropriate for constructing an invalid path.Consider adding a comment explaining the purpose of this invalid path for improved code readability:
- const invalidPath = path.join('/invalid_dir', 'tools.json'); + // Create an invalid path to test file write errors + const invalidPath = path.join('/invalid_dir', 'tools.json');
Line range hint
1-89: Consider adding tests for path handling.The changes to use
path.join()consistently improve the portability of the tests. However, to ensure robustness, consider adding specific tests for path handling:
- Test with paths containing different separators (forward slash, backslash) to verify cross-platform compatibility.
- Test with relative and absolute paths to ensure correct behavior in various scenarios.
- Test with empty path segments to verify proper handling of edge cases.
These additional tests would help validate the new path joining logic across different environments.
📜 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 (6)
scripts/build-tools.js (4)
Line range hint
8-22: LGTM: Function logic remains sound and unchanged.The
buildToolsfunction maintains its correct implementation, including proper error handling and asynchronous operations. The function signature addition in the summary likely reflects changes in the module's export structure rather than modifications to the function itself.
25-28: Improved cross-platform compatibility withpath.join()The change from
path.resolve()topath.join()is a good step towards ensuring better cross-platform compatibility, especially for Windows systems. This modification aligns with the intention mentioned in the previous comments to address path-related issues.
25-28: Further investigation needed for build failuresWhile the change to
path.join()is a step in the right direction for cross-platform compatibility, the persistence of issues on Ubuntu (as mentioned in the past comments) suggests that there might be underlying problems beyond path resolution.Consider the following steps:
- Verify if the build process is still failing on both Windows and Ubuntu with these changes.
- If failures persist, investigate other potential causes, such as issues with the
fsmodule or other aspects of the build process.- Consider adding more detailed error logging to pinpoint the exact cause of failures.
To help diagnose the issue, you can add more detailed logging to the
buildToolsfunction. Here's a script to modify the file:#!/bin/bash # Description: Add detailed logging to buildTools function # Test: Add logging statements to buildTools function sed -i '9i\ console.log(`Starting buildTools with paths:`);\n console.log(`automatedToolsPath: ${automatedToolsPath}`);\n console.log(`manualToolsPath: ${manualToolsPath}`);\n console.log(`toolsPath: ${toolsPath}`);\n console.log(`tagsPath: ${tagsPath}`);' scripts/build-tools.js sed -i '14i\ console.log(`GitHub extract data retrieved. Length: ${githubExtractData.length}`);' scripts/build-tools.js sed -i '16i\ console.log(`Automated tools converted. Count: ${Object.keys(automatedTools).length}`);' scripts/build-tools.js sed -i '21i\ console.log(`Combined tools written to file: ${toolsPath}`);' scripts/build-tools.js sed -i '23i\ console.log(`Build tools process completed successfully.`);' scripts/build-tools.js # Display the changes diff scripts/build-tools.js <(git show HEAD:scripts/build-tools.js)This script adds logging statements to help identify where the process might be failing. After applying these changes, run the build process again and analyze the logs to identify the root cause of any persistent issues.
25-28: Verify path resolution behaviorWhile
path.join()improves cross-platform compatibility, it's important to note that it behaves differently frompath.resolve(). Specifically:
join()concatenates path segments, whileresolve()resolves to an absolute path.resolve()processes segments from right to left, whilejoin()goes left to right.Please verify that these changes don't inadvertently affect the expected path resolution in your application.
To help verify the path resolution, you can run the following script:
This script will help you compare the outputs of
path.join()andpath.resolve()for the modified paths, allowing you to ensure that the change doesn't introduce any unexpected behavior.tests/build-tools.test.js (2)
2-2: LGTM: Correct import of thepathmodule.The
pathmodule is correctly imported, which is essential for thepath.join()function used throughout the file.
27-31: Excellent use ofpath.join()for constructing file paths.The change from
path.resolve()topath.join()is a good improvement:
- It's more appropriate for joining path segments without resolving to absolute paths.
- It maintains consistency across all path definitions.
- Using
__dirnameensures paths are relative to the current directory, which is a best practice.This change enhances the portability and reliability of the test suite across different environments.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
scripts/build-tools.js (1)
11-18: LGTM: Improved file writing process with directory check and retry mechanism.The changes address the path-related issues mentioned in previous comments and add resilience to the file writing process. The directory check before writing is a good practice.
Consider adding a comment explaining the purpose of the
retryWriteFilefunction for better code documentation.const automatedDir = dirname(automatedToolsPath); if (!fs.existsSync(automatedDir)) { fs.mkdirSync(automatedDir, { recursive: true }); } + // Use retryWriteFile to handle potential ENOENT errors and improve reliability await retryWriteFile(automatedToolsPath, JSON.stringify(automatedTools, null, ' '));tests/build-newsroom-videos.test.js (2)
18-20: Good improvement in cleanup logicThe addition of the existence check before removing the test directory is a good practice. It prevents potential errors when trying to remove a non-existent directory.
Consider using
fs.promises.rmfor asynchronous directory removal, which is generally preferred in Node.js for better performance:afterAll(async () => { if (existsSync(testDir)) { await fs.promises.rm(testDir, { recursive: true, force: true }); } });
24-26: Good addition for test setup reliabilityThe conditional creation of the test directory in the
beforeEachhook is a good practice. It ensures that the directory exists before each test, preventing potential errors.Consider using
fs.promises.mkdirfor asynchronous directory creation:beforeEach(async () => { await fs.promises.mkdir(testDir, { recursive: true }); fetch.mockClear(); });This approach is more idiomatic in modern Node.js and can help with performance in larger test suites.
scripts/build-newsroom-videos.js (1)
60-60: Renameresolvein the Promise to avoid shadowingThe parameter
resolvein yourPromiseexecutor may shadow theresolvefunction imported from thepathmodule. This can cause confusion or unexpected behavior.Rename the parameter to avoid shadowing:
- await new Promise((resolve) => setTimeout(resolve, delay)); + await new Promise((res) => setTimeout(res, delay));🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-60: scripts/build-newsroom-videos.js#L58-L60
Added lines #L58 - L60 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- scripts/build-newsroom-videos.js (2 hunks)
- scripts/build-tools.js (1 hunks)
- tests/build-newsroom-videos.test.js (4 hunks)
- tests/build-tools.test.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-tools.test.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
scripts/build-newsroom-videos.js
[warning] 58-60: scripts/build-newsroom-videos.js#L58-L60
Added lines #L58 - L60 were not covered by tests
[warning] 62-62: scripts/build-newsroom-videos.js#L62
Added line #L62 was not covered by testsscripts/build-tools.js
[warning] 33-35: scripts/build-tools.js#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 37-37: scripts/build-tools.js#L37
Added line #L37 was not covered by tests
🔇 Additional comments (6)
scripts/build-tools.js (3)
5-5: LGTM: Import ofdirnameis appropriate.The addition of
dirnamefrom thepathmodule is consistent with its usage in the modifiedbuildToolsfunction. This change is necessary and correct.
Line range hint
1-41: Overall: Good improvements to file writing reliability and cross-platform compatibility.The changes in this file effectively address the issues mentioned in previous comments regarding path handling across different operating systems. The introduction of the
retryWriteFilefunction and the directory existence check improve the reliability of file writing operations.Key points:
- The use of
path.dirnameand directory checks addresses potential ENOENT errors.- The new
retryWriteFilefunction adds resilience to file writing operations.- Consider adding more documentation and improving test coverage as suggested in previous comments.
These changes contribute positively to the overall robustness of the build tools script.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-35: scripts/build-tools.js#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 37-37: scripts/build-tools.js#L37
Added line #L37 was not covered by tests
26-41: LGTM: Well-implemented retry mechanism for file writing.The
retryWriteFilefunction is a good addition to handle transient file system issues. It correctly implements a retry mechanism with appropriate error handling for ENOENT errors.Suggestions for improvement:
- Consider logging or throwing an error if all retry attempts fail.
- The static analysis tool indicates that some lines are not covered by tests. Consider adding test cases to cover these scenarios.
To address the code coverage issue, let's verify the existing test coverage:
Would you like assistance in generating additional test cases to improve coverage for the
retryWriteFilefunction?🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 33-35: scripts/build-tools.js#L33-L35
Added lines #L33 - L35 were not covered by tests
[warning] 37-37: scripts/build-tools.js#L37
Added line #L37 was not covered by teststests/build-newsroom-videos.test.js (3)
1-1: LGTM: Improved import for better directory handlingThe addition of
existsSyncfrom thefsmodule is a good improvement. It allows for more robust directory existence checks throughout the test file, enhancing error handling and test reliability.
52-52: LGTM: Improved readabilityThe addition of this empty line improves the readability of the test by clearly separating the API call expectations from the file content assertions.
Line range hint
1-95: Overall assessment: Good improvements with minor suggestionsThe changes in this file generally improve error handling and test reliability, particularly in managing the test directory. The addition of
existsSyncchecks and the use ofmkdirSyncwith therecursiveoption are good practices.Key points:
- The import changes and directory existence checks in
afterAllandbeforeEachhooks are well-implemented.- There's a redundant directory check in the first test case that should be removed.
- Consider using
fs.promisesmethods for asynchronous file system operations, which is more idiomatic in modern Node.js.These changes enhance the robustness of the tests, but there's room for minor improvements as noted in the individual comments.
tests/build-newsroom-videos.test.js
Outdated
| if (!existsSync(testDir)) { | ||
| mkdirSync(testDir, { recursive: true }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Remove redundant directory check
The directory existence check and creation here is redundant with the one in the beforeEach hook. Since beforeEach runs before every test, including this one, the directory will always exist at this point.
Remove these lines to avoid unnecessary checks:
- if (!existsSync(testDir)) {
- mkdirSync(testDir, { recursive: true });
- }
-This will simplify the test and improve its readability without affecting its functionality.
📝 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 (!existsSync(testDir)) { | |
| mkdirSync(testDir, { recursive: true }); | |
| } |
scripts/build-newsroom-videos.js
Outdated
| if (err.code === 'ENOENT') { | ||
| console.error(`ENOENT error on attempt ${attempt + 1}. Retrying in ${delay}ms...`); | ||
| await new Promise((resolve) => setTimeout(resolve, delay)); | ||
| } else { | ||
| throw err; |
There was a problem hiding this comment.
Add tests to cover error handling in retryWriteFile
The error handling paths, specifically when catching ENOENT errors and other exceptions, are not currently covered by tests. Ensuring these paths are tested will improve the reliability of your code.
Consider adding unit tests that simulate ENOENT and other errors to verify the retry logic works as expected. Would you like assistance in writing these tests or should I open a new GitHub issue to track this task?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 58-60: scripts/build-newsroom-videos.js#L58-L60
Added lines #L58 - L60 were not covered by tests
[warning] 62-62: scripts/build-newsroom-videos.js#L62
Added line #L62 was not covered by tests
scripts/build-newsroom-videos.js
Outdated
| async function retryWriteFile(filePath, data, retries = 3, delay = 1000) { | ||
| for (let attempt = 0; attempt < retries; attempt++) { | ||
| try { | ||
| writeFileSync(filePath, data); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use asynchronous writeFile instead of writeFileSync
Using writeFileSync in an asynchronous function can block the event loop, negating the benefits of asynchronous code. Consider using fs.promises.writeFile to perform the file write operation asynchronously.
Apply this change to use the asynchronous writeFile:
- writeFileSync(filePath, data);
+ await fs.promises.writeFile(filePath, data);Update your imports to include fs.promises:
-const { writeFileSync, mkdirSync, existsSync } = require('fs');
+const { mkdirSync, existsSync, promises: fsPromises } = require('fs');And modify the code accordingly:
+ await fsPromises.writeFile(filePath, data);Committable suggestion was skipped due to low confidence.
scripts/build-newsroom-videos.js
Outdated
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Throw an error if all retries fail in retryWriteFile
If all retry attempts fail, the function currently exits silently without throwing an error. This can lead to unhandled exceptions upstream. It's important to notify the calling function of the failure.
Add an error throw after the retry loop:
}
+ throw new Error(`Failed to write file to ${filePath} after ${retries} attempts.`);
}Committable suggestion was skipped due to low confidence.
vishvamsinh28
changed the title
|
@vishvamsinh28 Kindly explain what exactly you changed with this PR. |
|
@anshgoyalevil I replaced the fs module with fs-extra to fix the issue. |
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
- package.json (1 hunks)
- scripts/build-newsroom-videos.js (2 hunks)
- scripts/build-tools.js (1 hunks)
- tests/build-newsroom-videos.test.js (2 hunks)
- tests/build-tools.test.js (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/build-newsroom-videos.js
- scripts/build-tools.js
- tests/build-newsroom-videos.test.js
- tests/build-tools.test.js
| @@ -70,6 +70,7 @@ | |||
| "clsx": "^2.1.0", | |||
| "cssnano": "^6.0.3", | |||
| "dotenv": "^16.4.4", | |||
| "fs-extra": "^11.2.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Native fs module is still widely used - migration to fs-extra is incomplete
The codebase shows inconsistent usage of file system modules:
fs-extrais only used in 4 files:build-tools.js,build-tools.test.js,build-newsroom-videos.js, andbuild-newsroom-videos.test.js- 19 other files still use the native
fsmodule
Since the PR's objective was to switch from native fs to fs-extra for better error handling and async operations, the migration appears to be incomplete. The changes should be consistent across the codebase to:
- Leverage
fs-extra's enhanced features uniformly - Maintain consistent error handling patterns
- Avoid mixing different file operation paradigms
🔗 Analysis chain
LGTM! Consider adding @types/fs-extra for TypeScript support.
The addition of fs-extra is appropriate for enhanced file system operations and better error handling. Since this project uses TypeScript, consider adding @types/fs-extra to devDependencies for improved type safety.
Let's verify the fs-extra usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify fs-extra is properly imported and used instead of native fs
# Test 1: Check if native 'fs' is still being imported
echo "Checking for native fs imports..."
rg "require\(['\"]fs['\"]\)" || rg "from ['\"]fs['\"]\""
# Test 2: Verify fs-extra usage
echo "Checking fs-extra usage..."
rg "require\(['\"]fs-extra['\"]\)" || rg "from ['\"]fs-extra['\"]\""
Length of output: 1781
|
|
||
| jest.mock('node-fetch-2', () => jest.fn()); | ||
|
|
||
| describe('buildNewsroomVideos', () => { | ||
| const testDir = resolve(__dirname, 'test_config'); | ||
| const testDir = join(os.tmpdir(), 'test_config'); |
There was a problem hiding this comment.
This is a nice addition. We should use os.tmpDir() for other scripts as well where a temp folder was being created inside the website directory.
|
/rtm |
anshgoyalevil
added
the
gsoc
This PR replaces resolve with path.join for paths in build-tools script and its test
Summary by CodeRabbit
fs-extrafor improved file system operations.