Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

feat: added test for build-rss.js #3101

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 55 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
63e8bf5
added test for build-rss.js
vishvamsinh28 Jul 21, 2024
c2570b9
Merge branch 'master' into rssTest
asyncapi-bot Aug 11, 2024
30f1c7f
Merge branch 'master' into rssTest
vishvamsinh28 Aug 16, 2024
aad7bf2
test updated
vishvamsinh28 Aug 16, 2024
312d4c5
Merge branch 'master' into rssTest
anshgoyalevil Aug 18, 2024
8d7269a
Merge branch 'master' into rssTest
vishvamsinh28 Aug 22, 2024
cfd547f
fewe
vishvamsinh28 Aug 22, 2024
5859c79
fedge
vishvamsinh28 Aug 22, 2024
44c3b63
fsfa
vishvamsinh28 Aug 22, 2024
de5edec
test for write operation error
vishvamsinh28 Aug 22, 2024
bf10b10
test updated
vishvamsinh28 Aug 22, 2024
1bacff6
egrwsdb
vishvamsinh28 Aug 22, 2024
3ce90ca
Merge branch 'master' into rssTest
vishvamsinh28 Aug 26, 2024
5d8fb67
use pretteir for rssData
vishvamsinh28 Aug 31, 2024
198c2a4
Merge branch 'master' into rssTest
anshgoyalevil Sep 5, 2024
5f4d05b
Merge branch 'master' into rssTest
asyncapi-bot Sep 5, 2024
5ab37aa
Merge branch 'master' into rssTest
vishvamsinh28 Sep 7, 2024
81b9845
Merge branch 'master' into rssTest
vishvamsinh28 Sep 11, 2024
92e86a3
Merge branch 'rssTest' of https://github.com/vishvamsinh28/website in…
vishvamsinh28 Sep 11, 2024
0d0cb30
updated rss mock data
vishvamsinh28 Sep 11, 2024
d7e0ac1
updated rss test
vishvamsinh28 Sep 11, 2024
fc57c2a
udadw
vishvamsinh28 Sep 11, 2024
1a8aa47
updated imports
vishvamsinh28 Sep 11, 2024
5ddb184
test updated
vishvamsinh28 Sep 11, 2024
5e8c0c7
Merge branch 'master' into rssTest
anshgoyalevil Sep 13, 2024
1eb6790
added try catch block for tests
vishvamsinh28 Sep 13, 2024
b03191d
Merge branch 'master' into rssTest
vishvamsinh28 Sep 15, 2024
239e65b
Merge branch 'master' into rssTest
vishvamsinh28 Sep 16, 2024
5e98d50
removed try catch block
vishvamsinh28 Sep 16, 2024
45f6ef6
Merge branch 'rssTest' of https://github.com/vishvamsinh28/website in…
vishvamsinh28 Sep 16, 2024
e6258c5
replaced trown with console.error
vishvamsinh28 Sep 16, 2024
7c9730f
using promise for throwign errors
vishvamsinh28 Sep 16, 2024
6e9f039
add error variable
vishvamsinh28 Sep 16, 2024
aac6e13
updated the test for rss
vishvamsinh28 Sep 16, 2024
0f61abe
Merge branch 'master' into rssTest
vishvamsinh28 Sep 20, 2024
e06fa47
updated test
vishvamsinh28 Sep 20, 2024
bc67c28
Merge branch 'master' into rssTest
anshgoyalevil Sep 23, 2024
8c93997
fwwfwf
vishvamsinh28 Sep 23, 2024
6e0b4ce
revert getAllPosts func changes
anshgoyalevil Sep 23, 2024
2631dc6
Merge branch 'master' into rssTest
vishvamsinh28 Sep 26, 2024
a66f2a2
Merge branch 'master' into rssTest
vishvamsinh28 Sep 28, 2024
a51c362
wsff
vishvamsinh28 Sep 28, 2024
4eb10e3
fefe
vishvamsinh28 Sep 28, 2024
4e75b0a
added test cases for errors
vishvamsinh28 Sep 28, 2024
5892f1e
Merge branch 'master' into rssTest
vishvamsinh28 Sep 30, 2024
e04cfba
Merge branch 'master' into rssTest
vishvamsinh28 Oct 4, 2024
07b2344
Merge branch 'master' into rssTest
vishvamsinh28 Oct 7, 2024
d55f553
suggested changes applied
vishvamsinh28 Oct 7, 2024
d9585fe
Merge branch 'master' into rssTest
vishvamsinh28 Oct 12, 2024
d41b851
Merge branch 'master' into rssTest
vishvamsinh28 Oct 13, 2024
d5b6882
empty line at end
vishvamsinh28 Oct 13, 2024
7980458
Merge branch 'rssTest' of https://github.com/vishvamsinh28/website in…
vishvamsinh28 Oct 13, 2024
6a93919
Merge branch 'master' into rssTest
vishvamsinh28 Oct 14, 2024
a5c083b
Merge branch 'master' into rssTest
vishvamsinh28 Oct 15, 2024
49f308a
replaced throw with promise.reject
vishvamsinh28 Oct 17, 2024
a4903ef
Merge branch 'master' into rssTest
vishvamsinh28 Oct 17, 2024
1ab85ee
Merge branch 'master' into rssTest
vishvamsinh28 Oct 20, 2024
36e7511
Merge branch 'master' into rssTest
anshgoyalevil Oct 23, 2024
1d39693
Merge branch 'master' into rssTest
vishvamsinh28 Oct 23, 2024
8a55fa2
test updated
vishvamsinh28 Oct 23, 2024
2ee9f6f
test updted
vishvamsinh28 Oct 23, 2024
6804d6e
rwwq
vishvamsinh28 Oct 23, 2024
fd0bb07
removed try/catch
vishvamsinh28 Oct 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 69 additions & 45 deletions scripts/build-rss.js
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');
}

Comment on lines 4 to 6
Copy link

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() 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.

function clean(s) {
Expand All @@ -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) => {
let posts = getAllPosts()[`${type}`]
posts = posts.filter(post => {
if (!post.date) {
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
})

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
Copy link

Choose a reason for hiding this comment

The 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.

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}`;
Comment on lines +59 to +61
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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');
}

Comment on lines +58 to +61
Copy link

Choose a reason for hiding this comment

The 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

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

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

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 +75 to +86
Copy link

Choose a reason for hiding this comment

The 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 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.

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 +95 to +97
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
} 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}`);
}

186 changes: 186 additions & 0 deletions tests/build-rss.test.js
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

@vishvamsinh28 @anshgoyalevil Comment not addressed

Copy link
Contributor Author

@vishvamsinh28 vishvamsinh28 Sep 28, 2024

Choose a reason for hiding this comment

The 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?

Copy link
Member

@akshatnema akshatnema Sep 28, 2024

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,186 @@
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();
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

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

@coderabbitai coderabbitai bot Oct 13, 2024

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@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 the rssFeed function 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 rssFeed throws 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.


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
}

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify asynchronous tests using Jest's resolves

Similar to the previous test, you can remove the try/catch block and use Jest's expect().resolves for better readability.

Refactored code:

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

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

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

The 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 rssFeed(type, title, desc, outputPath);
} catch (err) {
error = err
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify asynchronous tests using Jest's resolves

To enhance clarity, replace the try/catch block with expect().resolves.

Refactored code:

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

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

Choose a reason for hiding this comment

The 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 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.

Copy link

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify asynchronous tests using Jest's resolves

For consistency and readability, use expect().resolves instead of the try/catch pattern.

Refactored code:

await expect(rssFeed(type, title, desc, outputPath)).resolves.not.toThrow();


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

Choose a reason for hiding this comment

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

Committable suggestion was skipped due to low confidence.

});
Copy link

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Jest's rejects.toThrow for expected errors

When testing for expected errors, expect().rejects.toThrow() provides a cleaner approach than using a try/catch block.

Refactored code:

await expect(rssFeed(type, title, desc, invalidOutputPath)).rejects.toThrow(/ENOENT|EACCES/);

This directly asserts that the Promise rejects with an error matching the specified pattern.

});
Copy link

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Jest's rejects.toThrow for expected errors

Streamline your error handling in tests by using expect().rejects.toThrow().

Refactored code:

await expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow('Failed to generate RSS feed');

This makes the test more concise and declarative.

});

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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

});

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

Choose a reason for hiding this comment

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

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

});
Copy link

Choose a reason for hiding this comment

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

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

  1. Use expect().resolves or expect().rejects for asynchronous assertions.
  2. 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.

});
Loading
Loading