-
-
Notifications
You must be signed in to change notification settings - Fork 637
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: added test for build-rss.js #3101
base: master
Are you sure you want to change the base?
Changes from 60 commits
63e8bf5
c2570b9
30f1c7f
aad7bf2
312d4c5
8d7269a
cfd547f
5859c79
44c3b63
de5edec
bf10b10
1bacff6
3ce90ca
5d8fb67
198c2a4
5f4d05b
5ab37aa
81b9845
92e86a3
0d0cb30
d7e0ac1
fc57c2a
1a8aa47
5ddb184
5e8c0c7
1eb6790
b03191d
239e65b
5e98d50
45f6ef6
e6258c5
7c9730f
6e9f039
aac6e13
0f61abe
e06fa47
bc67c28
8c93997
6e0b4ce
2631dc6
a66f2a2
a51c362
4eb10e3
4e75b0a
5892f1e
e04cfba
07b2344
d55f553
d9585fe
d41b851
d5b6882
7980458
6a93919
a5c083b
49f308a
a4903ef
1ab85ee
36e7511
1d39693
8a55fa2
2ee9f6f
6804d6e
fd0bb07
17fd825
8f77c30
a3dfa93
968d071
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,8 +1,8 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const fs = require('fs') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const fs = require('fs').promises | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const json2xml = require('jgexml/json2xml') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
function getAllPosts() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return require('../config/posts.json') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return require('../config/posts.json'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
function clean(s) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -15,61 +15,85 @@ function clean(s) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
return s | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
module.exports = function rssFeed(type, title, desc, outputPath) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
module.exports = async function rssFeed(type, title, desc, outputPath) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const posts = getAllPosts()[`${type}`] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
.sort((i1, i2) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const i1Date = new Date(i1.date) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const i2Date = new Date(i2.date) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
let posts = getAllPosts()[`${type}`] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const missingDatePosts = posts.filter(post => !post.date); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
posts = posts.filter(post => post.date); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (i1.featured && !i2.featured) return -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!i1.featured && i2.featured) return 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return i2Date - i1Date | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (missingDatePosts.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Promise.reject(new Error('Missing date in post data')); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+32
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use In an async function, using 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+36
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vishvamsinh28 Ignore this suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Promise.reject(new Error('Missing required fields in post data')); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const link = `${base}${post.slug}${tracking}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use Inside the async function, throwing an error directly is preferred over returning 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
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; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+78
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use mime-types package for better image type detection Replace manual image type detection with the +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';
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vishvamsinh28 Ignore this suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+78
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve image type detection The current image type detection could be enhanced to handle more image formats and be more maintainable. Consider this improvement: 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';
- }
+ const imageTypes = {
+ '.jpg': 'image/jpeg',
+ '.jpeg': 'image/jpeg',
+ '.png': 'image/png',
+ '.svg': 'image/svg+xml',
+ '.webp': 'image/webp'
+ };
+ const ext = post.cover.toLowerCase().match(/\.[^.]+$/)?.[0];
+ enclosure["@type"] = imageTypes[ext] || 'image/jpeg'; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
item.enclosure = enclosure; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
rss.channel.item.push(item) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
await fs.writeFile(`./public/${outputPath}`, xml, 'utf8'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return `RSS feed generated successfully at ${outputPath}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Promise.reject(new Error(`Failed to generate RSS feed: ${err.message}`)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+98
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use In the 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
Suggested change
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, add the scenarios where the function should actually lead to an error, instead of executing it perfectly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vishvamsinh28 @anshgoyalevil Comment not addressed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll add more tests for it. 👍 I can add a test to check if the user doesn't have a posts.json file, but for that, I'll have to temporarily rename the file. Should I add that test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, consider a scenario if data inside posts.json is in wrong manner. Then it should return error, right? Make the tests for those cases as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akshatnema updated the test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,187 @@ | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
const rssFeed = require('../scripts/build-rss'); | ||
const { mockRssData, title, type, desc, missingDateMockData, incompletePostMockData } = require('./fixtures/rssData'); | ||
|
||
describe('rssFeed', () => { | ||
const testOutputDir = path.join(__dirname, '..', 'public', 'test-output'); | ||
const outputPath = 'test-output/rss.xml'; | ||
|
||
beforeAll(() => { | ||
if (!fs.existsSync(testOutputDir)) { | ||
fs.mkdirSync(testOutputDir, { recursive: true }); | ||
} | ||
}); | ||
|
||
afterAll(() => { | ||
if (fs.existsSync(testOutputDir)) { | ||
fs.readdirSync(testOutputDir).forEach(file => { | ||
fs.unlinkSync(path.join(testOutputDir, file)); | ||
}); | ||
fs.rmdirSync(testOutputDir); | ||
} | ||
}); | ||
|
||
afterEach(() => { | ||
jest.resetModules(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vishvamsinh28 any specific reason on why are we resetting all modules instead of mocks here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}); | ||
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Reset mocks instead of modules in Instead of using Refactored code: afterEach(() => {
jest.resetAllMocks();
}); |
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||
|
||
it('should prioritize featured posts over non-featured ones', 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 | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||
|
||
it('should sort posts by date in descending order', 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 | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 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. |
||
|
||
it('should set correct enclosure type based on image extension', 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 | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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
});
|
||
}); | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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 |
||
|
||
it('should throw an error when posts.json is malformed', async () => { | ||
jest.doMock('../config/posts.json', () => { | ||
return { 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'); | ||
} | ||
}); | ||
|
||
it('should handle empty posts array', async () => { | ||
const emptyMockData = { blog: [] }; | ||
jest.doMock('../config/posts.json', () => emptyMockData, { 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); | ||
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 expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Missing required fields'); | ||
} 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 expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Missing date in post data'); | ||
} catch (err) { | ||
error = err; | ||
expect(error).toBeDefined(); | ||
expect(error.message).toContain('Missing date in post data'); | ||
} | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider making
getAllPosts()
asynchronousSince the function has been converted to use async/await, consider making
getAllPosts()
asynchronous to avoid blocking operations: