Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add tests for check markdown script #3378

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
5a90f49
initial setup
vishvamsinh28 Nov 9, 2024
5373c9f
tests updated
vishvamsinh28 Nov 9, 2024
a30cef3
test updated again
vishvamsinh28 Nov 9, 2024
a9da240
fwqfwef
vishvamsinh28 Nov 9, 2024
1079101
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 10, 2024
86201ed
use async await
vishvamsinh28 Nov 10, 2024
7c44f4a
tests updated
vishvamsinh28 Nov 10, 2024
c98ecdd
wqegeg
vishvamsinh28 Nov 10, 2024
6e34205
test updated
vishvamsinh28 Nov 10, 2024
4445ce5
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 11, 2024
333e0b2
feg
vishvamsinh28 Nov 11, 2024
8849351
Fixed variable shadowing
vishvamsinh28 Nov 11, 2024
1fbac7c
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 13, 2024
fada5ab
added tests for continue
vishvamsinh28 Nov 13, 2024
68cd3ee
workflow re-run
vishvamsinh28 Nov 14, 2024
998af5a
workflow update
vishvamsinh28 Nov 14, 2024
f5c8964
ff
vishvamsinh28 Nov 14, 2024
d92b519
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 15, 2024
d0fe33a
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 18, 2024
216a511
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 22, 2024
8a5235b
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 23, 2024
a93b45d
suggetion applied
vishvamsinh28 Nov 23, 2024
2732f0f
suggetion applied
vishvamsinh28 Nov 23, 2024
4fcdea8
suggetion applied 2
vishvamsinh28 Nov 23, 2024
e1a16c1
test added for err
vishvamsinh28 Nov 23, 2024
7401e7f
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 28, 2024
0fbdc06
main function added
vishvamsinh28 Nov 28, 2024
02e221b
added test for main function
vishvamsinh28 Nov 28, 2024
b17b83e
tests updated
vishvamsinh28 Nov 28, 2024
35d2e84
jgk
vishvamsinh28 Nov 28, 2024
7921604
test updated
vishvamsinh28 Nov 28, 2024
9d75691
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 29, 2024
b36972d
Merge branch 'markdownTest' of https://github.com/vishvamsinh28/websi…
vishvamsinh28 Nov 29, 2024
dff6ccc
main function test added
vishvamsinh28 Nov 29, 2024
53c86fd
test updated
vishvamsinh28 Nov 29, 2024
92cc885
test updates
vishvamsinh28 Nov 29, 2024
871e305
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 29, 2024
c0afa97
script updated
vishvamsinh28 Nov 29, 2024
10c1ba5
add tests for main function
vishvamsinh28 Nov 29, 2024
2fa09e6
test updated
vishvamsinh28 Nov 29, 2024
aa0e241
status code upudated
vishvamsinh28 Nov 29, 2024
746dc88
test updated
vishvamsinh28 Nov 29, 2024
4558748
Merge branch 'markdownTest' of https://github.com/vishvamsinh28/websi…
vishvamsinh28 Nov 29, 2024
29cd78c
script updated
vishvamsinh28 Nov 29, 2024
e50392c
Merge branch 'master' into markdownTest
vishvamsinh28 Nov 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 26 additions & 31 deletions scripts/markdown/check-markdown.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const fs = require('fs');
const fs = require('fs').promises;
Copy link

@coderabbitai coderabbitai bot Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Switching to async file operations could impact performance

Based on the analysis of the codebase:

  • The script processes a large number of markdown files (150+ files ranging from 1KB to 116KB)
  • It recursively traverses directories and processes files in parallel using Promise.all
  • The script is used for validating frontmatter in both docs and blog folders
  • Each file requires multiple file operations (readdir, stat, readFile)

Given that:

  1. The script performs multiple file operations per file
  2. It processes a significant number of files simultaneously
  3. Previous learnings indicate synchronous operations were specifically chosen for performance reasons
  4. The parallel processing with Promise.all could lead to too many open file handles

The switch to async operations could potentially degrade performance and system resource usage. Consider keeping the synchronous file operations.

🔗 Analysis chain

Consider performance implications of async file operations

Previous experience with this codebase suggests that synchronous file operations (fs.readFileSync) might be more suitable for processing large markdown files. While the switch to fs.promises follows modern JavaScript practices, it could potentially impact performance when dealing with numerous files.

Let's check the typical file sizes and counts in the markdown directories:

Consider benchmarking both approaches (sync vs async) with your typical markdown file load to make an informed decision.


The previous results only showed files in node_modules. We need to check the actual documentation files that this script processes and understand the implementation pattern to make an informed decision about sync vs async operations.


The previous results show the implementation uses parallel processing with Promise.all on file operations, but we need to understand the actual directory structure and how the script is invoked to make a final decision about the performance implications.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check markdown file sizes and counts
echo "Number of markdown files:"
find . -name "*.md" | wc -l

echo "\nLargest markdown files:"
find . -name "*.md" -exec ls -lh {} \; | sort -rh | head -n 5

Length of output: 619


Script:

#!/bin/bash
# Let's check the actual markdown files in docs and blog directories, excluding node_modules
echo "Markdown files in docs and blog directories:"
fd -e md -E "node_modules" . docs blog

echo "\nSizes of markdown files in docs and blog:"
fd -e md -E "node_modules" . docs blog -x ls -lh {}

echo "\nChecking the implementation to understand the processing pattern:"
ast-grep --pattern 'function checkMarkdownFiles($$$) {
  $$$
}'

Length of output: 4036


Script:

#!/bin/bash
# Let's check the actual directory structure to understand where markdown files are located
echo "Directory structure:"
ls -la

echo "\nSearching for markdown files recursively from root:"
find . -name "*.md" -not -path "./node_modules/*" -exec ls -lh {} \;

echo "\nChecking how the script is invoked:"
ast-grep --pattern 'checkMarkdownFiles($$$)'

Length of output: 28230

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok 👍 , so how can we improve this script. Currently we believe that making this script asynchronous and checking multiple markdown files in parallel runs will improve the performance of script. Do you have any better alternative?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema, I understand your aim to improve the script's performance by making it asynchronous and processing multiple markdown files in parallel. Converting to async operations can indeed enhance performance, but without proper concurrency control, it might lead to resource exhaustion due to too many simultaneous file operations.

One way to mitigate this is by limiting the number of concurrent asynchronous operations. You can achieve this by using a concurrency control library like p-limit. Here's how you might modify the script:

const fs = require('fs').promises;
const matter = require('gray-matter');
const path = require('path');
const pLimit = require('p-limit');

const limit = pLimit(10); // Adjust the concurrency limit as needed

async function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') {
    try {
        const files = await fs.readdir(folderPath);
        const filePromises = files.map((file) => limit(async () => {
            const filePath = path.join(folderPath, file);
            const relativeFilePath = path.join(relativePath, file);

            // Skip the folder 'docs/reference/specification'
            if (relativeFilePath.includes('reference/specification')) {
                return;
            }

            try {
                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);

                    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;
    }
}

In this example:

  • We introduce p-limit to limit the number of concurrent async operations. Adjust the concurrency limit (10 in the example) based on performance testing.
  • Each file operation is wrapped with the concurrency limit to prevent resource overuse.
  • This approach allows you to benefit from asynchronous processing while maintaining system stability.

Alternatively, if the number of files is manageable, processing files sequentially using a for...of loop might suffice:

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')) {
                continue;
            }

            try {
                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);

                    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;
            }
        }
    } catch (err) {
        console.error(`Error in directory ${folderPath}:`, err);
        throw err;
    }
}

This sequential approach reduces complexity and resource usage at the cost of potentially longer execution time.

Suggestions:

  • Benchmark Both Approaches: Measure the performance of both the concurrent (with limited concurrency) and sequential implementations to see which offers the best balance between speed and resource usage for your specific case.
  • Adjust Concurrency Level: If using concurrency limiting, experiment with different limits to find the optimal setting.
  • Error Handling: Ensure robust error handling is in place to handle any unexpected issues during file processing.

Let me know if you'd like further assistance or have questions about implementing these changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the limit of resource will be internally managed by Nodejs, am I right?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema, you bring up a valid point. Node.js does manage asynchronous operations efficiently through its event loop, and it handles many aspects of resource management internally. However, when it comes to file system operations, especially with a large number of files, there are underlying operating system limits to consider—such as the maximum number of file descriptors that can be open simultaneously.

While Node.js abstracts much of the complexity, initiating a vast number of asynchronous file operations without any concurrency control can lead to errors like EMFILE: too many open files. This happens because the OS has a limit on how many files can be open at once, and Node.js doesn't automatically throttle the rate of file system calls to stay within these limits.

By implementing a concurrency limit (e.g., using the p-limit library), we can control the number of simultaneous file operations, preventing resource exhaustion while still benefiting from asynchronous processing. This approach helps maintain optimal performance without overloading the system's resources.

In essence, while Node.js handles asynchronous execution, it's important for us to manage the concurrency of resource-intensive operations like file I/O to avoid hitting OS-imposed limits.

Let me know if you have any questions or need assistance with implementing this approach.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema, understood. I'll open a new GitHub issue to track the enhancement regarding the concurrency limit.

Additionally, to incorporate the new tests for check-markdown.js into the CI pipeline, we should update the workflow file to ensure these tests are executed during continuous integration. Here's a suggested update to the workflow file:

# .github/workflows/ci.yml

name: CI

on:
  push:
    branches: [main]
  pull_request:
    branches: [main]

jobs:
  build:
    runs-on: ubuntu-latest

    steps:
      - uses: actions/checkout@v2

      - name: Set up Node.js
        uses: actions/setup-node@v2
        with:
          node-version: '16'

      - name: Install dependencies
        run: npm install

      - name: Run Tests
        run: npm test

      # Add a new step to run the markdown tests
      - name: Run Markdown Validation Tests
        run: npm test tests/markdown/check-markdown.test.js

This addition of the Run Markdown Validation Tests step will execute the new tests you've added for check-markdown.js during the CI process. Adjust the command if your test script or directory structure differs.

Let me know if you need any further assistance with this!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, workflow should be updated here - https://github.com/asyncapi/website/blob/master/.github/workflows/if-nodejs-pr-testing.yml#L84, to include secrets for the environment variable added.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema, understood.

To update the workflow at .github/workflows/if-nodejs-pr-testing.yml to include the secrets for the environment variable, you can modify the workflow file as follows:

# .github/workflows/if-nodejs-pr-testing.yml

# ...

jobs:
  build:
    runs-on: ubuntu-latest
    env:
      CONCURRENCY_LIMIT: ${{ secrets.CONCURRENCY_LIMIT }}

    steps:
      - uses: actions/checkout@v2

      - name: Set up Node.js
        uses: actions/setup-node@v2
        with:
          node-version: '16'

      - name: Install dependencies
        run: npm install

      - name: Run Tests
        run: npm test

      # Other steps...

Notes:

  • Environment Variables: By adding the env section under the build job, the CONCURRENCY_LIMIT environment variable will be available to all steps within the job.
  • Secrets Management: Ensure that the CONCURRENCY_LIMIT secret is added to your repository's secrets in GitHub settings. You can do this by navigating to Settings > Secrets and variables > Actions and adding a new repository secret named CONCURRENCY_LIMIT.
  • Usage in Scripts: Your script check-markdown.js can now access the CONCURRENCY_LIMIT environment variable via process.env.CONCURRENCY_LIMIT.

Let me know if this addresses the issue or if you need any further assistance!


✏️ Learnings added
Learnt from: akshatnema
PR: asyncapi/website#3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.472Z
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.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly create an issue for this, describing whole scenario in detail.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema, I've created the issue as requested, including a detailed description.

You can find it here: Limit concurrency in markdown file processing to improve performance.

Let me know if there's anything else I can assist with.

🔗 Analysis chain

@akshatnema, understood. I've prepared a GitHub issue to track this enhancement in detail.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Create a new GitHub issue to track the enhancement
gh issue create \
  --title "Limit concurrency in markdown file processing to improve performance" \
  --body "This issue aims to address the need for limiting concurrency in processing markdown files to improve performance and prevent potential resource exhaustion.

Currently, \`scripts/markdown/check-markdown.js\` processes markdown files asynchronously without a concurrency limit, which could lead to too many open file handles and impact system resources.

As discussed in [PR #3378](https://github.com/asyncapi/website/pull/3378) and the [related comment](https://github.com/asyncapi/website/pull/3378#discussion_r1855077352), it's important to implement concurrency control in the script.

Assigning to @akshatnema for further action." \
  --assignee akshatnema

Length of output: 809

const matter = require('gray-matter');
const path = require('path');

Expand Down Expand Up @@ -98,49 +98,44 @@
* @param {Function} validateFunction - The function used to validate the frontmatter.
* @param {string} [relativePath=''] - The relative path of the folder for logging purposes.
*/
function checkMarkdownFiles(folderPath, validateFunction, relativePath = '') {
fs.readdir(folderPath, (err, files) => {
if (err) {
console.error('Error reading directory:', err);
return;
}

files.forEach(file => {
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;

Check warning on line 110 in scripts/markdown/check-markdown.js

View check run for this annotation

Codecov / codecov/patch

scripts/markdown/check-markdown.js#L110

Added line #L110 was not covered by tests
}

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);
}
Copy link

@coderabbitai coderabbitai bot Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error handling and optimize async operations.

Several improvements could be made to this function:

  1. The error handling is too generic
  2. Multiple await operations inside the loop could impact performance
  3. 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.

Suggested change
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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishvamsinh28 Can you apply this suggestion?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

const docsFolderPath = path.resolve(__dirname, '../../markdown/docs');
const blogsFolderPath = path.resolve(__dirname, '../../markdown/blog');

checkMarkdownFiles(docsFolderPath, validateDocs);
checkMarkdownFiles(blogsFolderPath, validateBlogs);

module.exports = { validateBlogs, validateDocs, checkMarkdownFiles }
Copy link

@coderabbitai coderabbitai bot Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for main script execution

While the checkMarkdownFiles function has proper error handling, the main script execution doesn't handle potential errors.

Consider wrapping the main execution in an async function with error handling:

-checkMarkdownFiles(docsFolderPath, validateDocs);
-checkMarkdownFiles(blogsFolderPath, validateBlogs);
+async function main() {
+    try {
+        await Promise.all([
+            checkMarkdownFiles(docsFolderPath, validateDocs),
+            checkMarkdownFiles(blogsFolderPath, validateBlogs)
+        ]);
+    } catch (error) {
+        console.error('Failed to validate markdown files:', error);
+        process.exit(1);
+    }
+}
+
+main();
📝 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.

Suggested change
checkMarkdownFiles(docsFolderPath, validateDocs);
checkMarkdownFiles(blogsFolderPath, validateBlogs);
module.exports = { validateBlogs, validateDocs, checkMarkdownFiles }
async function main() {
try {
await Promise.all([
checkMarkdownFiles(docsFolderPath, validateDocs),
checkMarkdownFiles(blogsFolderPath, validateBlogs)
]);
} catch (error) {
console.error('Failed to validate markdown files:', error);
process.exit(1);
}
}
main();
module.exports = { validateBlogs, validateDocs, checkMarkdownFiles }
🧰 Tools
🪛 eslint (1.23.1)

[error] 149-149: Insert ;

(prettier/prettier)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishvamsinh28 Apply this suggestion as it will improve the performance of the script

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

84 changes: 84 additions & 0 deletions tests/markdown/check-markdown.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const fs = require('fs').promises;
const path = require('path');
const os = require('os');
const {
validateBlogs,
validateDocs,
checkMarkdownFiles
} = require('../../scripts/markdown/check-markdown');

describe('Frontmatter Validator', () => {
let tempDir;
let mockConsoleError;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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)

beforeEach(async () => {
mockConsoleError = jest.spyOn(console, 'error').mockImplementation();
tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'test-config'));
});

afterEach(async () => {
mockConsoleError.mockRestore();
await fs.rm(tempDir, { recursive: true, force: true });
});

it('validates authors array and returns specific errors', async () => {
const frontmatter = {
title: 'Test Blog',
date: '2024-01-01',
type: 'blog',
tags: ['test'],
cover: 'cover.jpg',
authors: [{ name: 'John' }, { photo: 'jane.jpg' }, { name: 'Bob', photo: 'bob.jpg', link: 'not-a-url' }]
};

const errors = validateBlogs(frontmatter);
expect(errors).toEqual(expect.arrayContaining([
'Author at index 0 is missing a photo',
'Author at index 1 is missing a name',
'Invalid URL for author at index 2: not-a-url'
]));
});

it('validates docs frontmatter for required fields', async () => {
const frontmatter = { title: 123, weight: 'not-a-number' };
const errors = validateDocs(frontmatter);
expect(errors).toEqual(expect.arrayContaining([
'Title is missing or not a string',
'Weight is missing or not a number'
]));
});

it('checks for errors in markdown files in a directory', async () => {
await fs.writeFile(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();
});

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);
});
Copy link

@coderabbitai coderabbitai bot Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!


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();
});
});
Loading