feat: added test for build-rss.js #3101
Conversation
vishvamsinh28
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3101--asyncapi-website.netlify.app/ |
sambhavgupta0705
added
the
gsoc
|
@vishvamsinh28 Statement and branch coverage aren't 100% for this file.
|
|
@anshgoyalevil updated the test, now it has 98.33% coverage. I will push another change to make it 100% |
scripts/build-rss.js
Outdated
| const xml = json2xml.getXml(feed, '@', '', 2) | ||
| fs.writeFileSync(`./public/${outputPath}`, xml, 'utf8') | ||
| } catch (err) { | ||
| throw new Error(`Failed to generate RSS feed: ${err.message}`); |
There was a problem hiding this comment.
instead of throwing error, can we return error as promise?
There was a problem hiding this comment.
We are processing the script synchronously, so returning a promise wouldn't be appropriate here
There was a problem hiding this comment.
Catching of error doesn't depend on whether the function is synchronous or asynchronous. It will still return the error, based on the function return type.
tests/build-rss.test.js
Outdated
| }); | ||
|
|
||
| it('should generate RSS feed and write to file', () => { | ||
| rssFeed(type, title, desc, outputPath); |
There was a problem hiding this comment.
can we add a try/catch block here to track the err variable here and see if any error is not returned from function execution? Try to do it for each test.
tests/fixtures/rssData.js
Outdated
There was a problem hiding this comment.
Why are we not following eslint rules inside this file? I see lot of indentation spaces here.
There was a problem hiding this comment.
I'm not getting any lint errors locally, and I have removed unnecessary indentation in previous commits.
|
There's still room for improvement, @vishvamsinh28 |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (7)
tests/fixtures/rssData.js (4)
1-43: LGTM! Consider adding more edge cases.The
mockRssDatastructure is well-designed for testing various scenarios, including featured and non-featured posts, different image formats, and consistent date formats.Consider adding more edge cases to enhance test coverage, such as:
- A post with an empty excerpt
- A post with a very long title or excerpt
- A post with special characters in the title or slug
63-72: Good test case for incomplete posts. Consider expanding.The
incompletePostMockDataprovides a valuable test case for handling posts with missing title attributes.Consider expanding this test case to include other scenarios of incomplete data, such as:
- A post missing an excerpt
- A post with an empty slug
This would provide more comprehensive coverage for error handling in the RSS builder.
74-77: LGTM! Consider using an object for configuration.The constants
type,title,desc, andoutputPathprovide clear configuration for the RSS feed generation.Consider grouping these configuration constants into a single object for better organization:
const rssConfig = { type: 'blog', title: 'Test Blog RSS', desc: 'Test blog RSS feed', outputPath: 'test-output/blog.xml' };This approach would make it easier to add or modify configuration options in the future.
79-79: Consider reordering exports for consistency.The module exports include all necessary constants, which is good.
Consider reordering the exports to match the order of declaration in the file for better readability:
module.exports = { mockRssData, missingDateMockData, incompletePostMockData, type, title, desc, outputPath };This makes it easier to locate exported items when referencing the file.
scripts/build-rss.js (2)
57-58: Enhance error messages with post identifiersWhen throwing an error due to missing required fields, including the post's identifier can help in debugging.
Modify the error message:
- throw new Error('Missing required fields in post data'); + throw new Error(`Missing required fields in post data for post: ${post.slug || post.title || 'unknown'}`);
67-72: Simplify image type detection logicThe current logic checks for multiple image types using multiple
ifstatements. Simplify it by using a mapping of extensions to MIME types.Refactor the code:
if (typeof enclosure["@url"] === 'string') { let tmp = enclosure["@url"].toLowerCase(); - if (tmp.indexOf('.png') >= 0) enclosure["@type"] = 'image/png'; - if (tmp.indexOf('.svg') >= 0) enclosure["@type"] = 'image/svg+xml'; - if (tmp.indexOf('.webp') >= 0) enclosure["@type"] = 'image/webp'; + const extension = tmp.split('.').pop().split('?')[0]; + const mimeTypes = { + png: 'image/png', + jpg: 'image/jpeg', + jpeg: 'image/jpeg', + svg: 'image/svg+xml', + webp: 'image/webp' + }; + enclosure["@type"] = mimeTypes[extension] || 'image/jpeg'; }This reduces code repetition and makes it easier to add support for more image types in the future.
tests/build-rss.test.js (1)
17-23: Handle potential errors during cleanup inafterAllWhen deleting files and directories during cleanup, it's good practice to handle potential errors to prevent tests from failing unexpectedly.
Example:
afterAll(() => { if (fs.existsSync(testOutputDir)) { fs.readdirSync(testOutputDir).forEach(file => { try { fs.unlinkSync(path.join(testOutputDir, file)); } catch (err) { console.error(`Error deleting file: ${file}`, err); } }); try { fs.rmdirSync(testOutputDir); } catch (err) { console.error(`Error removing directory: ${testOutputDir}`, err); } } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- scripts/build-rss.js (2 hunks)
- tests/build-rss.test.js (1 hunks)
- tests/fixtures/rssData.js (1 hunks)
🔇 Additional comments (3)
tests/fixtures/rssData.js (2)
45-61: Good test case for handling missing dates.The
missingDateMockDataprovides a valuable test case for ensuring the RSS builder can handle posts with missing date attributes.
1-79: Overall structure looks good. Indentation issues resolved.The file is well-structured with clear separation between different mock data sets. The indentation appears consistent throughout the file, addressing the concerns raised in previous reviews.
As mentioned in a previous review by @akshatnema, it's important to follow eslint rules. The current version of the file seems to adhere to consistent indentation practices, resolving the earlier concerns.
tests/build-rss.test.js (1)
100-110: 🛠️ Refactor suggestionCheck for the existence of enclosure elements before assertions
Currently, the test assumes that all enclosure elements exist in the RSS feed. To prevent potential errors, ensure that the enclosure elements are present before making assertions on them.
Example:
const parser = require('fast-xml-parser'); const xmlData = fs.readFileSync(filePath, 'utf8'); const parsedData = parser.parse(xmlData); // Verify that each item has an enclosure parsedData.rss.channel.item.forEach(item => { expect(item.enclosure).toBeDefined(); });
scripts/build-rss.js
Outdated
| throw new Error('Missing required fields in post data'); | ||
| } | ||
| const link = `${base}${post.slug}${tracking}`; | ||
| const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability by destructuring and formatting
The construction of the item object can be made more readable by destructuring post and formatting the object properties.
Refactor as follows:
- const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() }
+ const { title, excerpt, date } = post;
+ const pubDate = new Date(date).toUTCString();
+ const description = clean(excerpt);
+ const guid = { '@isPermaLink': true, '': link };
+ const item = {
+ title,
+ description,
+ link,
+ category: type,
+ guid,
+ pubDate
+ };This enhances readability and maintainability.
📝 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.
| const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() } | |
| const { title, excerpt, date } = post; | |
| const pubDate = new Date(date).toUTCString(); | |
| const description = clean(excerpt); | |
| const guid = { '@isPermaLink': true, '': link }; | |
| const item = { | |
| title, | |
| description, | |
| link, | |
| category: type, | |
| guid, | |
| pubDate | |
| }; |
There was a problem hiding this comment.
@vishvamsinh28 Apply this suggestion in the code.
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
scripts/build-rss.js
Outdated
| return new Promise((resolve, reject) => { | ||
| try { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using asynchronous file operations with Promises
The function rssFeed is now returning a Promise but still uses synchronous file operations like fs.writeFileSync. To fully leverage asynchronous behavior and avoid blocking the event loop, consider using fs.writeFile from the fs/promises module.
Apply this change:
- const fs = require('fs')
+ const fs = require('fs').promises
...
- fs.writeFileSync(`./public/${outputPath}`, xml, 'utf8')
+ await fs.writeFile(`./public/${outputPath}`, xml, 'utf8')Remember to make the function async:
- module.exports = function rssFeed(type, title, desc, outputPath) {
+ module.exports = async function rssFeed(type, title, desc, outputPath) {Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
@vishvamsinh28 apply this suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@akshatnema Done
scripts/build-rss.js
Outdated
| }) | ||
|
|
||
| if (i1.featured && !i2.featured) return -1 | ||
| if (!i1.featured && i2.featured) return 1 | ||
| return i2Date - i1Date | ||
| }) | ||
| const base = 'https://www.asyncapi.com' | ||
| const tracking = '?utm_source=rss'; | ||
|
|
||
| const base = 'https://www.asyncapi.com' | ||
| const tracking = '?utm_source=rss'; | ||
| const feed = {} | ||
| const rss = {} | ||
| rss['@version'] = '2.0' | ||
| rss["@xmlns:atom"] = 'http://www.w3.org/2005/Atom' | ||
| rss.channel = {} | ||
| rss.channel.title = title | ||
| rss.channel.link = `${base}/${outputPath}` | ||
| rss.channel["atom:link"] = {} | ||
| rss.channel["atom:link"]["@rel"] = 'self' | ||
| rss.channel["atom:link"]["@href"] = rss.channel.link | ||
| rss.channel["atom:link"]["@type"] = 'application/rss+xml' | ||
| rss.channel.description = desc | ||
| rss.channel.language = 'en-gb'; | ||
| rss.channel.copyright = 'Made with :love: by the AsyncAPI Initiative.'; | ||
| rss.channel.webMaster = '[email protected] (AsyncAPI Initiative)' | ||
| rss.channel.pubDate = new Date().toUTCString() | ||
| rss.channel.generator = 'next.js' | ||
| rss.channel.item = [] | ||
|
|
||
| const feed = {} | ||
| const rss = {} | ||
| rss['@version'] = '2.0' | ||
| rss["@xmlns:atom"] = 'http://www.w3.org/2005/Atom' | ||
| rss.channel = {} | ||
| rss.channel.title = title | ||
| rss.channel.link = `${base}/${outputPath}` | ||
| rss.channel["atom:link"] = {} | ||
| rss.channel["atom:link"]["@rel"] = 'self' | ||
| rss.channel["atom:link"]["@href"] = rss.channel.link | ||
| rss.channel["atom:link"]["@type"] = 'application/rss+xml' | ||
| rss.channel.description = desc | ||
| rss.channel.language = 'en-gb'; | ||
| rss.channel.copyright = 'Made with :love: by the AsyncAPI Initiative.'; | ||
| rss.channel.webMaster = '[email protected] (AsyncAPI Initiative)' | ||
| rss.channel.pubDate = new Date().toUTCString() | ||
| rss.channel.generator = 'next.js' | ||
| rss.channel.item = [] | ||
|
|
||
| for (let post of posts) { | ||
| const link = `${base}${post.slug}${tracking}`; | ||
| const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() } | ||
| if (post.cover) { | ||
| const enclosure = {}; | ||
| enclosure["@url"] = base+post.cover; | ||
| enclosure["@length"] = 15026; // dummy value, anything works | ||
| enclosure["@type"] = 'image/jpeg'; | ||
| if (typeof enclosure["@url"] === 'string') { | ||
| let tmp = enclosure["@url"].toLowerCase(); | ||
| if (tmp.indexOf('.png')>=0) enclosure["@type"] = 'image/png'; | ||
| if (tmp.indexOf('.svg')>=0) enclosure["@type"] = 'image/svg+xml'; | ||
| if (tmp.indexOf('.webp')>=0) enclosure["@type"] = 'image/webp'; | ||
| for (let post of posts) { | ||
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | ||
| throw new Error('Missing required fields in post data'); | ||
| } | ||
| const link = `${base}${post.slug}${tracking}`; | ||
| const item = { title: post.title, description: clean(post.excerpt), link, category: type, guid: { '@isPermaLink': true, '': link }, pubDate: new Date(post.date).toUTCString() } | ||
| if (post.cover) { | ||
| const enclosure = {}; | ||
| enclosure["@url"] = base + post.cover; | ||
| enclosure["@length"] = 15026; // dummy value, anything works | ||
| enclosure["@type"] = 'image/jpeg'; | ||
| if (typeof enclosure["@url"] === 'string') { | ||
| let tmp = enclosure["@url"].toLowerCase(); | ||
| if (tmp.indexOf('.png') >= 0) enclosure["@type"] = 'image/png'; | ||
| if (tmp.indexOf('.svg') >= 0) enclosure["@type"] = 'image/svg+xml'; | ||
| if (tmp.indexOf('.webp') >= 0) enclosure["@type"] = 'image/webp'; | ||
| } | ||
| item.enclosure = enclosure; | ||
| } | ||
| rss.channel.item.push(item) | ||
| } | ||
| item.enclosure = enclosure; | ||
| } | ||
| rss.channel.item.push(item) | ||
| } | ||
|
|
||
| feed.rss = rss | ||
| feed.rss = rss | ||
|
|
||
| const xml = json2xml.getXml(feed,'@','',2) | ||
| fs.writeFileSync(`./public/${outputPath}`, xml, 'utf8') | ||
| const xml = json2xml.getXml(feed, '@', '', 2); | ||
| fs.writeFileSync(`./public/${outputPath}`, xml, 'utf8') | ||
| resolve(`RSS feed generated successfully at ${outputPath}`); | ||
| } catch (err) { | ||
| reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage async/await for cleaner asynchronous code
By using async/await, the code becomes more readable and maintains the asynchronous behavior.
Modify the function definition and use await:
- module.exports = function rssFeed(type, title, desc, outputPath) {
+ module.exports = async function rssFeed(type, title, desc, outputPath) {Replace return new Promise and the try...catch block:
- return new Promise((resolve, reject) => {
- try {
...
- resolve(`RSS feed generated successfully at ${outputPath}`);
- } catch (err) {
- reject(new Error(`Failed to generate RSS feed: ${err.message}`));
- }
- });
+ try {
...
+ await fs.writeFile(`./public/${outputPath}`, xml, 'utf8');
+ return `RSS feed generated successfully at ${outputPath}`;
+ } catch (err) {
+ throw new Error(`Failed to generate RSS feed: ${err.message}`);
+ }This refactoring streamlines the asynchronous operations and error handling.
Committable suggestion was skipped due to low confidence.
scripts/build-rss.js
Outdated
| .sort((i1, i2) => { | ||
| if (!i1.date || !i2.date) { | ||
| throw new Error('Missing date in post data'); | ||
| } | ||
| const i1Date = new Date(i1.date) | ||
| const i2Date = new Date(i2.date) | ||
| if (i1.featured && !i2.featured) return -1 | ||
| if (!i1.featured && i2.featured) return 1 | ||
| return i2Date - i1Date | ||
| }) |
There was a problem hiding this comment.
Avoid throwing errors inside the sort comparator function
Throwing an error within the comparator can disrupt the sorting process and lead to unexpected behavior. It's better to validate the data before sorting.
Refactor the code to filter out posts missing dates before sorting:
- const posts = getAllPosts()[`${type}`]
- .sort((i1, i2) => {
- if (!i1.date || !i2.date) {
- throw new Error('Missing date in post data');
- }
- const i1Date = new Date(i1.date)
- const i2Date = new Date(i2.date)
- if (i1.featured && !i2.featured) return -1
- if (!i1.featured && i2.featured) return 1
- return i2Date - i1Date
- })
+ let posts = getAllPosts()[`${type}`]
+ posts = posts.filter(post => {
+ if (!post.date) {
+ console.warn(`Post "${post.title || post.slug}" is missing a date and will be excluded.`);
+ return false;
+ }
+ return true;
+ });
+ posts.sort((i1, i2) => {
+ const i1Date = new Date(i1.date)
+ const i2Date = new Date(i2.date)
+ if (i1.featured && !i2.featured) return -1
+ if (!i1.featured && i2.featured) return 1
+ return i2Date - i1Date
+ })This ensures the sorting function only handles valid data and improves robustness.
📝 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.
| .sort((i1, i2) => { | |
| if (!i1.date || !i2.date) { | |
| throw new Error('Missing date in post data'); | |
| } | |
| const i1Date = new Date(i1.date) | |
| const i2Date = new Date(i2.date) | |
| if (i1.featured && !i2.featured) return -1 | |
| if (!i1.featured && i2.featured) return 1 | |
| return i2Date - i1Date | |
| }) | |
| let posts = getAllPosts()[`${type}`] | |
| posts = posts.filter(post => { | |
| if (!post.date) { | |
| console.warn(`Post "${post.title || post.slug}" is missing a date and will be excluded.`); | |
| return false; | |
| } | |
| return true; | |
| }); | |
| posts.sort((i1, i2) => { | |
| const i1Date = new Date(i1.date) | |
| const i2Date = new Date(i2.date) | |
| if (i1.featured && !i2.featured) return -1 | |
| if (!i1.featured && i2.featured) return 1 | |
| return i2Date - i1Date | |
| }) |
There was a problem hiding this comment.
@vishvamsinh28 apply this suggestion in your code.
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
tests/build-rss.test.js
Outdated
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
|
|
||
| expect(error).toBeUndefined(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify asynchronous tests using Jest's resolves
Instead of using a try/catch block to handle the Promise, you can utilize Jest's expect().resolves for cleaner and more readable tests.
Refactored code:
await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();This approach automatically handles the Promise resolution and failure.
tests/build-rss.test.js
Outdated
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify asynchronous tests using Jest's resolves
Refactor the test to use expect().resolves for better readability.
Refactored code:
await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
tests/build-rss.test.js
Outdated
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error).toBeDefined(); | ||
| expect(error.message).toContain('Missing required fields'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Jest's rejects.toThrow for expected errors
Simplify the test by asserting the Promise rejection directly.
Refactored code:
await expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Missing required fields');
tests/build-rss.test.js
Outdated
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error).toBeDefined(); | ||
| expect(error.message).toContain('Missing date in post data'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Jest's rejects.toThrow for expected errors
Enhance the test by using expect().rejects.toThrow().
Refactored code:
await expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Missing date in post data');
tests/build-rss.test.js
Outdated
| const itemTitles = fileContent.match(/<title>(.*?)<\/title>/g); | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| expect(itemTitles[1]).toContain('Test Post 1'); | ||
| expect(itemTitles[2]).toContain('Another Featured Post'); | ||
| expect(itemTitles[3]).toContain('Non-Featured Post 1'); | ||
| expect(itemTitles[4]).toContain('Non-Featured Post 3'); | ||
| expect(itemTitles[5]).toContain('Non-Featured Post 2'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use an XML parser instead of regex for parsing RSS feed
For consistent and reliable parsing of the RSS feed, utilize an XML parser.
Example using fast-xml-parser:
const parser = require('fast-xml-parser');
const xmlData = fs.readFileSync(filePath, 'utf8');
const parsedData = parser.parse(xmlData);
const itemTitles = parsedData.rss.channel.item.map(item => item.title);
// Now you can assert on itemTitles array
expect(itemTitles[0]).toContain('Test Post 1');This method avoids potential issues with regex parsing and handles XML structures appropriately.
| afterEach(() => { | ||
| jest.resetModules(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset mocks instead of modules in afterEach
Instead of using jest.resetModules(), consider using jest.resetAllMocks() or jest.clearAllMocks() to reset your mocks between tests. This is generally more efficient and avoids unnecessary module resets.
Refactored code:
afterEach(() => {
jest.resetAllMocks();
});
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.resetModules(); |
There was a problem hiding this comment.
@vishvamsinh28 any specific reason on why are we resetting all modules instead of mocks here?
There was a problem hiding this comment.
I tried using jest.resetAllMocks() and jest.clearAllMocks(), but they didn't work as expected because posts.json still contains cached data during the next test but jest.resetModules() clears everything loads with freshdata
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/build-rss.js (2)
58-74: LGTM: Improved error handling and code readabilityThe changes in this section are good improvements:
- Moving RSS feed generation inside the try block enhances error handling.
- Checking for required fields prevents issues with incomplete data.
- The refactored item object creation improves code readability.
One minor suggestion:
Consider using object shorthand notation for the
itemobject creation:const item = { title, description, link, category: type, guid, pubDate };This makes the code even more concise.
18-98: LGTM: Overall improvements to structure and robustnessThe changes to the
rssFeedfunction have significantly improved its reliability, maintainability, and error handling. The async structure, additional checks, and improved error handling make the function more robust.One minor suggestion for consistency:
Consider using arrow function syntax for the
getAllPostsfunction to match the style of the mainrssFeedfunction:const getAllPosts = () => { return require('../config/posts.json'); };This would make the code style more consistent throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- scripts/build-rss.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
scripts/build-rss.js (5)
1-1: LGTM: Good use of promise-based fs moduleThe change to import
fs.promisesaligns well with the async nature of the function. This is a good practice for handling file operations asynchronously.
18-19: LGTM: Proper async function declarationThe change to declare
rssFeedas an async function is appropriate given the asynchronous operations within. This allows for better error handling and use of await.
21-27: LGTM: Improved post filteringThe addition of a filter to remove posts without dates before sorting is a good improvement. This addresses the previous concern about throwing errors in the sort comparator and makes the code more robust.
94-94: LGTM: Proper use of async file writingThe use of
awaitwithfs.writeFileis correct and consistent with the async nature of the function. This ensures proper handling of the asynchronous file writing operation.
96-98: LGTM: Improved error handlingThe addition of a try-catch block wrapping the entire function logic and throwing errors with more informative messages is a good improvement. This enhances the overall error handling and debugging capabilities of the function.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
tests/build-rss.test.js (1)
25-27: Consider usingjest.resetAllMocks()instead ofjest.resetModules()While
jest.resetModules()works for clearing cached data between tests, it might be more efficient to usejest.resetAllMocks()if you're primarily concerned with resetting mock functions. However, if clearing module cache is necessary for your tests to run correctly with fresh data, then the current implementation is acceptable.If you decide to keep
jest.resetModules(), consider adding a comment explaining why it's necessary:afterEach(() => { // Reset modules to ensure fresh data is loaded for each test jest.resetModules(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/build-rss.test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
tests/build-rss.test.js (1)
1-186: Comprehensive test suite with room for improvementOverall, this is a well-structured and comprehensive test suite for the
rssFeedfunction. It covers a wide range of scenarios including both positive and negative cases. Here are some general suggestions for improvement:
- Consistently use
expect().resolvesandexpect().rejectsfor asynchronous tests.- Implement an XML parser for more robust checking of RSS feed content.
- Consider extracting common setup code (like mocking
posts.json) into helper functions to reduce repetition.- Add tests for edge cases if not already covered, such as extremely large post arrays or posts with unusual characters in their content.
Great job on creating a thorough test suite. Implementing these suggestions will further enhance its robustness and maintainability.
tests/build-rss.test.js
Outdated
| it('should generate RSS feed and write to file', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| expect(fs.existsSync(filePath)).toBe(true); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
| expect(fileContent).toContain('<rss version="2.0"'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify asynchronous test using expect().resolves
Consider refactoring this test to use expect().resolves for a cleaner and more idiomatic Jest asynchronous test. This approach eliminates the need for a try/catch block and makes the test more concise.
Here's how you could refactor the test:
it('should generate RSS feed and write to file', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
const filePath = path.join(__dirname, '..', 'public', outputPath);
expect(fs.existsSync(filePath)).toBe(true);
const fileContent = fs.readFileSync(filePath, 'utf8');
expect(fileContent).toContain('<rss version="2.0"');
});This refactoring applies to other similar test cases in this file as well.
There was a problem hiding this comment.
@akshat-nema, thank you for pointing that out. You bring up a good question regarding error checking when the function rejects. In the refactored test using expect().resolves, we're asserting that the rssFeed function resolves successfully without throwing an error. If rssFeed were to reject (i.e., throw an error), the test would fail, thus effectively checking for errors.
Here's how it works:
it('should generate RSS feed and write to file', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
await expect(rssFeed(type, title, desc, outputPath)).resolves.toBeUndefined();
const filePath = path.join(__dirname, '..', 'public', outputPath);
expect(fs.existsSync(filePath)).toBe(true);
const fileContent = fs.readFileSync(filePath, 'utf8');
expect(fileContent).toContain('<rss version="2.0"');
});In this refactored version:
await expect(rssFeed(...)).resolves.toBeUndefined();asserts that therssFeedfunction resolves without returning a value (i.e.,undefined). If the function rejects with an error, the test will fail.
Alternatively, since we're primarily checking that rssFeed does not throw an error, we can simplify the test by removing the explicit error handling:
it('should generate RSS feed and write to file', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
await rssFeed(type, title, desc, outputPath);
const filePath = path.join(__dirname, '..', 'public', outputPath);
expect(fs.existsSync(filePath)).toBe(true);
const fileContent = fs.readFileSync(filePath, 'utf8');
expect(fileContent).toContain('<rss version="2.0"');
});In this case:
- If
rssFeedthrows an error, the test will automatically fail. - This approach relies on Jest's default behavior for unhandled promise rejections in async tests.
Both refactored versions effectively check for errors by ensuring that the function completes successfully. Let me know if this clarifies your concern or if you have any further questions.
tests/build-rss.test.js
Outdated
| it('should prioritize featured posts over non-featured ones', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err | ||
| } | ||
|
|
||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
|
|
||
| const itemTitles = fileContent.match(/<title>(.*?)<\/title>/g); | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| expect(itemTitles[1]).toContain('Test Post 1'); | ||
| expect(itemTitles[2]).toContain('Another Featured Post'); | ||
| expect(itemTitles[3]).toContain('Non-Featured Post 1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use an XML parser for more reliable RSS feed parsing
While using regex to extract titles works, it can be brittle and prone to errors if the XML structure changes. Consider using an XML parser like fast-xml-parser for more robust and reliable parsing of the RSS feed.
Here's an example of how you could refactor this using fast-xml-parser:
const parser = require('fast-xml-parser');
// ... (in the test case)
const xmlData = fs.readFileSync(filePath, 'utf8');
const parsedData = parser.parse(xmlData);
const items = parsedData.rss.channel.item;
expect(items[0].title).toContain('Test Post 1');
expect(items[1].title).toContain('Another Featured Post');
expect(items[2].title).toContain('Non-Featured Post 1');This approach would make your tests more resilient to changes in XML formatting and structure.
tests/build-rss.test.js
Outdated
| it('should sort posts by date in descending order', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err | ||
| } | ||
|
|
||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
|
|
||
| const itemTitles = fileContent.match(/<title>(.*?)<\/title>/g); | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| expect(itemTitles[1]).toContain('Test Post 1'); | ||
| expect(itemTitles[2]).toContain('Another Featured Post'); | ||
| expect(itemTitles[3]).toContain('Non-Featured Post 1'); | ||
| expect(itemTitles[4]).toContain('Non-Featured Post 3'); | ||
| expect(itemTitles[5]).toContain('Non-Featured Post 2'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistently use an XML parser across all tests
Similar to the previous test, this test case would benefit from using an XML parser instead of regex. This would ensure consistency across your test suite and make the tests more robust.
Apply the same XML parsing approach suggested in the previous comment to this test case as well. This will make your tests more maintainable and less prone to errors due to XML structure changes.
tests/build-rss.test.js
Outdated
| it('should set correct enclosure type based on image extension', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err | ||
| } | ||
|
|
||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.png"'); | ||
| expect(fileContent).toContain('type="image/png"'); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.svg"'); | ||
| expect(fileContent).toContain('type="image/svg+xml"'); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.webp"'); | ||
| expect(fileContent).toContain('type="image/webp"'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance enclosure type checking with XML parsing
The current string matching approach works but could be made more robust by using an XML parser. This would allow you to check the exact structure and attributes of the enclosure tags.
Consider refactoring this test to use an XML parser:
const parser = require('fast-xml-parser');
// ... (in the test case)
const xmlData = fs.readFileSync(filePath, 'utf8');
const parsedData = parser.parse(xmlData, { attributeNamePrefix: "", ignoreAttributes: false });
const items = parsedData.rss.channel.item;
expect(items[0].enclosure.url).toBe("https://www.asyncapi.com/img/test-cover.png");
expect(items[0].enclosure.type).toBe("image/png");
expect(items[1].enclosure.url).toBe("https://www.asyncapi.com/img/test-cover.svg");
expect(items[1].enclosure.type).toBe("image/svg+xml");
expect(items[2].enclosure.url).toBe("https://www.asyncapi.com/img/test-cover.webp");
expect(items[2].enclosure.type).toBe("image/webp");This approach would provide more precise checking of the RSS feed structure and content.
tests/build-rss.test.js
Outdated
| it('should catch and handle errors when write operation fails', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| const invalidOutputPath = "invalid/path"; | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, invalidOutputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error.message).toMatch(/ENOENT|EACCES/); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify error testing with expect().rejects
The current implementation using a try/catch block works, but Jest provides a cleaner way to test for rejected promises using expect().rejects.
Consider refactoring this test as follows:
it('should catch and handle errors when write operation fails', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
const invalidOutputPath = "invalid/path";
await expect(rssFeed(type, title, desc, invalidOutputPath))
.rejects.toThrow(/ENOENT|EACCES/);
});This approach is more concise and directly asserts that the promise rejects with an error matching the specified pattern.
tests/build-rss.test.js
Outdated
| it('should throw an error when posts.json is malformed', async () => { | ||
| jest.doMock('../config/posts.json', () => { | ||
| return { invalidKey: [] }; | ||
| }, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error).toBeDefined(); | ||
| expect(error.message).toContain('Failed to generate RSS feed'); | ||
| } | ||
| }); | ||
|
|
||
| it('should handle empty posts array', async () => { | ||
| const emptyMockData = { blog: [] }; | ||
| jest.doMock('../config/posts.json', () => emptyMockData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
| expect(fileContent).toContain('<rss version="2.0"'); | ||
| expect(fileContent).not.toContain('<item>'); | ||
| }); | ||
|
|
||
| it('should throw an error when post is missing required fields', async () => { | ||
|
|
||
| jest.doMock('../config/posts.json', () => incompletePostMockData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error).toBeDefined(); | ||
| expect(error.message).toContain('Missing required fields'); | ||
| } | ||
| }); | ||
|
|
||
| it('should throw an error when a post is missing a date field during sorting', async () => { | ||
|
|
||
| jest.doMock('../config/posts.json', () => missingDateMockData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await rssFeed(type, title, desc, outputPath); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error).toBeDefined(); | ||
| expect(error.message).toContain('Missing date in post data'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent refactoring across all remaining tests
The remaining test cases are comprehensive and cover important scenarios. However, they could benefit from the same refactoring suggestions made for previous tests:
- Use
expect().resolvesorexpect().rejectsfor asynchronous assertions. - Consider using an XML parser for more robust checking of RSS feed content.
Here's an example of how you could refactor one of these tests:
it('should throw an error when posts.json is malformed', async () => {
jest.doMock('../config/posts.json', () => ({ invalidKey: [] }), { virtual: true });
await expect(rssFeed(type, title, desc, outputPath))
.rejects.toThrow('Failed to generate RSS feed');
});Apply similar refactoring to the other remaining tests for consistency and improved readability across your test suite.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3101 +/- ##
==========================================
+ Coverage 52.30% 61.73% +9.43%
==========================================
Files 21 21
Lines 608 622 +14
==========================================
+ Hits 318 384 +66
+ Misses 290 238 -52 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
scripts/build-rss.js (3)
1-5: LGTM! Consider async JSON loading for large files.The change to use
fs.promisesis consistent with the function becoming async. However, for potentially large JSON files, consider using an async JSON parsing method to avoid blocking the event loop.You could refactor
getAllPoststo use async file reading:const getAllPosts = async () => { const data = await fs.readFile('../config/posts.json', 'utf8'); return JSON.parse(data); };This would require updating the
rssFeedfunction to awaitgetAllPosts().
21-35: LGTM! Consider adding a warning for filtered posts.The addition of filtering for posts without dates improves data integrity. The sorting logic remains correct.
Consider adding a warning message for posts that are filtered out due to missing dates:
posts = posts.filter(post => { if (!post.date) { console.warn(`Post "${post.title || 'Untitled'}" is missing a date and will be excluded from the RSS feed.`); return false; } return true; });This will help identify and fix data issues in the source JSON.
58-88: LGTM! Consider simplifying enclosure type detection.The post processing logic is well-structured with improved data integrity checks and readability. The enclosure handling is comprehensive but could be simplified.
Consider simplifying the enclosure type detection:
if (post.cover) { const enclosure = { "@url": base + post.cover, "@length": 15026, // dummy value "@type": 'image/jpeg' // default type }; const extension = post.cover.split('.').pop().toLowerCase(); const mimeTypes = { 'png': 'image/png', 'svg': 'image/svg+xml', 'webp': 'image/webp' }; enclosure["@type"] = mimeTypes[extension] || enclosure["@type"]; item.enclosure = enclosure; }This approach reduces repetition and is more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- scripts/build-rss.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
scripts/build-rss.js (5)
18-19: LGTM! Async function declaration is appropriate.The change to make
rssFeedan async function is appropriate given the asynchronous operations within the function. This allows for better error handling and use ofawaitfor asynchronous calls.
36-56: LGTM! Comprehensive RSS metadata setup.The RSS feed metadata setup is comprehensive and follows the RSS 2.0 specifications. The use of template literals for URLs and the consistent application of the
trackingparameter are appropriate.
93-94: LGTM! Appropriate XML generation and async file writing.The XML generation using
json2xml.getXmland the async file writing withfs.writeFileare appropriate and consistent with the function's async nature.
95-98: LGTM! Proper error handling and informative messages.The error handling is well-implemented, consistent with the Promise-based approach. Both success and error messages are informative, which will aid in debugging and monitoring.
Line range hint
1-98: Overall, excellent improvements to the RSS feed generation function.The changes to
rssFeedhave significantly enhanced its robustness, error handling, and code quality. The function is now asynchronous, has better data validation, and follows modern JavaScript practices. The suggestions provided in the review comments are minor optimizations that could further improve the code's efficiency and maintainability.Great job on this refactoring!
|
@vishvamsinh28 Can we apply above suggestions of reject and resolves for rssFeed function? |
|
@akshatnema test updated |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
tests/build-rss.test.js (2)
10-14: Add error handling to directory operationsThe setup and teardown logic is well-structured, but consider adding error handling for the directory operations.
beforeAll(() => { if (!fs.existsSync(testOutputDir)) { - fs.mkdirSync(testOutputDir, { recursive: true }); + try { + fs.mkdirSync(testOutputDir, { recursive: true }); + } catch (err) { + console.error(`Failed to create test directory: ${err.message}`); + throw err; + } } }); afterAll(() => { if (fs.existsSync(testOutputDir)) { + try { fs.readdirSync(testOutputDir).forEach(file => { fs.unlinkSync(path.join(testOutputDir, file)); }); fs.rmdirSync(testOutputDir); + } catch (err) { + console.error(`Failed to clean up test directory: ${err.message}`); + throw err; + } } });Also applies to: 16-23
6-187: Well-structured and comprehensive test suiteThe test suite provides excellent coverage of both positive and negative scenarios for the RSS feed generation functionality. The test cases are well-organized and cover important edge cases.
Consider these architectural improvements:
- Create a shared setup function for common test initialization
- Extract XML parsing logic into a test utility function
- Consider grouping related tests using nested describe blocks
scripts/build-rss.js (2)
52-52: Ensure the heart symbol renders correctly in the RSS feedThe string
:love:may not render as a heart symbol in the RSS feed. Replace:love:with a proper Unicode heart emoji or the word "love" to ensure it displays correctly.Apply this change:
- rss.channel.copyright = 'Made with :love: by the AsyncAPI Initiative.'; + rss.channel.copyright = 'Made with ❤️ by the AsyncAPI Initiative.';
80-85: Enhance image type detection for enclosure@typeCurrently, the code only checks for
.png,.svg, and.webpextensions. Including checks for.jpgand.jpegwill ensure that JPEG images are correctly identified and the@typeis set appropriately.Apply this change:
let tmp = enclosure["@url"].toLowerCase(); + if (tmp.indexOf('.jpg') >= 0 || tmp.indexOf('.jpeg') >= 0) enclosure["@type"] = 'image/jpeg'; if (tmp.indexOf('.png') >= 0) enclosure["@type"] = 'image/png'; if (tmp.indexOf('.svg') >= 0) enclosure["@type"] = 'image/svg+xml'; if (tmp.indexOf('.webp') >= 0) enclosure["@type"] = 'image/webp';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/build-rss.js (2 hunks)
- tests/build-rss.test.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
scripts/build-rss.js (1)
78-78:⚠️ Potential issueSet the actual file size for enclosure
@lengthThe
@lengthattribute should represent the actual size of the file in bytes. Using a dummy value may lead to incorrect metadata in the RSS feed.You can run the following script to compute the correct file sizes:
This script calculates the file sizes of all cover images, allowing you to set the accurate
@lengthvalue.
tests/build-rss.test.js
Outdated
| it('should generate RSS feed and write to file', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| let error; | ||
| try { | ||
| await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow(); | ||
| } catch (err) { | ||
| error = err; | ||
| } | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| const filePath = path.join(__dirname, '..', 'public', outputPath); | ||
| expect(fs.existsSync(filePath)).toBe(true); | ||
| const fileContent = fs.readFileSync(filePath, 'utf8'); | ||
| expect(fileContent).toContain('<rss version="2.0"'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify positive test cases using Jest's resolves pattern
The positive test cases (successful RSS generation) can be simplified by removing the try/catch blocks and error tracking.
Example refactor for the first test case:
it('should generate RSS feed and write to file', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
- let error;
- try {
- await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
- } catch (err) {
- error = err;
- }
-
- expect(error).toBeUndefined();
+ await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
const filePath = path.join(__dirname, '..', 'public', outputPath);
expect(fs.existsSync(filePath)).toBe(true);
const fileContent = fs.readFileSync(filePath, 'utf8');
expect(fileContent).toContain('<rss version="2.0"');
});Apply similar refactoring to other positive test cases.
Also applies to: 46-65, 67-88, 90-110
tests/build-rss.test.js
Outdated
| const itemTitles = fileContent.match(/<title>(.*?)<\/title>/g); | ||
|
|
||
| expect(error).toBeUndefined(); | ||
| expect(itemTitles[1]).toContain('Test Post 1'); | ||
| expect(itemTitles[2]).toContain('Another Featured Post'); | ||
| expect(itemTitles[3]).toContain('Non-Featured Post 1'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace regex parsing with an XML parser
Using regex to parse XML is fragile and could break with valid XML that doesn't match the exact pattern.
+const { XMLParser } = require('fast-xml-parser');
+const parser = new XMLParser();
it('should prioritize featured posts over non-featured ones', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
const filePath = path.join(__dirname, '..', 'public', outputPath);
const fileContent = fs.readFileSync(filePath, 'utf8');
- const itemTitles = fileContent.match(/<title>(.*?)<\/title>/g);
+ const parsed = parser.parse(fileContent);
+ const items = parsed.rss.channel.item;
- expect(itemTitles[1]).toContain('Test Post 1');
- expect(itemTitles[2]).toContain('Another Featured Post');
- expect(itemTitles[3]).toContain('Non-Featured Post 1');
+ expect(items[0].title).toBe('Test Post 1');
+ expect(items[1].title).toBe('Another Featured Post');
+ expect(items[2].title).toBe('Non-Featured Post 1');
});Also applies to: 80-88
tests/build-rss.test.js
Outdated
| it('should catch and handle errors when write operation fails', async () => { | ||
| jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); | ||
|
|
||
| const invalidOutputPath = "invalid/path"; | ||
|
|
||
| let error; | ||
| try { | ||
| await expect(rssFeed(type, title, desc, invalidOutputPath)) | ||
| .rejects.toThrow(/ENOENT|EACCES/); | ||
| } catch (err) { | ||
| error = err; | ||
| expect(error.message).toMatch(/ENOENT|EACCES/); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify negative test cases using Jest's rejects pattern
The negative test cases (error scenarios) can be simplified by removing the try/catch blocks and using Jest's rejects pattern.
Example refactor for error cases:
it('should catch and handle errors when write operation fails', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
const invalidOutputPath = "invalid/path";
- let error;
- try {
- await expect(rssFeed(type, title, desc, invalidOutputPath))
- .rejects.toThrow(/ENOENT|EACCES/);
- } catch (err) {
- error = err;
- expect(error.message).toMatch(/ENOENT|EACCES/);
- }
+ await expect(rssFeed(type, title, desc, invalidOutputPath))
+ .rejects.toThrow(/ENOENT|EACCES/);
});
it('should throw an error when posts.json is malformed', async () => {
jest.doMock('../config/posts.json', () => ({ invalidKey: [] }), { virtual: true });
- let error;
- try {
- await expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Failed to generate RSS feed');
- } catch (err) {
- error = err;
- expect(error).toBeDefined();
- expect(error.message).toContain('Failed to generate RSS feed');
- }
+ await expect(rssFeed(type, title, desc, outputPath))
+ .rejects.toThrow('Failed to generate RSS feed');
});Also applies to: 127-140, 160-172, 174-186
tests/build-rss.test.js
Outdated
| expect(error).toBeUndefined(); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.png"'); | ||
| expect(fileContent).toContain('type="image/png"'); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.svg"'); | ||
| expect(fileContent).toContain('type="image/svg+xml"'); | ||
| expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.webp"'); | ||
| expect(fileContent).toContain('type="image/webp"'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve enclosure type assertions using XML parser
The enclosure type assertions can be made more robust using XML parser instead of string matching.
+const { XMLParser } = require('fast-xml-parser');
+const parser = new XMLParser({
+ ignoreAttributes: false,
+ attributeNamePrefix: ""
+});
it('should set correct enclosure type based on image extension', async () => {
jest.doMock('../config/posts.json', () => mockRssData, { virtual: true });
await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();
const filePath = path.join(__dirname, '..', 'public', outputPath);
const fileContent = fs.readFileSync(filePath, 'utf8');
+ const parsed = parser.parse(fileContent);
+ const items = parsed.rss.channel.item;
- expect(fileContent).toContain('<enclosure url="https://www.asyncapi.com/img/test-cover.png"');
- expect(fileContent).toContain('type="image/png"');
+ expect(items[0].enclosure.url).toBe("https://www.asyncapi.com/img/test-cover.png");
+ expect(items[0].enclosure.type).toBe("image/png");
// Apply similar changes for other enclosure assertions
});Committable suggestion was skipped due to low confidence.
| if (missingDatePosts.length > 0) { | ||
| return Promise.reject(new Error('Missing date in post data')); | ||
| } |
There was a problem hiding this comment.
Use throw instead of return Promise.reject in async functions
In an async function, using throw is the standard way to reject a promise. Replacing return Promise.reject(...) with throw ensures proper error handling and stack traces.
Apply this change:
if (missingDatePosts.length > 0) {
- return Promise.reject(new Error('Missing date in post data'));
+ throw new Error('Missing date in post data');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (missingDatePosts.length > 0) { | |
| return Promise.reject(new Error('Missing date in post data')); | |
| } | |
| if (missingDatePosts.length > 0) { | |
| throw new Error('Missing date in post data'); | |
| } |
| } catch (err) { | ||
| return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | ||
| } |
There was a problem hiding this comment.
Use throw instead of return Promise.reject in async functions
In the catch block, you should throw the error to reject the promise properly. This maintains consistency in error handling throughout the function.
Apply this change:
} catch (err) {
- return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`));
+ throw new Error(`Failed to generate RSS feed: ${err.message}`);
}📝 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.
| } catch (err) { | |
| return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | |
| } | |
| } catch (err) { | |
| throw new Error(`Failed to generate RSS feed: ${err.message}`); | |
| } |
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | ||
| return Promise.reject(new Error('Missing required fields in post data')); | ||
| } |
There was a problem hiding this comment.
Use throw instead of return Promise.reject in async functions
Inside the async function, throwing an error directly is preferred over returning Promise.reject. This approach leads to cleaner code and better error propagation.
Apply this change:
if (!post.title || !post.slug || !post.excerpt || !post.date) {
- return Promise.reject(new Error('Missing required fields in post data'));
+ throw new Error('Missing required fields in post data');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | |
| return Promise.reject(new Error('Missing required fields in post data')); | |
| } | |
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | |
| throw new Error('Missing required fields in post data'); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- scripts/build-rss.js (2 hunks)
- tests/build-rss.test.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/build-rss.test.js
🧰 Additional context used
🔇 Additional comments (1)
scripts/build-rss.js (1)
32-34:⚠️ Potential issueReplace Promise.reject with throw
As noted in previous reviews, in async functions, use
throwinstead ofPromise.reject:if (missingDatePosts.length > 0) { - return Promise.reject(new Error('Missing date in post data')); + throw new Error('Missing date in post data'); }Likely invalid or redundant comment.
| const base = 'https://www.asyncapi.com' | ||
| const tracking = '?utm_source=rss'; | ||
|
|
||
| const feed = {} | ||
| const rss = {} | ||
| rss['@version'] = '2.0' | ||
| rss["@xmlns:atom"] = 'http://www.w3.org/2005/Atom' | ||
| rss.channel = {} | ||
| rss.channel.title = title | ||
| rss.channel.link = `${base}/${outputPath}` | ||
| rss.channel["atom:link"] = {} | ||
| rss.channel["atom:link"]["@rel"] = 'self' | ||
| rss.channel["atom:link"]["@href"] = rss.channel.link | ||
| rss.channel["atom:link"]["@type"] = 'application/rss+xml' | ||
| rss.channel.description = desc | ||
| rss.channel.language = 'en-gb'; | ||
| rss.channel.copyright = 'Made with :love: by the AsyncAPI Initiative.'; | ||
| rss.channel.webMaster = '[email protected] (AsyncAPI Initiative)' | ||
| rss.channel.pubDate = new Date().toUTCString() | ||
| rss.channel.generator = 'next.js' | ||
| rss.channel.item = [] | ||
| const feed = {} | ||
| const rss = {} | ||
| rss['@version'] = '2.0' | ||
| rss["@xmlns:atom"] = 'http://www.w3.org/2005/Atom' | ||
| rss.channel = {} | ||
| rss.channel.title = title | ||
| rss.channel.link = `${base}/${outputPath}` | ||
| rss.channel["atom:link"] = {} | ||
| rss.channel["atom:link"]["@rel"] = 'self' | ||
| rss.channel["atom:link"]["@href"] = rss.channel.link | ||
| rss.channel["atom:link"]["@type"] = 'application/rss+xml' | ||
| rss.channel.description = desc | ||
| rss.channel.language = 'en-gb'; | ||
| rss.channel.copyright = 'Made with :love: by the AsyncAPI Initiative.'; | ||
| rss.channel.webMaster = '[email protected] (AsyncAPI Initiative)' | ||
| rss.channel.pubDate = new Date().toUTCString() | ||
| rss.channel.generator = 'next.js' | ||
| rss.channel.item = [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider externalizing configuration values
Move hardcoded values to a configuration file for better maintainability:
// config/rss.js
module.exports = {
baseUrl: 'https://www.asyncapi.com',
tracking: '?utm_source=rss',
language: 'en-gb',
copyright: 'Made with :love: by the AsyncAPI Initiative.',
webMaster: '[email protected] (AsyncAPI Initiative)',
generator: 'next.js'
};This would make the configuration more maintainable and reusable.
| for (let post of posts) { | ||
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | ||
| return Promise.reject(new Error('Missing required fields in post data')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move validation before the loop
The current validation inside the loop will process some items before potentially failing. Move the validation before the loop for better efficiency:
+const invalidPosts = posts.filter(post =>
+ !post.title || !post.slug || !post.excerpt || !post.date
+);
+if (invalidPosts.length > 0) {
+ throw new Error('Missing required fields in post data');
+}
for (let post of posts) {
- if (!post.title || !post.slug || !post.excerpt || !post.date) {
- return Promise.reject(new Error('Missing required fields in post data'));
- }📝 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.
| for (let post of posts) { | |
| if (!post.title || !post.slug || !post.excerpt || !post.date) { | |
| return Promise.reject(new Error('Missing required fields in post data')); | |
| } | |
| const invalidPosts = posts.filter(post => | |
| !post.title || !post.slug || !post.excerpt || !post.date | |
| ); | |
| if (invalidPosts.length > 0) { | |
| throw new Error('Missing required fields in post data'); | |
| } | |
| for (let post of posts) { |
| if (post.cover) { | ||
| const enclosure = {}; | ||
| enclosure["@url"] = base + post.cover; | ||
| enclosure["@length"] = 15026; // dummy value, anything works | ||
| enclosure["@type"] = 'image/jpeg'; | ||
| if (typeof enclosure["@url"] === 'string') { | ||
| let tmp = enclosure["@url"].toLowerCase(); | ||
| if (tmp.indexOf('.png') >= 0) enclosure["@type"] = 'image/png'; | ||
| if (tmp.indexOf('.svg') >= 0) enclosure["@type"] = 'image/svg+xml'; | ||
| if (tmp.indexOf('.webp') >= 0) enclosure["@type"] = 'image/webp'; | ||
| } | ||
| item.enclosure = enclosure; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use mime-types package for better image type detection
Replace manual image type detection with the mime-types package:
+const mime = require('mime-types');
if (post.cover) {
const enclosure = {};
enclosure["@url"] = base + post.cover;
enclosure["@length"] = 15026;
- enclosure["@type"] = 'image/jpeg';
- if (typeof enclosure["@url"] === 'string') {
- let tmp = enclosure["@url"].toLowerCase();
- if (tmp.indexOf('.png') >= 0) enclosure["@type"] = 'image/png';
- if (tmp.indexOf('.svg') >= 0) enclosure["@type"] = 'image/svg+xml';
- if (tmp.indexOf('.webp') >= 0) enclosure["@type"] = 'image/webp';
- }
+ enclosure["@type"] = mime.lookup(post.cover) || 'image/jpeg';Committable suggestion was skipped due to low confidence.
| const xml = json2xml.getXml(feed, '@', '', 2); | ||
| await fs.writeFile(`./public/${outputPath}`, xml, 'utf8'); | ||
| } catch (err) { | ||
| return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | ||
| } |
There was a problem hiding this comment.
Improve error handling for XML generation
Add specific error handling for XML generation and replace Promise.reject with throw:
- const xml = json2xml.getXml(feed, '@', '', 2);
+ let xml;
+ try {
+ xml = json2xml.getXml(feed, '@', '', 2);
+ } catch (xmlError) {
+ throw new Error(`Failed to generate XML: ${xmlError.message}`);
+ }
await fs.writeFile(`./public/${outputPath}`, xml, 'utf8');
} catch (err) {
- return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`));
+ throw new Error(`Failed to generate RSS feed: ${err.message}`);
}📝 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.
| const xml = json2xml.getXml(feed, '@', '', 2); | |
| await fs.writeFile(`./public/${outputPath}`, xml, 'utf8'); | |
| } catch (err) { | |
| return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | |
| } | |
| let xml; | |
| try { | |
| xml = json2xml.getXml(feed, '@', '', 2); | |
| } catch (xmlError) { | |
| throw new Error(`Failed to generate XML: ${xmlError.message}`); | |
| } | |
| await fs.writeFile(`./public/${outputPath}`, xml, 'utf8'); | |
| } catch (err) { | |
| throw new Error(`Failed to generate RSS feed: ${err.message}`); | |
| } |
| function getAllPosts() { | ||
| return require('../config/posts.json') | ||
| return require('../config/posts.json'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making getAllPosts() asynchronous
Since the function has been converted to use async/await, consider making getAllPosts() asynchronous to avoid blocking operations:
-function getAllPosts() {
- return require('../config/posts.json');
+async function getAllPosts() {
+ const posts = await fs.readFile('../config/posts.json', 'utf8');
+ return JSON.parse(posts);
}Committable suggestion was skipped due to low confidence.
This PR adds test for build-rss.js script
Summary by CodeRabbit
New Features
Bug Fixes