feat: added tests for build-newsroom-videos.js #3075
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-3075--asyncapi-website.netlify.app/ |
anshgoyalevil
added
the
gsoc
|
@anshgoyalevil i should not mock fs module in this PR as well correct? I will update it |
|
Yes. Kindly use actual fs module. |
|
@anshgoyalevil okay will update it 👍 |
|
@vishvamsinh28 can you please check for conflicts once |
|
@sambhavgupta0705 fixed it |
| @@ -1,10 +1,9 @@ | |||
| const { writeFileSync } = require('fs'); | |||
| const { resolve } = require('path'); | |||
| const fetch = require('node-fetch-2') | |||
There was a problem hiding this comment.
revert this one
For now we are trying fixing this
scripts/build-newsroom-videos.js
Outdated
| })); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! status: ${response.status}`); |
There was a problem hiding this comment.
| throw new Error(`HTTP error! status: ${response.status}`); | |
| throw new Error(`HTTP error! with status code: ${response.status}`); |
tests/build-newsroom-videos.test.js
Outdated
| describe('buildNewsroomVideos', () => { | ||
| beforeEach(() => { | ||
| fetch.mockClear(); | ||
| process.env.YOUTUBE_TOKEN = 'testkey'; |
There was a problem hiding this comment.
this is hardcoded for testing only?
tests/build-newsroom-videos.test.js
Outdated
| const testDir = resolve(__dirname, 'test_config'); | ||
| const testFilePath = resolve(testDir, 'newsroom_videos.json'); | ||
|
|
||
| jest.mock('node-fetch', () => jest.fn()); |
There was a problem hiding this comment.
need to see will this node-fetch work or not
There was a problem hiding this comment.
The test is working because we are not actually calling it, but instead we are mocking it.
scripts/build-newsroom-videos.js
Outdated
| module.exports = { buildNewsroomVideos }; |
There was a problem hiding this comment.
add a newline to avoid lint error
tests/fixtures/newsroomData.js
Outdated
| }, | ||
| ], null, ' '); | ||
|
|
||
| module.exports = {mockApiResponse,expectedResult} |
|
@sambhavgupta0705 @anshgoyalevil Updated the test |
| } | ||
| } | ||
|
|
||
| buildNewsroomVideos() |
There was a problem hiding this comment.
Removed this function call?
There was a problem hiding this comment.
Maybe by mistake while updating the script, will add it again 👍👍
|
We can aim for 100% statement and branch coverage here.
|
scripts/build-newsroom-videos.js
Outdated
|
|
||
| async function buildNewsroomVideos(writePath = resolve(__dirname, '../config', 'newsroom_videos.json')) { |
There was a problem hiding this comment.
You are initializing writhPath here. This would make this branch uncovered, like you saw in the coverage report.
One solution would be to pass this thing as an argument while calling this function, like we did in
Lines 16 to 18 in 1ab6e4a
|
@anshgoyalevil @sambhavgupta0705 updated the test |
|
@anshgoyalevil @sambhavgupta0705 updated the test |
scripts/build-newsroom-videos.js
Outdated
|
|
||
| async function buildNewsroomVideos(writePath) { | ||
| const response = await fetch('https://youtube.googleapis.com/youtube/v3/search?' + new URLSearchParams({ | ||
| key: process.env.YOUTUBE_TOKEN, | ||
| part: 'snippet', | ||
| channelId: 'UCIz9zGwDLbrYQcDKVXdOstQ', | ||
| eventType: 'completed', | ||
| type: 'video', | ||
| order: 'Date', | ||
| maxResults: 5, | ||
| })); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! with status code: ${response.status}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| console.log(data) | ||
|
|
||
| if (!data.items || !Array.isArray(data.items)) { | ||
| throw new Error('Invalid data structure received from YouTube API'); |
There was a problem hiding this comment.
Why have you removed the try catch blocks? Error handling should be in place.
tests/build-newsroom-videos.test.js
Outdated
| it('should throw an error with incorrect parameters', async () => { | ||
| await expect(buildNewsroomVideos('randomPath')).rejects.toThrow('HTTP error! with status code: 404'); | ||
| }); |
There was a problem hiding this comment.
This test can be improved since the error you are throwing here isn't related to the randomPath thing. That's basically an issue with creating a file named randomPath, and isn't related to the HTTP call.
tests/build-newsroom-videos.test.js
Outdated
|
|
||
| const result = await buildNewsroomVideos(testFilePath); | ||
|
|
||
| expect(fetch).toHaveBeenCalledWith(expect.stringContaining('https://youtube.googleapis.com/youtube/v3/search?')); |
There was a problem hiding this comment.
There are other query parameters. Consider testing them also.
For example:
const expectedUrl = new URL('https://youtube.googleapis.com/youtube/v3/search');
expectedUrl.searchParams.set('key', 'testkey');
expectedUrl.searchParams.set('eventType', 'completed');
...etc
expect(fetch).toHaveBeenCalledWith(expectedUrl.toString());
|
@anshgoyalevil Updated the test 
This part of the script is not covered in the test because adding a test for it would cause the test to fail. This is due to the absence of an API key locally, which would result in errors because it will call actual function from the script. Additionally, running this test during PR checks could potentially modify the newsroom.json file, as the function would be called each time the tests are executed. |
|
/rtm |
This PR adds tests for build-newsroom-videos.js