feat: add tests for check markdown script #3378
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe changes in this pull request involve updates to the 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
Documentation and Community
|
✅ 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 #3378 +/- ##
==========================================
+ Coverage 67.77% 77.27% +9.50%
==========================================
Files 21 21
Lines 664 669 +5
==========================================
+ Hits 450 517 +67
+ Misses 214 152 -62 ☔ View full report in Codecov by Sentry. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3378--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: 2
🧹 Outside diff range and nitpick comments (6)
tests/markdown/check-markdown.test.js (3)
14-21: Consider improving error handling in beforeEach.While the setup works, the error handling could be more robust by using async/await pattern instead of the callback approach.
Consider refactoring to:
- beforeEach(done => { - mockConsoleError = jest.spyOn(console, 'error').mockImplementation(); - fs.mkdtemp(path.join(os.tmpdir(), 'test-config'), (err, directory) => { - if (err) throw err; - tempDir = directory; - done(); - }); - }); + beforeEach(async () => { + mockConsoleError = jest.spyOn(console, 'error').mockImplementation(); + tempDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'test-config')); + });
28-44: Add edge cases to author validation tests.The current test case doesn't cover all possible scenarios for author validation.
Consider adding tests for:
- Empty authors array
- Authors with empty strings
- Authors with malformed URLs (e.g., missing protocol)
- Maximum length validation for author fields
46-53: Enhance docs frontmatter validation test coverage.The current test only checks for invalid types but misses other important scenarios.
Consider adding test cases for:
- Empty frontmatter object
- Missing optional fields
- Edge cases for weight values (negative, zero, decimal)
- Special characters in title
scripts/markdown/check-markdown.js (3)
148-148: Fix the trailing comma in module.exports.The trailing comma in the exports statement is unnecessary and inconsistent with the rest of the codebase.
-module.exports = { validateBlogs, validateDocs, checkMarkdownFiles,} +module.exports = { validateBlogs, validateDocs, checkMarkdownFiles }
Line range hint
9-17: Consider using a more robust URL validation.The current URL validation might accept invalid URLs in some edge cases. Consider using a more comprehensive validation approach.
function isValidURL(str) { try { - new URL(str); - return true; + const url = new URL(str); + // Additional validation for http/https protocols + return ['http:', 'https:'].includes(url.protocol); } catch (err) { return false; } }
Line range hint
82-144: Consider refactoring to use promises for better testability.The
checkMarkdownFilesfunction uses nested callbacks, making it harder to test. Consider refactoring to use promises or async/await for better error handling and testability.Here's a suggested refactor:
async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') { try { const files = await fs.promises.readdir(folderPath); for (const file of files) { const filePath = path.join(folderPath, file); const relativeFilePath = path.join(relativePath, file); if (relativeFilePath.includes('reference/specification')) { continue; } const stats = await fs.promises.stat(filePath); if (stats.isDirectory()) { await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); } else if (path.extname(file) === '.md') { const fileContent = await fs.promises.readFile(filePath, 'utf-8'); const { data: frontmatter } = matter(fileContent); const errors = validateFunction(frontmatter); if (errors) { console.log(`Errors in file ${relativeFilePath}:`); errors.forEach(error => console.log(` - ${error}`)); process.exitCode = 1; } } } } catch (err) { console.error('Error processing files:', err); process.exitCode = 1; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(1 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🔇 Additional comments (2)
tests/markdown/check-markdown.test.js (1)
1-8: LGTM! Imports are well-organized.
The necessary Node.js built-in modules and validation functions are properly imported.
scripts/markdown/check-markdown.js (1)
147-148: Verify test coverage for exported functions.
The functions are now being exported for testing purposes. Let's verify that appropriate tests have been implemented.
✅ Verification successful
Let me gather more information about the test coverage by examining the actual test implementations.
Let me check the complete test file content to get a full picture of the test coverage.
Test coverage is comprehensive for all exported functions ✅
The test suite includes thorough coverage for all exported functions:
validateBlogs: Tests for author validation, multiple validation errors, and invalid frontmattervalidateDocs: Tests for required fields validationcheckMarkdownFiles: Tests for file processing, error handling for invalid files, and filesystem errors
The tests include proper assertions, error cases, and mock implementations for filesystem operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for the exported functions
# Test 1: Check if test file exists and contains tests for all exported functions
echo "Checking test coverage for exported functions..."
rg -A 5 "describe\(.*(?:validateBlogs|validateDocs|checkMarkdownFiles)" "tests/markdown/check-markdown.test.js"
# Test 2: Verify that the functions are only used in the main script and tests
echo "Verifying function usage across codebase..."
rg "(?:validateBlogs|validateDocs|checkMarkdownFiles)" --type js
Length of output: 1748
Script:
#!/bin/bash
# Check the actual test implementations
echo "Examining test implementations..."
rg -B 2 -A 10 "describe\('.*'" "tests/markdown/check-markdown.test.js"
# Check if there are any test assertions
echo "Checking test assertions..."
rg "expect|assert|should" "tests/markdown/check-markdown.test.js"
Length of output: 1192
Script:
#!/bin/bash
# Get the complete test file content
echo "Examining complete test file..."
cat "tests/markdown/check-markdown.test.js"
Length of output: 3860
| it('checks for errors in markdown files in a directory', done => { | ||
| fs.writeFileSync(path.join(tempDir, 'invalid.md'), `---\ntitle: Invalid Blog\n---`); | ||
| const mockConsoleLog = jest.spyOn(console, 'log').mockImplementation(); | ||
|
|
||
| checkMarkdownFiles(tempDir, validateBlogs); | ||
|
|
||
| setTimeout(() => { | ||
| expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:')); | ||
| mockConsoleLog.mockRestore(); | ||
| done(); | ||
| }, 100); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor async tests to use more reliable patterns.
The current implementation has several issues:
- Hardcoded timeout values (100ms) might cause flaky tests
- setTimeout with done callback is an anti-pattern in Jest
- Inconsistent async testing patterns across different test cases
Consider refactoring to use async/await pattern consistently:
- it('checks for errors in markdown files in a directory', done => {
- fs.writeFileSync(path.join(tempDir, 'invalid.md'), `---\ntitle: Invalid Blog\n---`);
- const mockConsoleLog = jest.spyOn(console, 'log').mockImplementation();
-
- checkMarkdownFiles(tempDir, validateBlogs);
-
- setTimeout(() => {
- expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:'));
- mockConsoleLog.mockRestore();
- done();
- }, 100);
+ it('checks for errors in markdown files in a directory', async () => {
+ fs.writeFileSync(path.join(tempDir, 'invalid.md'), `---\ntitle: Invalid Blog\n---`);
+ const mockConsoleLog = jest.spyOn(console, 'log').mockImplementation();
+
+ await checkMarkdownFiles(tempDir, validateBlogs);
+
+ expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:'));
+ mockConsoleLog.mockRestore();Also applies to: 83-94, 96-108
| authors: { name: 'John Doe' } | ||
| }; | ||
| const errors = validateBlogs(frontmatter); | ||
| console.log(errors) |
There was a problem hiding this comment.
Remove debug console.log statement.
Debug logging should not be committed to the test suite.
- console.log(errors)📝 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.log(errors) |
| setTimeout(() => { | ||
| expect(mockConsoleLog).toHaveBeenCalledWith(expect.stringContaining('Errors in file invalid.md:')); | ||
| mockConsoleLog.mockRestore(); | ||
| done(); | ||
| }, 100); | ||
| }); |
There was a problem hiding this comment.
Why are we adding timeout here?
There was a problem hiding this comment.
replaced it with async/await
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/markdown/check-markdown.test.js (3)
61-73: Enhance test assertions for multiple validation errors.The current test only checks the number of errors but not their specific content. Consider adding more specific assertions to ensure the exact validation messages are present.
Consider updating the test like this:
const errors = validateBlogs(frontmatter); -expect(errors.length).toBeGreaterThan(3); +expect(errors).toEqual(expect.arrayContaining([ + 'Title must be a string', + 'Date must be a valid date string', + 'Tags must be an array', + 'Cover must be a string', + 'Authors must be an array' +]));🧰 Tools
🪛 eslint
[error] 61-61: Delete
··(prettier/prettier)
[error] 62-62: Replace
········with····(prettier/prettier)
[error] 63-63: Delete
······(prettier/prettier)
[error] 64-64: Replace
············with······(prettier/prettier)
[error] 65-65: Replace
············with······(prettier/prettier)
[error] 66-66: Delete
······(prettier/prettier)
[error] 67-67: Replace
············with······(prettier/prettier)
[error] 68-68: Replace
············with······(prettier/prettier)
[error] 69-69: Delete
····(prettier/prettier)
[error] 70-70: Delete
····(prettier/prettier)
[error] 72-72: Replace
········with····(prettier/prettier)
[error] 73-73: Delete
··(prettier/prettier)
24-83: Add test cases for edge cases.The test suite would benefit from additional test cases covering:
- Empty frontmatter
- Malformed YAML
- Empty authors array
- Empty tags array
Would you like me to help generate these additional test cases?
🧰 Tools
🪛 eslint
[error] 24-24: Delete
··(prettier/prettier)
[error] 25-25: Delete
····(prettier/prettier)
[error] 26-26: Delete
······(prettier/prettier)
[error] 27-27: Replace
············with······(prettier/prettier)
[error] 28-28: Delete
······(prettier/prettier)
[error] 29-29: Replace
············with······(prettier/prettier)
[error] 30-30: Replace
············with······(prettier/prettier)
[error] 31-31: Delete
······(prettier/prettier)
[error] 32-32: Delete
····(prettier/prettier)
[error] 34-34: Delete
····(prettier/prettier)
[error] 35-35: Replace
········expect(errors).toEqual(with····expect(errors).toEqual(⏎······(prettier/prettier)
[error] 36-36: Replace
············with········(prettier/prettier)
[error] 37-37: Delete
····(prettier/prettier)
[error] 38-38: Replace
············with········(prettier/prettier)
[error] 39-39: Replace
··])with])⏎····(prettier/prettier)
[error] 40-40: Delete
··(prettier/prettier)
[error] 42-42: Delete
··(prettier/prettier)
[error] 43-43: Delete
····(prettier/prettier)
[error] 44-44: Replace
········with····(prettier/prettier)
[error] 45-47: Replace
····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number'withexpect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
[error] 48-48: Replace
········])with····(prettier/prettier)
[error] 49-49: Delete
··(prettier/prettier)
[error] 51-51: Delete
··(prettier/prettier)
[error] 52-52: Delete
····(prettier/prettier)
[error] 53-53: Replace
········with····(prettier/prettier)
[error] 55-55: Delete
····(prettier/prettier)
[error] 57-57: Delete
····(prettier/prettier)
[error] 58-58: Replace
········with····(prettier/prettier)
[error] 59-59: Delete
··(prettier/prettier)
[error] 61-61: Delete
··(prettier/prettier)
[error] 62-62: Replace
········with····(prettier/prettier)
[error] 63-63: Delete
······(prettier/prettier)
[error] 64-64: Replace
············with······(prettier/prettier)
[error] 65-65: Replace
············with······(prettier/prettier)
[error] 66-66: Delete
······(prettier/prettier)
[error] 67-67: Replace
············with······(prettier/prettier)
[error] 68-68: Replace
············with······(prettier/prettier)
[error] 69-69: Delete
····(prettier/prettier)
[error] 70-70: Delete
····(prettier/prettier)
[error] 72-72: Replace
········with····(prettier/prettier)
[error] 73-73: Delete
··(prettier/prettier)
[error] 75-75: Delete
··(prettier/prettier)
[error] 76-76: Replace
········with····(prettier/prettier)
[error] 77-77: Delete
····(prettier/prettier)
[error] 77-77: 'mockConsoleError' is already declared in the upper scope on line 12 column 9.
(no-shadow)
[error] 78-78: Delete
····(prettier/prettier)
[error] 79-79: Delete
····(prettier/prettier)
[error] 80-80: Delete
····(prettier/prettier)
[error] 81-81: Replace
········with····(prettier/prettier)
[error] 82-82: Delete
····(prettier/prettier)
1-85: Fix formatting issues throughout the file.The file has numerous formatting inconsistencies. Consider running Prettier to automatically fix these issues.
Run the following command to fix formatting:
npx prettier --write tests/markdown/check-markdown.test.js🧰 Tools
🪛 eslint
[error] 4-8: Replace
⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎with·validateBlogs,·validateDocs,·checkMarkdownFiles·(prettier/prettier)
[error] 11-11: Delete
··(prettier/prettier)
[error] 12-12: Delete
··(prettier/prettier)
[error] 14-14: Replace
····with··(prettier/prettier)
[error] 15-15: Delete
····(prettier/prettier)
[error] 16-16: Delete
····(prettier/prettier)
[error] 17-17: Replace
····with··(prettier/prettier)
[error] 19-19: Delete
··(prettier/prettier)
[error] 20-20: Replace
········with····(prettier/prettier)
[error] 21-21: Replace
········with····(prettier/prettier)
[error] 22-22: Delete
··(prettier/prettier)
[error] 24-24: Delete
··(prettier/prettier)
[error] 25-25: Delete
····(prettier/prettier)
[error] 26-26: Delete
······(prettier/prettier)
[error] 27-27: Replace
············with······(prettier/prettier)
[error] 28-28: Delete
······(prettier/prettier)
[error] 29-29: Replace
············with······(prettier/prettier)
[error] 30-30: Replace
············with······(prettier/prettier)
[error] 31-31: Delete
······(prettier/prettier)
[error] 32-32: Delete
····(prettier/prettier)
[error] 34-34: Delete
····(prettier/prettier)
[error] 35-35: Replace
········expect(errors).toEqual(with····expect(errors).toEqual(⏎······(prettier/prettier)
[error] 36-36: Replace
············with········(prettier/prettier)
[error] 37-37: Delete
····(prettier/prettier)
[error] 38-38: Replace
············with········(prettier/prettier)
[error] 39-39: Replace
··])with])⏎····(prettier/prettier)
[error] 40-40: Delete
··(prettier/prettier)
[error] 42-42: Delete
··(prettier/prettier)
[error] 43-43: Delete
····(prettier/prettier)
[error] 44-44: Replace
········with····(prettier/prettier)
[error] 45-47: Replace
····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number'withexpect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
[error] 48-48: Replace
········])with····(prettier/prettier)
[error] 49-49: Delete
··(prettier/prettier)
[error] 51-51: Delete
··(prettier/prettier)
[error] 52-52: Delete
····(prettier/prettier)
[error] 53-53: Replace
········with····(prettier/prettier)
[error] 55-55: Delete
····(prettier/prettier)
[error] 57-57: Delete
····(prettier/prettier)
[error] 58-58: Replace
········with····(prettier/prettier)
[error] 59-59: Delete
··(prettier/prettier)
[error] 61-61: Delete
··(prettier/prettier)
[error] 62-62: Replace
········with····(prettier/prettier)
[error] 63-63: Delete
······(prettier/prettier)
[error] 64-64: Replace
············with······(prettier/prettier)
[error] 65-65: Replace
············with······(prettier/prettier)
[error] 66-66: Delete
······(prettier/prettier)
[error] 67-67: Replace
············with······(prettier/prettier)
[error] 68-68: Replace
············with······(prettier/prettier)
[error] 69-69: Delete
····(prettier/prettier)
[error] 70-70: Delete
····(prettier/prettier)
[error] 72-72: Replace
········with····(prettier/prettier)
[error] 73-73: Delete
··(prettier/prettier)
[error] 75-75: Delete
··(prettier/prettier)
[error] 76-76: Replace
········with····(prettier/prettier)
[error] 77-77: Delete
····(prettier/prettier)
[error] 77-77: 'mockConsoleError' is already declared in the upper scope on line 12 column 9.
(no-shadow)
[error] 78-78: Delete
····(prettier/prettier)
[error] 79-79: Delete
····(prettier/prettier)
[error] 80-80: Delete
····(prettier/prettier)
[error] 81-81: Replace
········with····(prettier/prettier)
[error] 82-82: Delete
····(prettier/prettier)
[error] 83-84: Replace
··});·⏎·······with});(prettier/prettier)
scripts/markdown/check-markdown.js (1)
134-141: Improve script execution and error handling.The script execution could be enhanced to properly handle errors and provide better feedback.
Consider wrapping the execution in an async function:
const docsFolderPath = path.resolve(__dirname, '../../markdown/docs'); const blogsFolderPath = path.resolve(__dirname, '../../markdown/blog'); -checkMarkdownFiles(docsFolderPath, validateDocs); -checkMarkdownFiles(blogsFolderPath, validateBlogs); +async function main() { + try { + await Promise.all([ + checkMarkdownFiles(docsFolderPath, validateDocs), + checkMarkdownFiles(blogsFolderPath, validateBlogs) + ]); + } catch (err) { + console.error('Failed to check markdown files:', err); + process.exit(1); + } +} + +if (require.main === module) { + main(); +} module.exports = { validateBlogs, validateDocs, checkMarkdownFiles };🧰 Tools
🪛 eslint
[error] 141-141: Insert
;(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(2 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🪛 GitHub Check: codecov/patch
scripts/markdown/check-markdown.js
[warning] 110-110: scripts/markdown/check-markdown.js#L110
Added line #L110 was not covered by tests
🪛 eslint
scripts/markdown/check-markdown.js
[error] 102-102: Delete ··
(prettier/prettier)
[error] 103-103: Replace ········ with ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
[error] 104-129: iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations.
(no-restricted-syntax)
[error] 105-105: Replace ············ with ······
(prettier/prettier)
[error] 106-106: Delete ······
(prettier/prettier)
[error] 108-108: Replace ············ with ······
(prettier/prettier)
[error] 109-109: Delete ······
(prettier/prettier)
[error] 110-110: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Unexpected use of continue statement.
(no-continue)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 113-113: Replace ············ with ······
(prettier/prettier)
[error] 113-113: Unexpected await inside a loop.
(no-await-in-loop)
[error] 115-115: Delete ······
(prettier/prettier)
[error] 116-116: Replace ············ with ······
(prettier/prettier)
[error] 117-117: Delete ········
(prettier/prettier)
[error] 117-117: Unexpected await inside a loop.
(no-await-in-loop)
[error] 118-118: Replace ············ with ······
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 119-119: Unexpected await inside a loop.
(no-await-in-loop)
[error] 120-120: Delete ········
(prettier/prettier)
[error] 122-122: Replace ················ with ········
(prettier/prettier)
[error] 123-123: Replace ················ with ········
(prettier/prettier)
[error] 124-124: Delete ··········
(prettier/prettier)
[error] 125-125: Replace ····················errors.forEach(error with ··········errors.forEach((error)
(prettier/prettier)
[error] 126-126: Replace ···················· with ··········
(prettier/prettier)
[error] 127-127: Replace ················ with ········
(prettier/prettier)
[error] 128-128: Replace ············ with ······
(prettier/prettier)
[error] 129-129: Replace ········ with ····
(prettier/prettier)
[error] 130-130: Delete ··
(prettier/prettier)
[error] 131-131: Delete ····
(prettier/prettier)
[error] 132-132: Delete ··
(prettier/prettier)
[error] 141-141: Insert ;
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 4-8: Replace ⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 11-11: Delete ··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 14-14: Replace ···· with ··
(prettier/prettier)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ···· with ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Replace ········ with ····
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ··
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
[error] 25-25: Delete ····
(prettier/prettier)
[error] 26-26: Delete ······
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Delete ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Delete ······
(prettier/prettier)
[error] 32-32: Delete ····
(prettier/prettier)
[error] 34-34: Delete ····
(prettier/prettier)
[error] 35-35: Replace ········expect(errors).toEqual( with ····expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 36-36: Replace ············ with ········
(prettier/prettier)
[error] 37-37: Delete ····
(prettier/prettier)
[error] 38-38: Replace ············ with ········
(prettier/prettier)
[error] 39-39: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 40-40: Delete ··
(prettier/prettier)
[error] 42-42: Delete ··
(prettier/prettier)
[error] 43-43: Delete ····
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-47: Replace ····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 48-48: Replace ········]) with ····
(prettier/prettier)
[error] 49-49: Delete ··
(prettier/prettier)
[error] 51-51: Delete ··
(prettier/prettier)
[error] 52-52: Delete ····
(prettier/prettier)
[error] 53-53: Replace ········ with ····
(prettier/prettier)
[error] 55-55: Delete ····
(prettier/prettier)
[error] 57-57: Delete ····
(prettier/prettier)
[error] 58-58: Replace ········ with ····
(prettier/prettier)
[error] 59-59: Delete ··
(prettier/prettier)
[error] 61-61: Delete ··
(prettier/prettier)
[error] 62-62: Replace ········ with ····
(prettier/prettier)
[error] 63-63: Delete ······
(prettier/prettier)
[error] 64-64: Replace ············ with ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Delete ····
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 72-72: Replace ········ with ····
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 75-75: Delete ··
(prettier/prettier)
[error] 76-76: Replace ········ with ····
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 77-77: 'mockConsoleError' is already declared in the upper scope on line 12 column 9.
(no-shadow)
[error] 78-78: Delete ····
(prettier/prettier)
[error] 79-79: Delete ····
(prettier/prettier)
[error] 80-80: Delete ····
(prettier/prettier)
[error] 81-81: Replace ········ with ····
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 83-84: Replace ··});·⏎······· with });
(prettier/prettier)
🔇 Additional comments (1)
scripts/markdown/check-markdown.js (1)
1-1: 🛠️ Refactor suggestion
Consider using synchronous file operations.
A previous implementation (PR #3301) found that synchronous operations (fs.readFileSync) are preferred when processing large markdown files with recursion and loops. The current change to fs.promises might impact performance.
Let's verify the performance impact:
Consider reverting to synchronous operations:
-const fs = require('fs').promises;
+const fs = require('fs');| let tempDir; | ||
| let mockConsoleError; | ||
|
|
There was a problem hiding this comment.
Fix variable shadowing of mockConsoleError.
The mockConsoleError variable is already declared in the outer scope (line 12) but is redeclared in the test case (line 77). This could lead to confusion and potential issues.
Apply this fix:
it('logs error to console when an error occurs in checkMarkdownFiles', async () => {
const invalidFolderPath = path.join(tempDir, 'non-existent-folder');
- const mockConsoleError = jest.spyOn(console, 'error').mockImplementation();
await checkMarkdownFiles(invalidFolderPath, validateBlogs);
expect(mockConsoleError.mock.calls[0][0]).toContain('Error processing files:');
- mockConsoleError.mockRestore();
});Also applies to: 75-82
🧰 Tools
🪛 eslint
[error] 11-11: Delete ··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
scripts/markdown/check-markdown.js
Outdated
| async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') { | ||
| try { | ||
| const files = await fs.readdir(folderPath); | ||
| for (const file of files) { | ||
| const filePath = path.join(folderPath, file); | ||
| const relativeFilePath = path.join(relativePath, file); | ||
|
|
||
| // Skip the folder 'docs/reference/specification' | ||
| if (relativeFilePath.includes('reference/specification')) { | ||
| return; | ||
| continue; | ||
| } | ||
|
|
||
| fs.stat(filePath, (err, stats) => { | ||
| if (err) { | ||
| console.error('Error reading file stats:', err); | ||
| return; | ||
| } | ||
| const stats = await fs.stat(filePath); | ||
|
|
||
| // Recurse if directory, otherwise validate markdown file | ||
| if (stats.isDirectory()) { | ||
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | ||
| } else if (path.extname(file) === '.md') { | ||
| const fileContent = await fs.readFile(filePath, 'utf-8'); | ||
| const { data: frontmatter } = matter(fileContent); | ||
|
|
||
| // Recurse if directory, otherwise validate markdown file | ||
| if (stats.isDirectory()) { | ||
| checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | ||
| } else if (path.extname(file) === '.md') { | ||
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | ||
| const { data: frontmatter } = matter(fileContent); | ||
|
|
||
| const errors = validateFunction(frontmatter); | ||
| if (errors) { | ||
| console.log(`Errors in file ${relativeFilePath}:`); | ||
| errors.forEach(error => console.log(` - ${error}`)); | ||
| process.exitCode = 1; | ||
| } | ||
| const errors = validateFunction(frontmatter); | ||
| if (errors) { | ||
| console.log(`Errors in file ${relativeFilePath}:`); | ||
| errors.forEach(error => console.log(` - ${error}`)); | ||
| process.exitCode = 1; | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error('Error processing files:', err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and optimize async operations.
Several improvements could be made to this function:
- The error handling is too generic
- Multiple
awaitoperations inside the loop could impact performance - The 'reference/specification' skip condition needs test coverage
Consider these improvements:
async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') {
try {
const files = await fs.readdir(folderPath);
+ const filePromises = files.map(async file => {
const filePath = path.join(folderPath, file);
const relativeFilePath = path.join(relativePath, file);
if (relativeFilePath.includes('reference/specification')) {
return;
}
- const stats = await fs.stat(filePath);
+ try {
+ const stats = await fs.stat(filePath);
- if (stats.isDirectory()) {
- await checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
- } else if (path.extname(file) === '.md') {
- const fileContent = await fs.readFile(filePath, 'utf-8');
- const { data: frontmatter } = matter(fileContent);
+ if (stats.isDirectory()) {
+ await checkMarkdownFiles(filePath, validateFunction, relativeFilePath);
+ } else if (path.extname(file) === '.md') {
+ const fileContent = await fs.readFile(filePath, 'utf-8');
+ const { data: frontmatter } = matter(fileContent);
- const errors = validateFunction(frontmatter);
- if (errors) {
- console.log(`Errors in file ${relativeFilePath}:`);
- errors.forEach(error => console.log(` - ${error}`));
- process.exitCode = 1;
+ const errors = validateFunction(frontmatter);
+ if (errors) {
+ console.log(`Errors in file ${relativeFilePath}:`);
+ errors.forEach(error => console.log(` - ${error}`));
+ process.exitCode = 1;
+ }
}
+ } catch (err) {
+ console.error(`Error processing file ${relativeFilePath}:`, err);
+ throw err;
}
- }
+ });
+ await Promise.all(filePromises);
} catch (err) {
- console.error('Error processing files:', err);
+ console.error(`Error in directory ${folderPath}:`, err);
+ throw 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.
| async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') { | |
| try { | |
| const files = await fs.readdir(folderPath); | |
| for (const file of files) { | |
| const filePath = path.join(folderPath, file); | |
| const relativeFilePath = path.join(relativePath, file); | |
| // Skip the folder 'docs/reference/specification' | |
| if (relativeFilePath.includes('reference/specification')) { | |
| return; | |
| continue; | |
| } | |
| fs.stat(filePath, (err, stats) => { | |
| if (err) { | |
| console.error('Error reading file stats:', err); | |
| return; | |
| } | |
| const stats = await fs.stat(filePath); | |
| // Recurse if directory, otherwise validate markdown file | |
| if (stats.isDirectory()) { | |
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = await fs.readFile(filePath, 'utf-8'); | |
| const { data: frontmatter } = matter(fileContent); | |
| // Recurse if directory, otherwise validate markdown file | |
| if (stats.isDirectory()) { | |
| checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = fs.readFileSync(filePath, 'utf-8'); | |
| const { data: frontmatter } = matter(fileContent); | |
| const errors = validateFunction(frontmatter); | |
| if (errors) { | |
| console.log(`Errors in file ${relativeFilePath}:`); | |
| errors.forEach(error => console.log(` - ${error}`)); | |
| process.exitCode = 1; | |
| } | |
| const errors = validateFunction(frontmatter); | |
| if (errors) { | |
| console.log(`Errors in file ${relativeFilePath}:`); | |
| errors.forEach(error => console.log(` - ${error}`)); | |
| process.exitCode = 1; | |
| } | |
| }); | |
| }); | |
| }); | |
| } | |
| } | |
| } catch (err) { | |
| console.error('Error processing files:', err); | |
| } | |
| async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') { | |
| try { | |
| const files = await fs.readdir(folderPath); | |
| const filePromises = files.map(async file => { | |
| const filePath = path.join(folderPath, file); | |
| const relativeFilePath = path.join(relativePath, file); | |
| if (relativeFilePath.includes('reference/specification')) { | |
| return; | |
| } | |
| try { | |
| const stats = await fs.stat(filePath); | |
| if (stats.isDirectory()) { | |
| await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); | |
| } else if (path.extname(file) === '.md') { | |
| const fileContent = await fs.readFile(filePath, 'utf-8'); | |
| const { data: frontmatter } = matter(fileContent); | |
| const errors = validateFunction(frontmatter); | |
| if (errors) { | |
| console.log(`Errors in file ${relativeFilePath}:`); | |
| errors.forEach(error => console.log(` - ${error}`)); | |
| process.exitCode = 1; | |
| } | |
| } | |
| } catch (err) { | |
| console.error(`Error processing file ${relativeFilePath}:`, err); | |
| throw err; | |
| } | |
| }); | |
| await Promise.all(filePromises); | |
| } catch (err) { | |
| console.error(`Error in directory ${folderPath}:`, err); | |
| throw err; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: scripts/markdown/check-markdown.js#L110
Added line #L110 was not covered by tests
🪛 eslint
[error] 102-102: Delete ··
(prettier/prettier)
[error] 103-103: Replace ········ with ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
[error] 104-129: iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations.
(no-restricted-syntax)
[error] 105-105: Replace ············ with ······
(prettier/prettier)
[error] 106-106: Delete ······
(prettier/prettier)
[error] 108-108: Replace ············ with ······
(prettier/prettier)
[error] 109-109: Delete ······
(prettier/prettier)
[error] 110-110: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Unexpected use of continue statement.
(no-continue)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 113-113: Replace ············ with ······
(prettier/prettier)
[error] 113-113: Unexpected await inside a loop.
(no-await-in-loop)
[error] 115-115: Delete ······
(prettier/prettier)
[error] 116-116: Replace ············ with ······
(prettier/prettier)
[error] 117-117: Delete ········
(prettier/prettier)
[error] 117-117: Unexpected await inside a loop.
(no-await-in-loop)
[error] 118-118: Replace ············ with ······
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 119-119: Unexpected await inside a loop.
(no-await-in-loop)
[error] 120-120: Delete ········
(prettier/prettier)
[error] 122-122: Replace ················ with ········
(prettier/prettier)
[error] 123-123: Replace ················ with ········
(prettier/prettier)
[error] 124-124: Delete ··········
(prettier/prettier)
[error] 125-125: Replace ····················errors.forEach(error with ··········errors.forEach((error)
(prettier/prettier)
[error] 126-126: Replace ···················· with ··········
(prettier/prettier)
[error] 127-127: Replace ················ with ········
(prettier/prettier)
[error] 128-128: Replace ············ with ······
(prettier/prettier)
[error] 129-129: Replace ········ with ····
(prettier/prettier)
[error] 130-130: Delete ··
(prettier/prettier)
[error] 131-131: Delete ····
(prettier/prettier)
[error] 132-132: Delete ··
(prettier/prettier)
There was a problem hiding this comment.
@vishvamsinh28 Can you 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!
There was a problem hiding this comment.
@akshatnema Done
|
@akshatnema Please review this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
tests/markdown/check-markdown.test.js (2)
42-49: Enhance docs frontmatter validation test coverage.Consider adding more edge cases to thoroughly test the validation:
- Empty string for title
- Negative or decimal values for weight
- Missing fields
it('validates docs frontmatter for required fields', async () => { - const frontmatter = { title: 123, weight: 'not-a-number' }; + const testCases = [ + { frontmatter: { title: 123, weight: 'not-a-number' }, expectedError: 'Title is missing or not a string' }, + { frontmatter: { title: '', weight: 1 }, expectedError: 'Title cannot be empty' }, + { frontmatter: { title: 'Valid', weight: -1 }, expectedError: 'Weight must be a positive number' }, + { frontmatter: {}, expectedError: 'Title is required' } + ]; + + for (const { frontmatter, expectedError } of testCases) { + const errors = validateDocs(frontmatter); + expect(errors).toContain(expectedError); + } });🧰 Tools
🪛 eslint
[error] 42-42: Delete
··(prettier/prettier)
[error] 43-43: Replace
········with····(prettier/prettier)
[error] 44-44: Replace
········with····(prettier/prettier)
[error] 45-47: Replace
····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number'withexpect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
[error] 48-48: Delete
····])(prettier/prettier)
[error] 49-49: Replace
····with··(prettier/prettier)
61-73: Make validation error expectations more specific.The current test only checks the number of errors. Consider asserting specific error messages to ensure the validation logic is working as expected.
it('returns multiple validation errors for invalid blog frontmatter', async () => { const frontmatter = { title: 123, date: 'invalid-date', type: 'blog', tags: 'not-an-array', cover: ['not-a-string'], authors: { name: 'John Doe' } }; const errors = validateBlogs(frontmatter); - expect(errors.length).toBeGreaterThan(3); + expect(errors).toEqual(expect.arrayContaining([ + 'Title must be a string', + 'Date must be a valid date string', + 'Tags must be an array', + 'Cover must be a string', + 'Authors must be an array' + ])); });🧰 Tools
🪛 eslint
[error] 61-61: Replace
····with··(prettier/prettier)
[error] 62-62: Delete
····(prettier/prettier)
[error] 63-63: Replace
············with······(prettier/prettier)
[error] 64-64: Delete
······(prettier/prettier)
[error] 65-65: Replace
············with······(prettier/prettier)
[error] 66-66: Replace
············with······(prettier/prettier)
[error] 67-67: Delete
······(prettier/prettier)
[error] 68-68: Replace
············with······(prettier/prettier)
[error] 69-69: Delete
····(prettier/prettier)
[error] 70-70: Replace
········with····(prettier/prettier)
[error] 72-72: Replace
········with····(prettier/prettier)
[error] 73-73: Delete
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 4-8: Replace ⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 11-11: Delete ··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 14-14: Replace ···· with ··
(prettier/prettier)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ···· with ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Replace ········ with ····
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ··
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
[error] 25-25: Delete ····
(prettier/prettier)
[error] 26-26: Delete ······
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Delete ······
(prettier/prettier)
[error] 29-29: Delete ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Delete ······
(prettier/prettier)
[error] 32-32: Delete ····
(prettier/prettier)
[error] 34-34: Delete ····
(prettier/prettier)
[error] 35-35: Replace ········expect(errors).toEqual( with ····expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 36-36: Delete ····
(prettier/prettier)
[error] 37-37: Delete ····
(prettier/prettier)
[error] 38-38: Replace ············ with ········
(prettier/prettier)
[error] 39-39: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 40-40: Delete ··
(prettier/prettier)
[error] 42-42: Delete ··
(prettier/prettier)
[error] 43-43: Replace ········ with ····
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-47: Replace ····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 48-48: Delete ····])
(prettier/prettier)
[error] 49-49: Replace ···· with ··
(prettier/prettier)
[error] 51-51: Delete ··
(prettier/prettier)
[error] 52-52: Delete ····
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 55-55: Delete ····
(prettier/prettier)
[error] 57-57: Replace ········ with ····
(prettier/prettier)
[error] 58-58: Delete ····
(prettier/prettier)
[error] 59-59: Delete ··
(prettier/prettier)
[error] 61-61: Replace ···· with ··
(prettier/prettier)
[error] 62-62: Delete ····
(prettier/prettier)
[error] 63-63: Replace ············ with ······
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Replace ············ with ······
(prettier/prettier)
[error] 67-67: Delete ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Delete ····
(prettier/prettier)
[error] 70-70: Replace ········ with ····
(prettier/prettier)
[error] 72-72: Replace ········ with ····
(prettier/prettier)
[error] 73-73: Delete ··
(prettier/prettier)
[error] 75-75: Delete ··
(prettier/prettier)
[error] 76-76: Delete ····
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 77-77: 'mockConsoleError' is already declared in the upper scope on line 12 column 9.
(no-shadow)
[error] 78-78: Delete ····
(prettier/prettier)
[error] 79-79: Delete ····
(prettier/prettier)
[error] 80-80: Delete ····
(prettier/prettier)
[error] 81-81: Delete ····
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 83-83: Replace ··});····· with });
(prettier/prettier)
🔇 Additional comments (2)
tests/markdown/check-markdown.test.js (2)
14-22: LGTM! Well-structured test lifecycle hooks.
The setup and teardown logic is well implemented with proper async handling and cleanup.
🧰 Tools
🪛 eslint
[error] 14-14: Replace ···· with ··
(prettier/prettier)
[error] 15-15: Delete ····
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ···· with ··
(prettier/prettier)
[error] 19-19: Delete ··
(prettier/prettier)
[error] 20-20: Replace ········ with ····
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ··
(prettier/prettier)
11-13:
Fix variable shadowing of mockConsoleError.
The mockConsoleError variable is already declared in the outer scope and is shadowed in a test case below (line 77). This could lead to confusion and potential issues.
🧰 Tools
🪛 eslint
[error] 11-11: Delete ··
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
|
@akshatnema Tests updated |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/markdown/check-markdown.test.js (2)
20-23: Add error handling to afterEach cleanup.The teardown could fail silently if there's an issue cleaning up the temporary directory.
afterEach(async () => { mockConsoleError.mockRestore(); - await fs.rm(tempDir, { recursive: true, force: true }); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Failed to clean up temp directory: ${error.message}`); + } });🧰 Tools
🪛 eslint
[error] 20-20: Delete
··(prettier/prettier)
[error] 21-21: Replace
········with····(prettier/prettier)
[error] 22-22: Delete
····(prettier/prettier)
[error] 23-23: Replace
····with··(prettier/prettier)
117-130: Expand main function error test coverage.The test only covers a single error scenario (readdir failure). Consider testing other potential failure points.
it('should handle main function errors', async () => { const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => { }); - jest.spyOn(fs, 'readdir').mockRejectedValue(new Error('Test error')); + + // Test readdir failure + jest.spyOn(fs, 'readdir').mockRejectedValueOnce(new Error('readdir error')); + await main(); + expect(mockConsoleError).toHaveBeenCalledWith( + 'Failed to validate markdown files:', + expect.any(Error) + ); + expect(mockExit).toHaveBeenCalledWith(1); + + // Test validation failure + jest.spyOn(fs, 'readdir').mockResolvedValueOnce(['test.md']); + jest.spyOn(fs, 'readFile').mockResolvedValueOnce('---\ntitle: 123\n---'); + await main(); + expect(mockConsoleError).toHaveBeenCalledWith( + expect.stringContaining('Errors in file test.md:') + ); + expect(mockExit).toHaveBeenCalledWith(1); - await main(); - - expect(mockConsoleError).toHaveBeenCalledWith( - 'Failed to validate markdown files:', - expect.any(Error) - ); - expect(mockExit).toHaveBeenCalledWith(1); - mockExit.mockRestore(); });🧰 Tools
🪛 eslint
[error] 117-117: Delete
··(prettier/prettier)
[error] 118-118: Replace
········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{·with····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{(prettier/prettier)
[error] 119-119: Delete
····(prettier/prettier)
[error] 121-121: Replace
········with····(prettier/prettier)
[error] 123-126: Replace
········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········with····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)(prettier/prettier)
[error] 127-127: Delete
····(prettier/prettier)
[error] 129-129: Delete
····(prettier/prettier)
scripts/markdown/check-markdown.js (2)
Line range hint
101-140: Consider enhancing error handling and skip condition clarityWhile the async implementation is good, consider these improvements:
- Add more context to error messages
- Make the skip condition more maintainable
Consider this refactoring:
async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') { + const SKIP_PATHS = ['reference/specification']; try { const files = await fs.readdir(folderPath); const filePromises = files.map(async (file) => { const filePath = path.join(folderPath, file); const relativeFilePath = path.join(relativePath, file); - if (relativeFilePath.includes('reference/specification')) { + if (SKIP_PATHS.some(skipPath => relativeFilePath.includes(skipPath))) { return; } try { const stats = await fs.stat(filePath); if (stats.isDirectory()) { await checkMarkdownFiles(filePath, validateFunction, relativeFilePath); } else if (path.extname(file) === '.md') { const fileContent = await fs.readFile(filePath, 'utf-8'); const { data: frontmatter } = matter(fileContent); const errors = validateFunction(frontmatter); if (errors) { - console.log(`Errors in file ${relativeFilePath}:`); + console.error(`Validation errors in markdown file ${relativeFilePath}:`); errors.forEach(error => console.log(` - ${error}`)); process.exitCode = 1; } } } catch (err) { - console.error(`Error processing file ${relativeFilePath}:`, err); + console.error(`Failed to process markdown file ${relativeFilePath}:`, err.message); throw err; } }); await Promise.all(filePromises); } catch (err) { - console.error(`Error in directory ${folderPath}:`, err); + console.error(`Failed to process directory ${folderPath}:`, err.message); throw err; } }🧰 Tools
🪛 eslint
[error] 110-110: Delete
········(prettier/prettier)
[error] 111-111: Delete
······(prettier/prettier)
[error] 113-113: Delete
······(prettier/prettier)
[error] 114-114: Delete
········(prettier/prettier)
[error] 116-116: Delete
········(prettier/prettier)
[error] 117-117: Replace
················with········(prettier/prettier)
[error] 118-118: Delete
··········(prettier/prettier)
[error] 119-119: Replace
················with········(prettier/prettier)
[error] 120-120: Delete
··········(prettier/prettier)
[error] 121-121: Replace
····················with··········(prettier/prettier)
[error] 123-123: Replace
····················with··········(prettier/prettier)
146-156: LGTM: Well-structured main function with proper error handlingThe main function effectively orchestrates the validation process. Consider adding more detailed logging to help with debugging:
async function main() { try { + console.log('Starting markdown validation...'); await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs), checkMarkdownFiles(blogsFolderPath, validateBlogs) ]); + console.log('Markdown validation completed successfully'); } catch (error) { - console.error('Failed to validate markdown files:', error); + console.error('Failed to validate markdown files:', error.message); + console.error('Stack trace:', error.stack); process.exit(1); } }🧰 Tools
🪛 eslint
[error] 147-147: Delete
··(prettier/prettier)
[error] 148-148: Replace
········with····(prettier/prettier)
[error] 149-149: Delete
······(prettier/prettier)
[error] 150-150: Replace
············with······(prettier/prettier)
[error] 151-151: Replace
········with····(prettier/prettier)
[error] 152-152: Delete
··(prettier/prettier)
[error] 153-153: Delete
····(prettier/prettier)
[error] 154-154: Delete
····(prettier/prettier)
[error] 155-155: Delete
··(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(4 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (4)
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.852Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
🪛 eslint
scripts/markdown/check-markdown.js
[error] 102-102: Replace ···· with ··
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: Delete ········
(prettier/prettier)
[error] 116-116: Delete ········
(prettier/prettier)
[error] 117-117: Replace ················ with ········
(prettier/prettier)
[error] 118-118: Delete ··········
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 120-120: Delete ··········
(prettier/prettier)
[error] 130-130: Replace ············ with ······
(prettier/prettier)
[error] 131-131: Delete ········
(prettier/prettier)
[error] 132-132: Delete ········
(prettier/prettier)
[error] 133-133: Delete ······
(prettier/prettier)
[error] 134-134: Replace ········ with ····
(prettier/prettier)
[error] 136-136: Delete ····
(prettier/prettier)
[error] 137-137: Delete ··
(prettier/prettier)
[error] 138-138: Delete ····
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 140-140: Delete ··
(prettier/prettier)
[error] 147-147: Delete ··
(prettier/prettier)
[error] 148-148: Replace ········ with ····
(prettier/prettier)
[error] 149-149: Delete ······
(prettier/prettier)
[error] 150-150: Replace ············ with ······
(prettier/prettier)
[error] 151-151: Replace ········ with ····
(prettier/prettier)
[error] 152-152: Delete ··
(prettier/prettier)
[error] 153-153: Delete ····
(prettier/prettier)
[error] 154-154: Delete ····
(prettier/prettier)
[error] 155-155: Delete ··
(prettier/prettier)
[error] 160-160: Insert ;
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 4-9: Replace ⏎····main,⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·main,·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 15-15: Replace ···· with ··
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ········ with ····
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ····
(prettier/prettier)
[error] 23-23: Replace ···· with ··
(prettier/prettier)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Delete ······
(prettier/prettier)
[error] 33-33: Replace ········ with ····
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ····expect(errors).toEqual( with expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 37-37: Delete ····
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
[error] 43-43: Delete ··
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-45: Delete ····
(prettier/prettier)
[error] 46-48: Replace ········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with ····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 49-49: Replace ········]) with ····
(prettier/prettier)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 52-52: Replace ···· with ··
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 54-54: Replace ········ with ····
(prettier/prettier)
[error] 56-56: Delete ····
(prettier/prettier)
[error] 58-58: Replace ········ with ····
(prettier/prettier)
[error] 59-59: Delete ····
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Delete ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Delete ····
(prettier/prettier)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Delete ··
(prettier/prettier)
[error] 76-76: Replace ···· with ··
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 79-80: Replace ········await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))⏎············ with ····await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 83-83: Delete ··
(prettier/prettier)
[error] 85-85: Replace ···· with ··
(prettier/prettier)
[error] 86-86: Delete ····
(prettier/prettier)
[error] 87-87: Replace ········ with ····
(prettier/prettier)
[error] 88-88: Delete ····
(prettier/prettier)
[error] 90-90: Replace ········ with ····
(prettier/prettier)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Replace ········expect(mockConsoleLog).not.toHaveBeenCalledWith(expect.stringContaining('Errors·in·file·reference/specification/skipped.md')); with ····expect(mockConsoleLog).not.toHaveBeenCalledWith(⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 95-95: Insert );⏎
(prettier/prettier)
[error] 96-96: Delete ··
(prettier/prettier)
[error] 98-98: Delete ··
(prettier/prettier)
[error] 99-99: Replace ········ with ····
(prettier/prettier)
[error] 100-100: Delete ····
(prettier/prettier)
[error] 102-102: Replace ········ with ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
[error] 105-105: Replace ············ with ······
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 107-107: Delete ······
(prettier/prettier)
[error] 108-108: Delete ········
(prettier/prettier)
[error] 109-109: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Delete ······
(prettier/prettier)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 112-112: Replace ········ with ····
(prettier/prettier)
[error] 114-114: Delete ····
(prettier/prettier)
[error] 115-115: Delete ··
(prettier/prettier)
[error] 117-117: Delete ··
(prettier/prettier)
[error] 118-118: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 119-119: Delete ····
(prettier/prettier)
[error] 121-121: Replace ········ with ····
(prettier/prettier)
[error] 123-126: Replace ········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with ····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 127-127: Delete ····
(prettier/prettier)
[error] 129-129: Delete ····
(prettier/prettier)
[error] 130-131: Replace ··});⏎ with });
(prettier/prettier)
🔇 Additional comments (3)
tests/markdown/check-markdown.test.js (1)
1-132: Overall test implementation looks good!
The test suite is well-structured with comprehensive coverage of various scenarios. It effectively uses Jest's mocking capabilities and follows good async/await patterns. The suggested improvements above would further enhance the robustness of the tests, but the current implementation is solid.
🧰 Tools
🪛 eslint
[error] 4-9: Replace ⏎····main,⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·main,·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 15-15: Replace ···· with ··
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ········ with ····
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ····
(prettier/prettier)
[error] 23-23: Replace ···· with ··
(prettier/prettier)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Delete ······
(prettier/prettier)
[error] 33-33: Replace ········ with ····
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ····expect(errors).toEqual( with expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 37-37: Delete ····
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
[error] 43-43: Delete ··
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-45: Delete ····
(prettier/prettier)
[error] 46-48: Replace ········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with ····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 49-49: Replace ········]) with ····
(prettier/prettier)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 52-52: Replace ···· with ··
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 54-54: Replace ········ with ····
(prettier/prettier)
[error] 56-56: Delete ····
(prettier/prettier)
[error] 58-58: Replace ········ with ····
(prettier/prettier)
[error] 59-59: Delete ····
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Delete ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Delete ····
(prettier/prettier)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Delete ··
(prettier/prettier)
[error] 76-76: Replace ···· with ··
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 79-80: Replace ········await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))⏎············ with ····await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 83-83: Delete ··
(prettier/prettier)
[error] 85-85: Replace ···· with ··
(prettier/prettier)
[error] 86-86: Delete ····
(prettier/prettier)
[error] 87-87: Replace ········ with ····
(prettier/prettier)
[error] 88-88: Delete ····
(prettier/prettier)
[error] 90-90: Replace ········ with ····
(prettier/prettier)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Replace ········expect(mockConsoleLog).not.toHaveBeenCalledWith(expect.stringContaining('Errors·in·file·reference/specification/skipped.md')); with ····expect(mockConsoleLog).not.toHaveBeenCalledWith(⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 95-95: Insert );⏎
(prettier/prettier)
[error] 96-96: Delete ··
(prettier/prettier)
[error] 98-98: Delete ··
(prettier/prettier)
[error] 99-99: Replace ········ with ····
(prettier/prettier)
[error] 100-100: Delete ····
(prettier/prettier)
[error] 102-102: Replace ········ with ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
[error] 105-105: Replace ············ with ······
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 107-107: Delete ······
(prettier/prettier)
[error] 108-108: Delete ········
(prettier/prettier)
[error] 109-109: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Delete ······
(prettier/prettier)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 112-112: Replace ········ with ····
(prettier/prettier)
[error] 114-114: Delete ····
(prettier/prettier)
[error] 115-115: Delete ··
(prettier/prettier)
[error] 117-117: Delete ··
(prettier/prettier)
[error] 118-118: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 119-119: Delete ····
(prettier/prettier)
[error] 121-121: Replace ········ with ····
(prettier/prettier)
[error] 123-126: Replace ········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with ····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 127-127: Delete ····
(prettier/prettier)
[error] 129-129: Delete ····
(prettier/prettier)
[error] 130-131: Replace ··});⏎ with });
(prettier/prettier)
scripts/markdown/check-markdown.js (2)
1-1: LGTM: Appropriate use of fs.promises
The transition to fs.promises aligns with modern JavaScript practices and enables cleaner async/await syntax.
160-160: LGTM: Appropriate exports for testing
The addition of the main function to the exports enables proper unit testing.
🧰 Tools
🪛 eslint
[error] 160-160: Insert ;
(prettier/prettier)
| it('returns multiple validation errors for invalid blog frontmatter', async () => { | ||
| const frontmatter = { | ||
| title: 123, | ||
| date: 'invalid-date', | ||
| type: 'blog', | ||
| tags: 'not-an-array', | ||
| cover: ['not-a-string'], | ||
| authors: { name: 'John Doe' } | ||
| }; | ||
| const errors = validateBlogs(frontmatter); | ||
|
|
||
| expect(errors.length).toBeGreaterThan(3); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test assertions for multiple validation errors.
The test only checks the number of errors but not their specific content. This could allow incorrect error messages to pass the test.
it('returns multiple validation errors for invalid blog frontmatter', async () => {
const frontmatter = {
title: 123,
date: 'invalid-date',
type: 'blog',
tags: 'not-an-array',
cover: ['not-a-string'],
authors: { name: 'John Doe' }
};
const errors = validateBlogs(frontmatter);
- expect(errors.length).toBeGreaterThan(3);
+ expect(errors).toEqual(expect.arrayContaining([
+ 'Title must be a string',
+ 'Date must be a valid date string',
+ 'Tags must be an array',
+ 'Cover must be a string',
+ 'Authors must be an array'
+ ]));
});Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Delete ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Delete ····
(prettier/prettier)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Delete ··
(prettier/prettier)
There was a problem hiding this comment.
@vishvamsinh28, thank you for making the changes!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/markdown/check-markdown.test.js (3)
20-23: Add error handling in cleanup.The
afterEachhook should handle potential cleanup failures gracefully to prevent test pollution.afterEach(async () => { mockConsoleError.mockRestore(); - await fs.rm(tempDir, { recursive: true, force: true }); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Failed to clean up temp directory: ${error.message}`); + } });🧰 Tools
🪛 eslint
[error] 20-20: Delete
··(prettier/prettier)
[error] 21-21: Replace
········with····(prettier/prettier)
[error] 22-22: Delete
····(prettier/prettier)
[error] 23-23: Replace
····with··(prettier/prettier)
43-50: Add edge cases for docs frontmatter validation.The test could be more comprehensive by including additional scenarios.
it('validates docs frontmatter for required fields', async () => { - const frontmatter = { title: 123, weight: 'not-a-number' }; + const testCases = [ + { input: { title: 123, weight: 'not-a-number' }, + expected: ['Title is missing or not a string', 'Weight is missing or not a number'] }, + { input: { title: '', weight: -1 }, + expected: ['Title is missing or not a string'] }, + { input: {}, + expected: ['Title is missing or not a string', 'Weight is missing or not a number'] } + ]; + + for (const { input, expected } of testCases) { + const errors = validateDocs(input); + expect(errors).toEqual(expect.arrayContaining(expected)); + } });🧰 Tools
🪛 eslint
[error] 43-43: Delete
··(prettier/prettier)
[error] 44-44: Replace
········with····(prettier/prettier)
[error] 45-45: Delete
····(prettier/prettier)
[error] 46-48: Replace
········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number'with····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
[error] 49-49: Delete
····])(prettier/prettier)
[error] 50-50: Delete
··(prettier/prettier)
98-115: Add test for partial file read failure.The test could be enhanced to cover scenarios where the file is partially read.
it('logs and rethrows error when an exception occurs while processing a file', async () => { const filePath = path.join(tempDir, 'invalid.md'); await fs.writeFile(filePath, `---\ntitle: Valid Title\n---`); - const mockReadFile = jest.spyOn(fs, 'readFile').mockRejectedValue(new Error('Test readFile error')); + const mockReadFile = jest.spyOn(fs, 'readFile') + .mockImplementation(() => Promise.reject(new Error('ECONNRESET: Connection reset by peer'))); try { await checkMarkdownFiles(tempDir, validateBlogs); } catch (err) { expect(mockConsoleError).toHaveBeenCalledWith( expect.stringContaining(`Error processing file invalid.md:`), expect.any(Error) ); - expect(err.message).toBe('Test readFile error'); + expect(err.message).toBe('ECONNRESET: Connection reset by peer'); + expect(err).toHaveProperty('code', 'ECONNRESET'); } mockReadFile.mockRestore(); });🧰 Tools
🪛 eslint
[error] 98-98: Delete
··(prettier/prettier)
[error] 99-99: Replace
········with····(prettier/prettier)
[error] 100-100: Delete
····(prettier/prettier)
[error] 102-102: Replace
········with····(prettier/prettier)
[error] 104-104: Delete
····(prettier/prettier)
[error] 105-105: Delete
······(prettier/prettier)
[error] 106-106: Delete
····(prettier/prettier)
[error] 107-107: Replace
············with······(prettier/prettier)
[error] 108-108: Delete
········(prettier/prettier)
[error] 109-109: Replace
················with········(prettier/prettier)
[error] 110-110: Delete
······(prettier/prettier)
[error] 111-111: Delete
······(prettier/prettier)
[error] 112-112: Replace
········with····(prettier/prettier)
[error] 114-114: Delete
····(prettier/prettier)
[error] 115-115: Delete
··(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 4-9: Replace ⏎····main,⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·main,·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 15-15: Replace ···· with ··
(prettier/prettier)
[error] 16-16: Delete ····
(prettier/prettier)
[error] 17-17: Replace ········ with ····
(prettier/prettier)
[error] 18-18: Delete ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Delete ····
(prettier/prettier)
[error] 23-23: Replace ···· with ··
(prettier/prettier)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Delete ······
(prettier/prettier)
[error] 33-33: Delete ····
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ····expect(errors).toEqual( with expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 37-37: Replace ············ with ········
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
[error] 43-43: Delete ··
(prettier/prettier)
[error] 44-44: Replace ········ with ····
(prettier/prettier)
[error] 45-45: Delete ····
(prettier/prettier)
[error] 46-48: Replace ········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with ····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 49-49: Delete ····])
(prettier/prettier)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 52-52: Delete ··
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 54-54: Replace ········ with ····
(prettier/prettier)
[error] 56-56: Delete ····
(prettier/prettier)
[error] 58-58: Delete ····
(prettier/prettier)
[error] 59-59: Replace ········ with ····
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Delete ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Replace ············ with ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Replace ········ with ····
(prettier/prettier)
[error] 73-73: Replace ········ with ····
(prettier/prettier)
[error] 74-74: Delete ··
(prettier/prettier)
[error] 76-76: Delete ··
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 79-80: Replace ········await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))⏎············ with ····await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))
(prettier/prettier)
[error] 82-82: Delete ····
(prettier/prettier)
[error] 83-83: Delete ··
(prettier/prettier)
[error] 85-85: Delete ··
(prettier/prettier)
[error] 86-86: Delete ····
(prettier/prettier)
[error] 87-87: Replace ········ with ····
(prettier/prettier)
[error] 88-88: Delete ····
(prettier/prettier)
[error] 90-90: Replace ········ with ····
(prettier/prettier)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Replace ····expect(mockConsoleLog).not.toHaveBeenCalledWith(expect.stringContaining('Errors·in·file·reference/specification/skipped.md')); with expect(mockConsoleLog).not.toHaveBeenCalledWith(⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 95-95: Insert );⏎
(prettier/prettier)
[error] 96-96: Delete ··
(prettier/prettier)
[error] 98-98: Delete ··
(prettier/prettier)
[error] 99-99: Replace ········ with ····
(prettier/prettier)
[error] 100-100: Delete ····
(prettier/prettier)
[error] 102-102: Replace ········ with ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
[error] 105-105: Delete ······
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 107-107: Replace ············ with ······
(prettier/prettier)
[error] 108-108: Delete ········
(prettier/prettier)
[error] 109-109: Replace ················ with ········
(prettier/prettier)
[error] 110-110: Delete ······
(prettier/prettier)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 112-112: Replace ········ with ····
(prettier/prettier)
[error] 114-114: Delete ····
(prettier/prettier)
[error] 115-115: Delete ··
(prettier/prettier)
[error] 117-117: Delete ··
(prettier/prettier)
[error] 118-118: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 119-119: Replace ········ with ····
(prettier/prettier)
[error] 121-121: Replace ········ with ····
(prettier/prettier)
[error] 123-126: Replace ········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with ····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 127-127: Delete ····
(prettier/prettier)
[error] 129-129: Delete ····
(prettier/prettier)
[error] 130-130: Delete ··
(prettier/prettier)
🔇 Additional comments (2)
tests/markdown/check-markdown.test.js (2)
25-41: LGTM! Well-structured test for author validation.
The test thoroughly validates the authors array with multiple scenarios and specific error messages.
🧰 Tools
🪛 eslint
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Delete ····
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Delete ······
(prettier/prettier)
[error] 33-33: Delete ····
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ····expect(errors).toEqual( with expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 37-37: Replace ············ with ········
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ············ with ········
(prettier/prettier)
[error] 40-40: Replace ··]) with ])⏎····
(prettier/prettier)
[error] 41-41: Delete ··
(prettier/prettier)
62-74: 🛠️ Refactor suggestion
Improve test assertion specificity.
The current test only checks the number of errors without validating their content.
it('returns multiple validation errors for invalid blog frontmatter', async () => {
const frontmatter = {
title: 123,
date: 'invalid-date',
type: 'blog',
tags: 'not-an-array',
cover: ['not-a-string'],
authors: { name: 'John Doe' }
};
const errors = validateBlogs(frontmatter);
- expect(errors.length).toBeGreaterThan(3);
+ expect(errors).toEqual(expect.arrayContaining([
+ 'Title must be a string',
+ 'Date must be a valid date string',
+ 'Tags must be an array',
+ 'Cover must be a string',
+ 'Authors must be an array'
+ ]));
});Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 62-62: Delete ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Delete ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Replace ············ with ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Replace ········ with ····
(prettier/prettier)
[error] 73-73: Replace ········ with ····
(prettier/prettier)
[error] 74-74: Delete ··
(prettier/prettier)
| it('should handle main function errors', async () => { | ||
| const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => { }); | ||
| jest.spyOn(fs, 'readdir').mockRejectedValue(new Error('Test error')); | ||
|
|
||
| await main(); | ||
|
|
||
| expect(mockConsoleError).toHaveBeenCalledWith( | ||
| 'Failed to validate markdown files:', | ||
| expect.any(Error) | ||
| ); | ||
| expect(mockExit).toHaveBeenCalledWith(1); | ||
|
|
||
| mockExit.mockRestore(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for successful main function execution.
The test suite only covers the error case for the main function.
+ it('should handle successful main function execution', async () => {
+ const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {});
+ await fs.mkdir(path.join(tempDir, 'content'), { recursive: true });
+ await fs.writeFile(
+ path.join(tempDir, 'content/valid.md'),
+ '---\ntitle: "Valid Post"\ndate: "2024-01-01"\ntype: "blog"\ntags: ["test"]\ncover: "cover.jpg"\nauthors: [{"name": "Test", "photo": "test.jpg"}]\n---'
+ );
+
+ process.env.INIT_CWD = tempDir;
+ await main();
+
+ expect(mockConsoleError).not.toHaveBeenCalled();
+ expect(mockExit).toHaveBeenCalledWith(0);
+
+ mockExit.mockRestore();
+ delete process.env.INIT_CWD;
+ });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 eslint
[error] 117-117: Delete ··
(prettier/prettier)
[error] 118-118: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 119-119: Replace ········ with ····
(prettier/prettier)
[error] 121-121: Replace ········ with ····
(prettier/prettier)
[error] 123-126: Replace ········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with ····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 127-127: Delete ····
(prettier/prettier)
[error] 129-129: Delete ····
(prettier/prettier)
[error] 130-130: Delete ··
(prettier/prettier)
There was a problem hiding this comment.
added test to check main function execution
There was a problem hiding this comment.
@vishvamsinh28 Great! Thank you for adding the test to check main function execution. This improves the test coverage and ensures proper validation.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@vishvamsinh28 CodeRabbitAI has provided code suggestions. Kindly implement them and also pick the nitpick comments as well, if they are good to implement |
vishvamsinh28
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/markdown/check-markdown.test.js (2)
20-23: Add error handling to afterEach cleanup.The cleanup phase should handle potential errors to prevent test failures due to cleanup issues.
afterEach(async () => { mockConsoleError.mockRestore(); - await fs.rm(tempDir, { recursive: true, force: true }); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Cleanup failed: ${error.message}`); + } });🧰 Tools
🪛 eslint
[error] 20-20: Delete
··(prettier/prettier)
[error] 21-21: Replace
········with····(prettier/prettier)
[error] 22-22: Replace
········with····(prettier/prettier)
[error] 23-23: Delete
··(prettier/prettier)
73-77: Use consistent string quotes.The test uses a mix of single and double quotes for string literals. Maintain consistency by using single quotes throughout.
expect(errors).toEqual([ - "Invalid date format: invalid-date", - "Tags should be an array", - "Cover must be a string", - "Authors should be an array"]); + 'Invalid date format: invalid-date', + 'Tags should be an array', + 'Cover must be a string', + 'Authors should be an array' +]);🧰 Tools
🪛 eslint
[error] 73-73: Delete
····(prettier/prettier)
[error] 74-74: Replace
············"Invalid·date·format:·invalid-date"with······'Invalid·date·format:·invalid-date'(prettier/prettier)
[error] 75-75: Replace
············"Tags·should·be·an·array"with······'Tags·should·be·an·array'(prettier/prettier)
[error] 76-76: Replace
······"Cover·must·be·a·string"with'Cover·must·be·a·string'(prettier/prettier)
[error] 77-77: Replace
············"Authors·should·be·an·array"with······'Authors·should·be·an·array'⏎····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
🪛 eslint
tests/markdown/check-markdown.test.js
[error] 4-9: Replace ⏎····main,⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·main,·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 15-15: Delete ··
(prettier/prettier)
[error] 16-16: Replace ········ with ····
(prettier/prettier)
[error] 17-17: Replace ········ with ····
(prettier/prettier)
[error] 18-18: Replace ···· with ··
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 21-21: Replace ········ with ····
(prettier/prettier)
[error] 22-22: Replace ········ with ····
(prettier/prettier)
[error] 23-23: Delete ··
(prettier/prettier)
[error] 25-25: Delete ··
(prettier/prettier)
[error] 26-26: Replace ········ with ····
(prettier/prettier)
[error] 27-27: Replace ············ with ······
(prettier/prettier)
[error] 28-28: Replace ············ with ······
(prettier/prettier)
[error] 29-29: Replace ············ with ······
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Delete ······
(prettier/prettier)
[error] 33-33: Delete ····
(prettier/prettier)
[error] 35-35: Delete ····
(prettier/prettier)
[error] 36-36: Replace ········expect(errors).toEqual( with ····expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 37-37: Delete ····
(prettier/prettier)
[error] 38-38: Replace ············ with ········
(prettier/prettier)
[error] 39-39: Delete ····
(prettier/prettier)
[error] 40-40: Replace ········])); with ······])
(prettier/prettier)
[error] 41-41: Insert );⏎··
(prettier/prettier)
[error] 43-43: Replace ···· with ··
(prettier/prettier)
[error] 44-44: Delete ····
(prettier/prettier)
[error] 45-45: Replace ········ with ····
(prettier/prettier)
[error] 46-48: Replace ····expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 49-49: Replace ········]) with ····
(prettier/prettier)
[error] 50-50: Delete ··
(prettier/prettier)
[error] 52-52: Replace ···· with ··
(prettier/prettier)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 54-54: Replace ········ with ····
(prettier/prettier)
[error] 56-56: Delete ····
(prettier/prettier)
[error] 58-58: Replace ········ with ····
(prettier/prettier)
[error] 59-59: Delete ····
(prettier/prettier)
[error] 60-60: Delete ··
(prettier/prettier)
[error] 62-62: Replace ···· with ··
(prettier/prettier)
[error] 63-63: Delete ····
(prettier/prettier)
[error] 64-64: Delete ······
(prettier/prettier)
[error] 65-65: Replace ············ with ······
(prettier/prettier)
[error] 66-66: Delete ······
(prettier/prettier)
[error] 67-67: Replace ············ with ······
(prettier/prettier)
[error] 68-68: Replace ············ with ······
(prettier/prettier)
[error] 69-69: Replace ············ with ······
(prettier/prettier)
[error] 70-70: Delete ····
(prettier/prettier)
[error] 71-71: Replace ········ with ····
(prettier/prettier)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Replace ············"Invalid·date·format:·invalid-date" with ······'Invalid·date·format:·invalid-date'
(prettier/prettier)
[error] 75-75: Replace ············"Tags·should·be·an·array" with ······'Tags·should·be·an·array'
(prettier/prettier)
[error] 76-76: Replace ······"Cover·must·be·a·string" with 'Cover·must·be·a·string'
(prettier/prettier)
[error] 77-77: Replace ············"Authors·should·be·an·array" with ······'Authors·should·be·an·array'⏎····
(prettier/prettier)
[error] 78-78: Replace ···· with ··
(prettier/prettier)
[error] 80-80: Delete ··
(prettier/prettier)
[error] 81-81: Replace ········ with ····
(prettier/prettier)
[error] 83-84: Replace ····await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))⏎············ with await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))
(prettier/prettier)
[error] 86-86: Delete ····
(prettier/prettier)
[error] 87-87: Replace ···· with ··
(prettier/prettier)
[error] 89-89: Delete ··
(prettier/prettier)
[error] 90-90: Replace ········ with ····
(prettier/prettier)
[error] 91-91: Replace ········ with ····
(prettier/prettier)
[error] 92-92: Delete ····
(prettier/prettier)
[error] 94-94: Replace ········ with ····
(prettier/prettier)
[error] 96-96: Delete ····
(prettier/prettier)
[error] 98-98: Replace ········expect(mockConsoleLog).not.toHaveBeenCalledWith(expect.stringContaining('Errors·in·file·reference/specification/skipped.md')); with ····expect(mockConsoleLog).not.toHaveBeenCalledWith(⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 99-99: Insert );⏎
(prettier/prettier)
[error] 100-100: Replace ···· with ··
(prettier/prettier)
[error] 102-102: Delete ··
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 108-108: Delete ····
(prettier/prettier)
[error] 109-109: Replace ············ with ······
(prettier/prettier)
[error] 110-110: Delete ····
(prettier/prettier)
[error] 111-111: Replace ············ with ······
(prettier/prettier)
[error] 112-112: Replace ················ with ········
(prettier/prettier)
[error] 113-113: Delete ········
(prettier/prettier)
[error] 114-114: Replace ············ with ······
(prettier/prettier)
[error] 115-115: Delete ······
(prettier/prettier)
[error] 116-116: Delete ····
(prettier/prettier)
[error] 118-118: Delete ····
(prettier/prettier)
[error] 119-119: Replace ···· with ··
(prettier/prettier)
[error] 121-121: Delete ··
(prettier/prettier)
[error] 122-122: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 123-123: Delete ····
(prettier/prettier)
[error] 125-125: Delete ····
(prettier/prettier)
[error] 127-130: Replace ····expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 131-131: Delete ····
(prettier/prettier)
[error] 133-133: Delete ····
(prettier/prettier)
[error] 134-134: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
tests/markdown/check-markdown.test.js (1)
121-135: 🛠️ Refactor suggestion
Enhance main function error test coverage.
The test only covers the error case. Add test coverage for the success case and different types of validation errors.
+ it('should handle successful markdown validation', async () => {
+ const mockExit = jest.spyOn(process, 'exit').mockImplementation(() => {});
+ process.env.INIT_CWD = tempDir;
+
+ await fs.writeFile(
+ path.join(tempDir, 'valid.md'),
+ '---\ntitle: "Valid Post"\ndate: "2024-01-01"\ntype: "blog"\ntags: ["test"]\ncover: "cover.jpg"\nauthors: [{"name": "Test", "photo": "test.jpg"}]\n---'
+ );
+
+ await main();
+
+ expect(mockConsoleError).not.toHaveBeenCalled();
+ expect(mockExit).toHaveBeenCalledWith(0);
+
+ mockExit.mockRestore();
+ delete process.env.INIT_CWD;
+ });Likely invalid or redundant comment.
🧰 Tools
🪛 eslint
[error] 121-121: Delete ··
(prettier/prettier)
[error] 122-122: Replace ········const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{· with ····const·mockExit·=·jest.spyOn(process,·'exit').mockImplementation(()·=>·{
(prettier/prettier)
[error] 123-123: Delete ····
(prettier/prettier)
[error] 125-125: Delete ····
(prettier/prettier)
[error] 127-130: Replace ····expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 131-131: Delete ····
(prettier/prettier)
[error] 133-133: Delete ····
(prettier/prettier)
[error] 134-134: Delete ··
(prettier/prettier)
| it('logs and rethrows error when an exception occurs while processing a file', async () => { | ||
| const filePath = path.join(tempDir, 'invalid.md'); | ||
| await fs.writeFile(filePath, `---\ntitle: Valid Title\n---`); | ||
|
|
||
| const mockReadFile = jest.spyOn(fs, 'readFile').mockRejectedValue(new Error('Test readFile error')); | ||
|
|
||
| try { | ||
| await checkMarkdownFiles(tempDir, validateBlogs); | ||
| } catch (err) { | ||
| expect(mockConsoleError).toHaveBeenCalledWith( | ||
| expect.stringContaining(`Error processing file invalid.md:`), | ||
| expect.any(Error) | ||
| ); | ||
| expect(err.message).toBe('Test readFile error'); | ||
| } | ||
|
|
||
| mockReadFile.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor try-catch to use Jest's built-in assertion.
Replace the try-catch block with Jest's expect().rejects pattern for cleaner and more idiomatic test code.
- it('logs and rethrows error when an exception occurs while processing a file', async () => {
+ it('logs and rejects when an exception occurs while processing a file', async () => {
const filePath = path.join(tempDir, 'invalid.md');
await fs.writeFile(filePath, `---\ntitle: Valid Title\n---`);
const mockReadFile = jest.spyOn(fs, 'readFile').mockRejectedValue(new Error('Test readFile error'));
- try {
- await checkMarkdownFiles(tempDir, validateBlogs);
- } catch (err) {
- expect(mockConsoleError).toHaveBeenCalledWith(
- expect.stringContaining(`Error processing file invalid.md:`),
- expect.any(Error)
- );
- expect(err.message).toBe('Test readFile error');
- }
+ await expect(checkMarkdownFiles(tempDir, validateBlogs)).rejects.toThrow('Test readFile error');
+ expect(mockConsoleError).toHaveBeenCalledWith(
+ expect.stringContaining(`Error processing file invalid.md:`),
+ expect.any(Error)
+ );
mockReadFile.mockRestore();
});📝 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('logs and rethrows error when an exception occurs while processing a file', async () => { | |
| const filePath = path.join(tempDir, 'invalid.md'); | |
| await fs.writeFile(filePath, `---\ntitle: Valid Title\n---`); | |
| const mockReadFile = jest.spyOn(fs, 'readFile').mockRejectedValue(new Error('Test readFile error')); | |
| try { | |
| await checkMarkdownFiles(tempDir, validateBlogs); | |
| } catch (err) { | |
| expect(mockConsoleError).toHaveBeenCalledWith( | |
| expect.stringContaining(`Error processing file invalid.md:`), | |
| expect.any(Error) | |
| ); | |
| expect(err.message).toBe('Test readFile error'); | |
| } | |
| mockReadFile.mockRestore(); | |
| }); | |
| it('logs and rejects when an exception occurs while processing a file', async () => { | |
| const filePath = path.join(tempDir, 'invalid.md'); | |
| await fs.writeFile(filePath, `---\ntitle: Valid Title\n---`); | |
| const mockReadFile = jest.spyOn(fs, 'readFile').mockRejectedValue(new Error('Test readFile error')); | |
| await expect(checkMarkdownFiles(tempDir, validateBlogs)).rejects.toThrow('Test readFile error'); | |
| expect(mockConsoleError).toHaveBeenCalledWith( | |
| expect.stringContaining(`Error processing file invalid.md:`), | |
| expect.any(Error) | |
| ); | |
| mockReadFile.mockRestore(); | |
| }); |
🧰 Tools
🪛 eslint
[error] 102-102: Delete ··
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 106-106: Delete ····
(prettier/prettier)
[error] 108-108: Delete ····
(prettier/prettier)
[error] 109-109: Replace ············ with ······
(prettier/prettier)
[error] 110-110: Delete ····
(prettier/prettier)
[error] 111-111: Replace ············ with ······
(prettier/prettier)
[error] 112-112: Replace ················ with ········
(prettier/prettier)
[error] 113-113: Delete ········
(prettier/prettier)
[error] 114-114: Replace ············ with ······
(prettier/prettier)
[error] 115-115: Delete ······
(prettier/prettier)
[error] 116-116: Delete ····
(prettier/prettier)
[error] 118-118: Delete ····
(prettier/prettier)
[error] 119-119: Replace ···· with ··
(prettier/prettier)
There was a problem hiding this comment.
@vishvamsinh28 Great! Glad to hear that you've applied the suggestion.
(^_^)/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@akshatnema updated the tests |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
tests/markdown/check-markdown.test.js (5)
19-19: Consider using a more descriptive temporary directory name.The current name 'test-config' doesn't clearly indicate the purpose of these tests.
-tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test-config')); +tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'markdown-validator-tests-'));🧰 Tools
🪛 eslint
[error] 19-19: Replace
········with····(prettier/prettier)
22-26: Add error handling in afterEach cleanup.The cleanup operation should handle potential errors to prevent test failures due to cleanup issues.
afterEach(async () => { mockConsoleError.mockRestore(); mockProcessExit.mockRestore(); - await fs.rm(tempDir, { recursive: true, force: true }); + try { + await fs.rm(tempDir, { recursive: true, force: true }); + } catch (error) { + console.warn(`Failed to clean up temporary directory: ${error.message}`); + } });🧰 Tools
🪛 eslint
[error] 22-22: Delete
··(prettier/prettier)
[error] 23-23: Delete
····(prettier/prettier)
[error] 24-24: Replace
········with····(prettier/prettier)
[error] 25-25: Delete
····(prettier/prettier)
[error] 26-26: Delete
··(prettier/prettier)
77-80: Use consistent string quotes.The test uses double quotes for error messages while single quotes are used elsewhere in the file.
- "Invalid date format: invalid-date", - "Tags should be an array", - "Cover must be a string", - "Authors should be an array"]); + 'Invalid date format: invalid-date', + 'Tags should be an array', + 'Cover must be a string', + 'Authors should be an array' + ]);🧰 Tools
🪛 eslint
[error] 77-77: Replace
············"Invalid·date·format:·invalid-date"with······'Invalid·date·format:·invalid-date'(prettier/prettier)
[error] 78-78: Replace
······"Tags·should·be·an·array"with'Tags·should·be·an·array'(prettier/prettier)
[error] 79-79: Replace
······"Cover·must·be·a·string"with'Cover·must·be·a·string'(prettier/prettier)
[error] 80-80: Replace
······"Authors·should·be·an·array"with'Authors·should·be·an·array'⏎····(prettier/prettier)
28-44: Add test case for empty authors array.The current tests don't verify behavior when authors array is empty, which is a valid edge case.
+ it('validates empty authors array', async () => { + const frontmatter = { + title: 'Test Blog', + date: '2024-01-01', + type: 'blog', + tags: ['test'], + cover: 'cover.jpg', + authors: [] + }; + + const errors = validateBlogs(frontmatter); + expect(errors).toEqual(['Authors array cannot be empty']); + }); +🧰 Tools
🪛 eslint
[error] 28-28: Delete
··(prettier/prettier)
[error] 29-29: Delete
····(prettier/prettier)
[error] 30-30: Replace
············with······(prettier/prettier)
[error] 31-31: Replace
············with······(prettier/prettier)
[error] 32-32: Replace
············with······(prettier/prettier)
[error] 33-33: Delete
······(prettier/prettier)
[error] 34-34: Replace
············with······(prettier/prettier)
[error] 35-35: Replace
············with······(prettier/prettier)
[error] 36-36: Delete
····(prettier/prettier)
[error] 38-38: Delete
····(prettier/prettier)
[error] 39-39: Replace
····expect(errors).toEqual(withexpect(errors).toEqual(⏎······(prettier/prettier)
[error] 40-40: Replace
············with········(prettier/prettier)
[error] 41-41: Delete
····(prettier/prettier)
[error] 42-42: Replace
············with········(prettier/prettier)
[error] 43-43: Replace
··]));with])(prettier/prettier)
[error] 44-44: Delete
}(prettier/prettier)
46-53: Add test for validateDocs with missing required fields.The current test only checks for invalid types but not for missing required fields.
+ it('validates docs frontmatter for missing required fields', async () => { + const frontmatter = {}; + const errors = validateDocs(frontmatter); + expect(errors).toEqual(expect.arrayContaining([ + 'Title is missing or not a string', + 'Weight is missing or not a number' + ])); + }); +🧰 Tools
🪛 eslint
[error] 47-47: Replace
········with····(prettier/prettier)
[error] 48-48: Delete
····(prettier/prettier)
[error] 49-51: Replace
········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number'with····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])(prettier/prettier)
[error] 52-52: Replace
········])with····(prettier/prettier)
[error] 53-53: Replace
····with··(prettier/prettier)
scripts/markdown/check-markdown.js (3)
Line range hint
101-140: Add error handling for invalid frontmatter parsingThe
matter()function call could throw an error if the frontmatter is malformed, but this isn't handled in the try-catch block.const fileContent = await fs.readFile(filePath, 'utf-8'); -const { data: frontmatter } = matter(fileContent); +let frontmatter; +try { + ({ data: frontmatter } = matter(fileContent)); +} catch (err) { + console.error(`Invalid frontmatter in file ${relativeFilePath}:`, err); + process.exitCode = 1; + return; +}🧰 Tools
🪛 eslint
[error] 110-110: Delete
········(prettier/prettier)
[error] 111-111: Delete
······(prettier/prettier)
[error] 113-113: Delete
······(prettier/prettier)
[error] 114-114: Delete
········(prettier/prettier)
[error] 116-116: Delete
········(prettier/prettier)
[error] 117-117: Delete
········(prettier/prettier)
[error] 118-118: Delete
··········(prettier/prettier)
[error] 119-119: Replace
················with········(prettier/prettier)
[error] 120-120: Delete
··········(prettier/prettier)
[error] 121-121: Replace
····················with··········(prettier/prettier)
[error] 123-123: Replace
····················with··········(prettier/prettier)
Line range hint
104-136: Consider chunking file processing for large directoriesUsing
Promise.allwithmapcould lead to memory issues with large directories as it processes all files simultaneously.Consider processing files in chunks or implementing a queue system for better memory management.
🧰 Tools
🪛 eslint
[error] 110-110: Delete
········(prettier/prettier)
[error] 111-111: Delete
······(prettier/prettier)
[error] 113-113: Delete
······(prettier/prettier)
[error] 114-114: Delete
········(prettier/prettier)
[error] 116-116: Delete
········(prettier/prettier)
[error] 117-117: Delete
········(prettier/prettier)
[error] 118-118: Delete
··········(prettier/prettier)
[error] 119-119: Replace
················with········(prettier/prettier)
[error] 120-120: Delete
··········(prettier/prettier)
[error] 121-121: Replace
····················with··········(prettier/prettier)
[error] 123-123: Replace
····················with··········(prettier/prettier)
160-160: Add missing semicolonAdd a semicolon at the end of the exports statement to maintain consistency.
-module.exports = { validateBlogs, validateDocs, checkMarkdownFiles, main } +module.exports = { validateBlogs, validateDocs, checkMarkdownFiles, main };🧰 Tools
🪛 eslint
[error] 160-160: Insert
;(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
scripts/markdown/check-markdown.js(4 hunks)tests/markdown/check-markdown.test.js(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (3)
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
🪛 eslint
scripts/markdown/check-markdown.js
[error] 102-102: Replace ···· with ··
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: Delete ········
(prettier/prettier)
[error] 116-116: Delete ········
(prettier/prettier)
[error] 117-117: Delete ········
(prettier/prettier)
[error] 118-118: Delete ··········
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 120-120: Delete ··········
(prettier/prettier)
[error] 130-130: Replace ············ with ······
(prettier/prettier)
[error] 131-131: Delete ········
(prettier/prettier)
[error] 132-132: Delete ········
(prettier/prettier)
[error] 133-133: Replace ············ with ······
(prettier/prettier)
[error] 134-134: Replace ········ with ····
(prettier/prettier)
[error] 136-136: Delete ····
(prettier/prettier)
[error] 137-137: Delete ··
(prettier/prettier)
[error] 138-138: Delete ····
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 140-140: Delete ··
(prettier/prettier)
[error] 146-146: 'docsFolderPath' is already declared in the upper scope on line 143 column 7.
(no-shadow)
[error] 146-146: 'blogsFolderPath' is already declared in the upper scope on line 144 column 7.
(no-shadow)
[error] 147-147: Delete ··
(prettier/prettier)
[error] 148-148: Delete ····
(prettier/prettier)
[error] 149-149: Delete ······
(prettier/prettier)
[error] 150-150: Replace ············ with ······
(prettier/prettier)
[error] 151-151: Replace ········ with ····
(prettier/prettier)
[error] 152-152: Delete ··
(prettier/prettier)
[error] 153-153: Delete ····
(prettier/prettier)
[error] 154-154: Replace ········ with ····
(prettier/prettier)
[error] 155-155: Delete ··
(prettier/prettier)
[error] 158-158: Insert ·
(prettier/prettier)
[error] 160-160: Insert ;
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 4-9: Replace ⏎····main,⏎····validateBlogs,⏎····validateDocs,⏎····checkMarkdownFiles⏎ with ·main,·validateBlogs,·validateDocs,·checkMarkdownFiles·
(prettier/prettier)
[error] 12-12: Delete ··
(prettier/prettier)
[error] 13-13: Delete ··
(prettier/prettier)
[error] 14-14: Delete ··
(prettier/prettier)
[error] 16-16: Delete ··
(prettier/prettier)
[error] 17-17: Delete ····
(prettier/prettier)
[error] 18-18: Replace ········ with ····
(prettier/prettier)
[error] 19-19: Replace ········ with ····
(prettier/prettier)
[error] 20-20: Delete ··
(prettier/prettier)
[error] 22-22: Delete ··
(prettier/prettier)
[error] 23-23: Delete ····
(prettier/prettier)
[error] 24-24: Replace ········ with ····
(prettier/prettier)
[error] 25-25: Delete ····
(prettier/prettier)
[error] 26-26: Delete ··
(prettier/prettier)
[error] 28-28: Delete ··
(prettier/prettier)
[error] 29-29: Delete ····
(prettier/prettier)
[error] 30-30: Replace ············ with ······
(prettier/prettier)
[error] 31-31: Replace ············ with ······
(prettier/prettier)
[error] 32-32: Replace ············ with ······
(prettier/prettier)
[error] 33-33: Delete ······
(prettier/prettier)
[error] 34-34: Replace ············ with ······
(prettier/prettier)
[error] 35-35: Replace ············ with ······
(prettier/prettier)
[error] 36-36: Delete ····
(prettier/prettier)
[error] 38-38: Delete ····
(prettier/prettier)
[error] 39-39: Replace ····expect(errors).toEqual( with expect(errors).toEqual(⏎······
(prettier/prettier)
[error] 40-40: Replace ············ with ········
(prettier/prettier)
[error] 41-41: Delete ····
(prettier/prettier)
[error] 42-42: Replace ············ with ········
(prettier/prettier)
[error] 43-43: Replace ··])); with ])
(prettier/prettier)
[error] 44-44: Delete }
(prettier/prettier)
[error] 45-46: Replace ⏎·· with ··});⏎⏎
(prettier/prettier)
[error] 47-47: Replace ········ with ····
(prettier/prettier)
[error] 48-48: Delete ····
(prettier/prettier)
[error] 49-51: Replace ········expect(errors).toEqual(expect.arrayContaining([⏎············'Title·is·missing·or·not·a·string',⏎············'Weight·is·missing·or·not·a·number' with ····expect(errors).toEqual(⏎······expect.arrayContaining(['Title·is·missing·or·not·a·string',·'Weight·is·missing·or·not·a·number'])
(prettier/prettier)
[error] 52-52: Replace ········]) with ····
(prettier/prettier)
[error] 53-53: Replace ···· with ··
(prettier/prettier)
[error] 55-55: Delete ··
(prettier/prettier)
[error] 56-56: Replace ········ with ····
(prettier/prettier)
[error] 57-57: Delete ····
(prettier/prettier)
[error] 59-59: Replace ········ with ····
(prettier/prettier)
[error] 61-61: Delete ····
(prettier/prettier)
[error] 62-62: Delete ····
(prettier/prettier)
[error] 63-63: Delete ··
(prettier/prettier)
[error] 65-65: Delete ··
(prettier/prettier)
[error] 66-66: Delete ····
(prettier/prettier)
[error] 67-67: Delete ······
(prettier/prettier)
[error] 68-68: Delete ······
(prettier/prettier)
[error] 69-69: Delete ······
(prettier/prettier)
[error] 70-70: Replace ············ with ······
(prettier/prettier)
[error] 71-71: Replace ············ with ······
(prettier/prettier)
[error] 72-72: Replace ············ with ······
(prettier/prettier)
[error] 73-73: Delete ····
(prettier/prettier)
[error] 74-74: Replace ········ with ····
(prettier/prettier)
[error] 76-76: Delete ····
(prettier/prettier)
[error] 77-77: Replace ············"Invalid·date·format:·invalid-date" with ······'Invalid·date·format:·invalid-date'
(prettier/prettier)
[error] 78-78: Replace ······"Tags·should·be·an·array" with 'Tags·should·be·an·array'
(prettier/prettier)
[error] 79-79: Replace ······"Cover·must·be·a·string" with 'Cover·must·be·a·string'
(prettier/prettier)
[error] 80-80: Replace ······"Authors·should·be·an·array" with 'Authors·should·be·an·array'⏎····
(prettier/prettier)
[error] 81-81: Replace ···· with ··
(prettier/prettier)
[error] 83-83: Delete ··
(prettier/prettier)
[error] 84-84: Replace ········ with ····
(prettier/prettier)
[error] 86-87: Replace ····await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))⏎············ with await·expect(checkMarkdownFiles(invalidFolderPath,·validateBlogs))
(prettier/prettier)
[error] 89-89: Replace ········ with ····
(prettier/prettier)
[error] 90-90: Delete ··
(prettier/prettier)
[error] 92-92: Replace ···· with ··
(prettier/prettier)
[error] 93-93: Replace ········ with ····
(prettier/prettier)
[error] 94-94: Delete ····
(prettier/prettier)
[error] 95-95: Replace ········ with ····
(prettier/prettier)
[error] 97-97: Delete ····
(prettier/prettier)
[error] 99-99: Replace ········ with ····
(prettier/prettier)
[error] 101-101: Replace ····expect(mockConsoleLog).not.toHaveBeenCalledWith(expect.stringContaining('Errors·in·file·reference/specification/skipped.md')); with expect(mockConsoleLog).not.toHaveBeenCalledWith(⏎······expect.stringContaining('Errors·in·file·reference/specification/skipped.md')
(prettier/prettier)
[error] 102-102: Insert );⏎
(prettier/prettier)
[error] 103-103: Delete ··
(prettier/prettier)
[error] 105-105: Delete ··
(prettier/prettier)
[error] 106-106: Replace ········ with ····
(prettier/prettier)
[error] 107-107: Delete ····
(prettier/prettier)
[error] 109-109: Replace ········ with ····
(prettier/prettier)
[error] 111-111: Delete ····
(prettier/prettier)
[error] 112-112: Replace ········ with ····
(prettier/prettier)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: Delete ······
(prettier/prettier)
[error] 115-115: Replace ········ with ····
(prettier/prettier)
[error] 117-117: Delete ····
(prettier/prettier)
[error] 118-118: Delete ··
(prettier/prettier)
[error] 120-120: Delete ··
(prettier/prettier)
[error] 121-121: Delete ····
(prettier/prettier)
[error] 123-123: Replace ········ with ····
(prettier/prettier)
[error] 125-125: Delete ····
(prettier/prettier)
[error] 127-130: Replace ········expect(mockConsoleError).toHaveBeenCalledWith(⏎············'Failed·to·validate·markdown·files:',⏎············expect.any(Error)⏎········ with ····expect(mockConsoleError).toHaveBeenCalledWith('Failed·to·validate·markdown·files:',·expect.any(Error)
(prettier/prettier)
[error] 131-131: Delete ··
(prettier/prettier)
[error] 133-133: Delete ··
(prettier/prettier)
[error] 134-135: Delete ⏎····
(prettier/prettier)
[error] 137-137: Replace ········expect(mockConsoleError).not.toHaveBeenCalledWith() with ····expect(mockConsoleError).not.toHaveBeenCalledWith();
(prettier/prettier)
[error] 138-139: Replace ····});⏎ with ··});
(prettier/prettier)
🔇 Additional comments (1)
scripts/markdown/check-markdown.js (1)
1-1: LGTM: Appropriate use of fs.promises
The change to use fs.promises is consistent with the async implementation pattern.
| it('should handle successful main function execution', async () => { | ||
|
|
||
| await main(); | ||
|
|
||
| expect(mockConsoleError).not.toHaveBeenCalledWith() | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage for successful main function execution.
The current test for successful execution is incomplete:
- No test data is created
- The assertion is incorrect (toHaveBeenCalledWith without arguments)
it('should handle successful main function execution', async () => {
+ await fs.mkdir(path.join(tempDir, 'content'), { recursive: true });
+ await fs.writeFile(
+ path.join(tempDir, 'content/valid.md'),
+ '---\ntitle: "Valid Post"\ndate: "2024-01-01"\ntype: "blog"\ntags: ["test"]\ncover: "cover.jpg"\nauthors: [{"name": "Test", "photo": "test.jpg"}]\n---'
+ );
+
+ process.env.INIT_CWD = tempDir;
await main();
- expect(mockConsoleError).not.toHaveBeenCalledWith()
+ expect(mockConsoleError).not.toHaveBeenCalled();
+ expect(mockProcessExit).toHaveBeenCalledWith(0);
+
+ delete process.env.INIT_CWD;
});📝 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 successful main function execution', async () => { | |
| await main(); | |
| expect(mockConsoleError).not.toHaveBeenCalledWith() | |
| }); | |
| it('should handle successful main function execution', async () => { | |
| await fs.mkdir(path.join(tempDir, 'content'), { recursive: true }); | |
| await fs.writeFile( | |
| path.join(tempDir, 'content/valid.md'), | |
| '---\ntitle: "Valid Post"\ndate: "2024-01-01"\ntype: "blog"\ntags: ["test"]\ncover: "cover.jpg"\nauthors: [{"name": "Test", "photo": "test.jpg"}]\n---' | |
| ); | |
| process.env.INIT_CWD = tempDir; | |
| await main(); | |
| expect(mockConsoleError).not.toHaveBeenCalled(); | |
| expect(mockProcessExit).toHaveBeenCalledWith(0); | |
| delete process.env.INIT_CWD; | |
| }); |
🧰 Tools
🪛 eslint
[error] 133-133: Delete ··
(prettier/prettier)
[error] 134-135: Delete ⏎····
(prettier/prettier)
[error] 137-137: Replace ········expect(mockConsoleError).not.toHaveBeenCalledWith() with ····expect(mockConsoleError).not.toHaveBeenCalledWith();
(prettier/prettier)
There was a problem hiding this comment.
not applying this one because we are essentially calling the checkMarkdownFiles function inside main, and we've already tested that in another test. So, in this case, we're just calling the main function to ensure it doesn't throw any errors. We have another test for main function errors.
There was a problem hiding this comment.
@vishvamsinh28, understood. Thank you for the clarification.
(_/)
(•_•)
/>💧
✏️ Learnings added
Learnt from: vishvamsinh28
PR: asyncapi/website#3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.384Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
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.
scripts/markdown/check-markdown.js
Outdated
| async function main(docsFolderPath, blogsFolderPath) { | ||
| try { | ||
| await Promise.all([ | ||
| checkMarkdownFiles(docsFolderPath, validateDocs), | ||
| checkMarkdownFiles(blogsFolderPath, validateBlogs) | ||
| ]); | ||
| } catch (error) { | ||
| console.error('Failed to validate markdown files:', error); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix parameter shadowing and add input validation
The function has two issues:
- Parameters shadow global variables defined on lines 143-144
- Missing validation for input parameters
-async function main(docsFolderPath, blogsFolderPath) {
+async function main(docsPath, blogsPath) {
+ if (!docsPath || !blogsPath) {
+ throw new Error('Both docsPath and blogsPath must be provided');
+ }
try {
await Promise.all([
- checkMarkdownFiles(docsFolderPath, validateDocs),
- checkMarkdownFiles(blogsFolderPath, validateBlogs)
+ checkMarkdownFiles(docsPath, validateDocs),
+ checkMarkdownFiles(blogsPath, validateBlogs)
]);
} catch (error) {📝 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.
| async function main(docsFolderPath, blogsFolderPath) { | |
| try { | |
| await Promise.all([ | |
| checkMarkdownFiles(docsFolderPath, validateDocs), | |
| checkMarkdownFiles(blogsFolderPath, validateBlogs) | |
| ]); | |
| } catch (error) { | |
| console.error('Failed to validate markdown files:', error); | |
| process.exit(1); | |
| } | |
| } | |
| async function main(docsPath, blogsPath) { | |
| if (!docsPath || !blogsPath) { | |
| throw new Error('Both docsPath and blogsPath must be provided'); | |
| } | |
| try { | |
| await Promise.all([ | |
| checkMarkdownFiles(docsPath, validateDocs), | |
| checkMarkdownFiles(blogsPath, validateBlogs) | |
| ]); | |
| } catch (error) { | |
| console.error('Failed to validate markdown files:', error); | |
| process.exit(1); | |
| } | |
| } |
🧰 Tools
🪛 eslint
[error] 146-146: 'docsFolderPath' is already declared in the upper scope on line 143 column 7.
(no-shadow)
[error] 146-146: 'blogsFolderPath' is already declared in the upper scope on line 144 column 7.
(no-shadow)
[error] 147-147: Delete ··
(prettier/prettier)
[error] 148-148: Delete ····
(prettier/prettier)
[error] 149-149: Delete ······
(prettier/prettier)
[error] 150-150: Replace ············ with ······
(prettier/prettier)
[error] 151-151: Replace ········ with ····
(prettier/prettier)
[error] 152-152: Delete ··
(prettier/prettier)
[error] 153-153: Delete ····
(prettier/prettier)
[error] 154-154: Replace ········ with ····
(prettier/prettier)
[error] 155-155: Delete ··
(prettier/prettier)
There was a problem hiding this comment.
I added parameters for testing and pushed them by mistake, but I removed them in the latest commit.
There was a problem hiding this comment.
@vishvamsinh28, thank you for addressing the issue. I'm glad to see it's resolved!
(^.^)/
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/markdown/check-markdown.js (2)
146-156: Add input validation for folder pathsWhile the error handling is good, consider adding validation for the folder paths to prevent potential issues.
async function main() { + if (!fs.access(docsFolderPath) || !fs.access(blogsFolderPath)) { + throw new Error('Invalid folder paths provided'); + } try { await Promise.all([ checkMarkdownFiles(docsFolderPath, validateDocs), checkMarkdownFiles(blogsFolderPath, validateBlogs) ]); } catch (error) { console.error('Failed to validate markdown files:', error); process.exit(1); } }🧰 Tools
🪛 eslint
[error] 147-147: Delete
··(prettier/prettier)
[error] 148-148: Replace
········with····(prettier/prettier)
[error] 149-149: Delete
······(prettier/prettier)
[error] 150-150: Replace
············with······(prettier/prettier)
[error] 151-151: Replace
········with····(prettier/prettier)
[error] 152-152: Delete
··(prettier/prettier)
[error] 153-153: Delete
····(prettier/prettier)
[error] 154-154: Delete
····(prettier/prettier)
[error] 155-155: Delete
··(prettier/prettier)
158-160: Consider conditional execution of main functionThe immediate execution of the main function might interfere with testing. Consider executing only when the file is run directly.
-main(); +if (require.main === module) { + main(); +} module.exports = { validateBlogs, validateDocs, checkMarkdownFiles, main };🧰 Tools
🪛 eslint
[error] 160-160: Insert
;(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/markdown/check-markdown.js(4 hunks)
🧰 Additional context used
📓 Learnings (1)
scripts/markdown/check-markdown.js (4)
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:34:51.303Z
Learning: When reviewing `scripts/markdown/check-markdown.js`, optimizations should be addressed in separate issues and not included in the current pull request.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.851Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
Learnt from: anshgoyalevil
PR: asyncapi/website#3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-11-12T06:59:33.852Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
🪛 eslint
scripts/markdown/check-markdown.js
[error] 102-102: Replace ···· with ··
(prettier/prettier)
[error] 103-103: Delete ····
(prettier/prettier)
[error] 104-104: Replace ········ with ····
(prettier/prettier)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: Delete ········
(prettier/prettier)
[error] 116-116: Delete ········
(prettier/prettier)
[error] 117-117: Replace ················ with ········
(prettier/prettier)
[error] 118-118: Delete ··········
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 120-120: Delete ··········
(prettier/prettier)
[error] 130-130: Replace ············ with ······
(prettier/prettier)
[error] 131-131: Delete ········
(prettier/prettier)
[error] 132-132: Delete ········
(prettier/prettier)
[error] 133-133: Delete ······
(prettier/prettier)
[error] 134-134: Replace ········ with ····
(prettier/prettier)
[error] 136-136: Delete ····
(prettier/prettier)
[error] 137-137: Delete ··
(prettier/prettier)
[error] 138-138: Delete ····
(prettier/prettier)
[error] 139-139: Delete ····
(prettier/prettier)
[error] 140-140: Delete ··
(prettier/prettier)
[error] 147-147: Delete ··
(prettier/prettier)
[error] 148-148: Replace ········ with ····
(prettier/prettier)
[error] 149-149: Delete ······
(prettier/prettier)
[error] 150-150: Replace ············ with ······
(prettier/prettier)
[error] 151-151: Replace ········ with ····
(prettier/prettier)
[error] 152-152: Delete ··
(prettier/prettier)
[error] 153-153: Delete ····
(prettier/prettier)
[error] 154-154: Delete ····
(prettier/prettier)
[error] 155-155: Delete ··
(prettier/prettier)
[error] 160-160: Insert ;
(prettier/prettier)
🔇 Additional comments (2)
scripts/markdown/check-markdown.js (2)
1-1: LGTM: Appropriate use of fs.promises
The switch to fs.promises is consistent with the async/await pattern used throughout the file.
Line range hint 101-140: LGTM: Well-structured async implementation with proper error handling
The async implementation includes:
- Proper error handling with try-catch blocks
- Clear error messages with file paths
- Appropriate error propagation
Let's verify the error handling:
✅ Verification successful
Let me gather more information about the error handling patterns in the codebase to ensure consistency.
Error handling implementation is robust and consistent
The error handling implementation follows a consistent pattern throughout the file:
- Proper error logging with contextual information (file paths)
- Appropriate error propagation via throw statements
- Clear distinction between validation errors (console.log) and processing errors (console.error)
- Nested try-catch blocks handle errors at both file and directory levels
- Process exit code is set appropriately for validation failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if error handling is consistent across the codebase
rg -A 2 "console.error.*Error.*processing" .
Length of output: 280
Script:
#!/bin/bash
# Check for error handling patterns in the codebase
rg -A 2 "console\.(error|log).*Error" .
# Check for similar file processing functions
ast-grep --pattern 'async function $_(folderPath, $$$) {
try {
$$$
} catch (err) {
$$$
}
}'
Length of output: 4427
🧰 Tools
🪛 eslint
[error] 110-110: Delete ········
(prettier/prettier)
[error] 111-111: Delete ······
(prettier/prettier)
[error] 113-113: Delete ······
(prettier/prettier)
[error] 114-114: Delete ········
(prettier/prettier)
[error] 116-116: Delete ········
(prettier/prettier)
[error] 117-117: Replace ················ with ········
(prettier/prettier)
[error] 118-118: Delete ··········
(prettier/prettier)
[error] 119-119: Replace ················ with ········
(prettier/prettier)
[error] 120-120: Delete ··········
(prettier/prettier)
[error] 121-121: Replace ···················· with ··········
(prettier/prettier)
[error] 123-123: Replace ···················· with ··········
(prettier/prettier)
This PR adds tests for check-markdown.js script
Summary by CodeRabbit
New Features
mainfunction to orchestrate markdown validation.Bug Fixes
Tests
mainfunction.