feat: increase code coverage #3517
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. |
WalkthroughThis pull request introduces several enhancements to test coverage and error handling across multiple files. The Jest configuration is updated to expand the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3517 +/- ##
===========================================
+ Coverage 98.66% 100.00% +1.33%
===========================================
Files 21 19 -2
Lines 672 668 -4
===========================================
+ Hits 663 668 +5
+ Misses 9 0 -9 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3517--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/index.test.js (1)
7-7: Address import order and missing semicolon.Static analysis suggests moving the
fsimport above other local imports and adding a trailing semicolon for consistency. Here's a quick fix:-const fs = require('fs') +const fs = require('fs');🧰 Tools
🪛 eslint
[error] 7-7:
fsimport should occur before import of../scripts/build-rss(import/order)
[error] 7-7: Insert
;(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jest.config.js(1 hunks)scripts/build-pages.js(1 hunks)tests/dashboard/build-dashboard.test.js(1 hunks)tests/index.test.js(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/build-pages.js
🧰 Additional context used
🪛 eslint
tests/index.test.js
[error] 7-7: fs import should occur before import of ../scripts/build-rss
(import/order)
[error] 7-7: Insert ;
(prettier/prettier)
tests/dashboard/build-dashboard.test.js
[error] 199-200: Delete ⏎····
(prettier/prettier)
[error] 201-202: Delete ··⏎
(prettier/prettier)
🔇 Additional comments (2)
jest.config.js (1)
7-7: Consider reconsidering these ignore patterns if the goal is to reach 100% coverage.
You're ignoring two additional script files (categorylist.js, tags-color.js) in coveragePathIgnorePatterns. If your PR aims to achieve maximum coverage, it may be counterproductive to exempt these files.
tests/index.test.js (1)
36-45: Good improvement - ensures error handling is tested.
The test effectively mocks readdirSync to return an empty array, verifying that the start function throws the expected error. This significantly fortifies negative path coverage.
| it('should handle write failures gracefully', async () => { | ||
|
|
||
| await expect(writeToFile).rejects.toThrow(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the test invocation covers the function call.
Your test awaits writeToFile but doesn't explicitly invoke it with parameters. Clarify whether this is intentional. If your intention is to test calling writeToFile() without arguments, adjust the test as follows to ensure it actually attempts a call:
- await expect(writeToFile).rejects.toThrow();
+ await expect(writeToFile()).rejects.toThrow();Also, remove extra blank lines for compliance with the style guide.
📝 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.
| it('should handle write failures gracefully', async () => { | |
| await expect(writeToFile).rejects.toThrow(); | |
| }); | |
| it('should handle write failures gracefully', async () => { | |
| await expect(writeToFile()).rejects.toThrow(); | |
| }); |
🧰 Tools
🪛 eslint
[error] 199-200: Delete ⏎····
(prettier/prettier)
[error] 201-202: Delete ··⏎
(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/index.test.js (2)
7-7: Fix import order.Move the
fsimport before the local imports to follow the standard import ordering convention:
- Built-in modules
- External dependencies
- Local imports
+const fs = require('fs'); const rssFeed = require('../scripts/build-rss'); const { buildPostList } = require('../scripts/build-post-list'); const buildCaseStudiesList = require('../scripts/casestudies'); const buildAdoptersList = require('../scripts/adopters'); const buildFinanceInfoList = require('../scripts/finance'); const start = require('../scripts/index'); -const fs = require('fs');🧰 Tools
🪛 eslint
[error] 7-7:
fsimport should occur before import of../scripts/build-rss(import/order)
37-45: LGTM! Consider adding more error scenarios.The test case effectively validates the error handling when no finance data is found. To achieve 100% coverage, consider adding tests for:
- Error scenarios in other build functions (buildPostList, buildCaseStudiesList, etc.)
- Error handling in the RSS feed generation
Would you like me to help generate additional test cases for these scenarios?
tests/dashboard/build-dashboard.test.js (1)
198-202: LGTM! Clean up formatting.The test case effectively validates write failures. Remove the extra blank lines to maintain consistent formatting:
it('should handle write failures gracefully', async () => { - await expect(writeToFile()).rejects.toThrow(); - }); - + });🧰 Tools
🪛 eslint
[error] 199-200: Delete
⏎····(prettier/prettier)
[error] 201-202: Delete
··⏎(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/dashboard/build-dashboard.test.js(1 hunks)tests/index.test.js(2 hunks)
🧰 Additional context used
🪛 eslint
tests/dashboard/build-dashboard.test.js
[error] 199-200: Delete ⏎····
(prettier/prettier)
[error] 201-202: Delete ··⏎
(prettier/prettier)
tests/index.test.js
[error] 7-7: fs import should occur before import of ../scripts/build-rss
(import/order)
scripts/build-pages.js
Outdated
| if (!fs.existsSync(TARGET_DIR)) { | ||
| fs.mkdirSync(TARGET_DIR, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
Can we add a test that enters this if clause?
There was a problem hiding this comment.
Yes, added a test for it
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build-pages.js (1)
64-64: Resolve formatting/linting issue.
Ensure consistency with the project's style guide by adding proper spacing and a trailing semicolon.-module.exports = { copyAndRenameFiles,capitalizeJsxTags, ensureDirectoryExists } +module.exports = { copyAndRenameFiles, capitalizeJsxTags, ensureDirectoryExists };🧰 Tools
🪛 eslint
[error] 64-64: Replace
capitalizeJsxTags,·ensureDirectoryExists·}with·capitalizeJsxTags,·ensureDirectoryExists·};⏎(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/build-pages.js(2 hunks)tests/build-pages.test.js(2 hunks)
🧰 Additional context used
🪛 eslint
tests/build-pages.test.js
[error] 50-50: 'TEST_DIR' is already declared in the upper scope on line 20 column 9.
(no-shadow)
scripts/build-pages.js
[error] 64-64: Replace capitalizeJsxTags,·ensureDirectoryExists·} with ·capitalizeJsxTags,·ensureDirectoryExists·};⏎
(prettier/prettier)
🔇 Additional comments (3)
tests/build-pages.test.js (1)
3-3: Successfully imported the new utility function.
It's great to see ensureDirectoryExists added to the test. This improves maintainability by clearly separating responsibilities for directory creation from file operations.
scripts/build-pages.js (2)
10-13: Robust directory creation logic.
Using fs.mkdirSync(directory, { recursive: true }) ensures that nested directories will be created as needed. Consider wrapping file operations in a try/catch block if you need to handle errors more gracefully.
16-16: Automatic directory initialization is a good practice.
Calling ensureDirectoryExists(TARGET_DIR) before copying and renaming files is a clean way to avoid file operation errors.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
/rtm |
This PR increases the code coverage for the scripts, bringing it to 100%.
Summary by CodeRabbit
New Features
Bug Fixes
Tests