fix: consolidates utility functions by merging into a single utils.ts file #3996
Conversation
…ut cleanup in the processBatch function in check-edit-links.ts
Siddhant-Pragyan-Sinha
requested review from
derberg,
akshatnema,
anshgoyalevil,
sambhavgupta0705,
Mayaleeeee and
asyncapi-bot-eve
as code owners
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request consolidates several utility functions by updating import paths across various script and test files. Specifically, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Utils as Utils Module (writeJSON)
participant FS as File System
Caller->>Utils: call writeJSON(readPath, writePath)
Utils->>FS: readFile(readPath)
FS-->>Utils: return file content
Utils->>Utils: convertToJson(content)
Utils->>FS: writeFile(writePath, JSON content)
FS-->>Utils: acknowledge write completion
Utils-->>Caller: return result (or handle error)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
tests/adopters/index.test.js (1)
6-6: Remove extra newline.There's an extra blank line here that should be removed according to the linting rules.
- jest.mock('../../scripts/utils.ts', () => ({🧰 Tools
🪛 ESLint
[error] 6-7: Delete
⏎(prettier/prettier)
tests/markdown/check-markdown.test.js (1)
14-14: Remove extra newline.There's an extra blank line here that should be removed according to the linting rules.
- jest.mock('../../scripts/utils', () => ({🧰 Tools
🪛 ESLint
[error] 14-15: Delete
⏎(prettier/prettier)
tests/readAndWriteJson.test.js (5)
34-41: Fix parameter shadowing and whitespace issues.The parameter names in the mock implementation shadow variables from the outer scope, which can lead to confusion and potential bugs. Also, there's extra whitespace that should be removed.
- fs.writeFile.mockResolvedValue(); - - // Setup the mock implementation for writeJSON for this specific test - writeJSON.mockImplementation(async (readPath, writePath) => { + fs.writeFile.mockResolvedValue(); + + // Setup the mock implementation for writeJSON for this specific test + writeJSON.mockImplementation(async (rPath, wPath) => { - const content = await fs.readFile(readPath, 'utf-8'); + const content = await fs.readFile(rPath, 'utf-8'); const jsonData = convertToJson(content); - await fs.writeFile(writePath, JSON.stringify(jsonData)); + await fs.writeFile(wPath, JSON.stringify(jsonData)); return jsonData; });🧰 Tools
🪛 ESLint
[error] 34-34: Delete
····(prettier/prettier)
[error] 36-36: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 36-36: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
53-62: Fix parameter shadowing and whitespace issues.The parameter names in the mock implementation shadow variables from the outer scope. Also, there's extra whitespace that should be removed.
- fs.readFile.mockRejectedValue(error); - - // Setup the mock implementation for writeJSON for this specific test - writeJSON.mockImplementation(async (readPath, writePath) => { + fs.readFile.mockRejectedValue(error); + + // Setup the mock implementation for writeJSON for this specific test + writeJSON.mockImplementation(async (rPath) => { try { - await fs.readFile(readPath, 'utf-8'); + await fs.readFile(rPath, 'utf-8'); // This shouldn't happen in this test } catch (err) { throw new Error(`Error while reading file\nError: ${err}`); } });🧰 Tools
🪛 ESLint
[error] 53-53: Delete
····(prettier/prettier)
[error] 55-55: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 55-55: 'writePath' is defined but never used.
(no-unused-vars)
[error] 55-55: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
75-86: Fix parameter shadowing and whitespace issues.The parameter names in the mock implementation shadow variables from the outer scope. Also, there's extra whitespace that should be removed.
- convertToJson.mockImplementation(() => { - throw error; - }); - - // Setup the mock implementation for writeJSON for this specific test - writeJSON.mockImplementation(async (readPath, writePath) => { + convertToJson.mockImplementation(() => { + throw error; + }); + + // Setup the mock implementation for writeJSON for this specific test + writeJSON.mockImplementation(async (rPath, wPath) => { try { - const content = await fs.readFile(readPath, 'utf-8'); + const content = await fs.readFile(rPath, 'utf-8'); const jsonData = convertToJson(content); - await fs.writeFile(writePath, JSON.stringify(jsonData)); + await fs.writeFile(wPath, JSON.stringify(jsonData)); return jsonData; } catch (err) { throw new Error(`Error while conversion\nError: ${err}`); } });🧰 Tools
🪛 ESLint
[error] 75-75: Delete
····(prettier/prettier)
[error] 77-77: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 77-77: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
98-109: Fix parameter shadowing and whitespace issues.The parameter names in the mock implementation shadow variables from the outer scope. Also, there's extra whitespace that should be removed.
- fs.writeFile.mockRejectedValue(error); - - // Setup the mock implementation for writeJSON for this specific test - writeJSON.mockImplementation(async (readPath, writePath) => { + fs.writeFile.mockRejectedValue(error); + + // Setup the mock implementation for writeJSON for this specific test + writeJSON.mockImplementation(async (rPath, wPath) => { try { - const content = await fs.readFile(readPath, 'utf-8'); + const content = await fs.readFile(rPath, 'utf-8'); const jsonData = convertToJson(content); - await fs.writeFile(writePath, JSON.stringify(jsonData)); + await fs.writeFile(wPath, JSON.stringify(jsonData)); return jsonData; } catch (err) { throw new Error(`Error while writing file\nError: ${err}`); } });🧰 Tools
🪛 ESLint
[error] 98-98: Delete
····(prettier/prettier)
[error] 100-100: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 100-100: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
1-115: Consider refactoring test mocks for improved maintainability.Each test case reimplements very similar mock implementations for
writeJSON. Consider creating a helper function to generate these mocks based on which stage should fail (read, convert, or write) to reduce duplication and improve maintainability.🧰 Tools
🪛 ESLint
[error] 34-34: Delete
····(prettier/prettier)
[error] 36-36: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 36-36: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 53-53: Delete
····(prettier/prettier)
[error] 55-55: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 55-55: 'writePath' is defined but never used.
(no-unused-vars)
[error] 55-55: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 75-75: Delete
····(prettier/prettier)
[error] 77-77: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 77-77: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 98-98: Delete
····(prettier/prettier)
[error] 100-100: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 100-100: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
tests/tools/tools-object.test.js (1)
15-15: Fix formatting issue with arrow function.According to the linting rules, the parameter should be wrapped in parentheses.
- convertToJson: jest.fn(content => content) + convertToJson: jest.fn((content) => content)🧰 Tools
🪛 ESLint
[error] 15-15: Replace
contentwith(content)(prettier/prettier)
tests/markdown/check-edit-links.test.js (3)
58-60: Fix formatting issues.There are some formatting inconsistencies in this block according to your project's Prettier configuration.
- const noIgnoredFiles = localTestPaths.every((testPath) => - !ignoreFiles.some((ignorePath) => testPath.filePath.endsWith(ignorePath)) - ); + const noIgnoredFiles = localTestPaths.every((testPath) => + !ignoreFiles.some((ignorePath) => testPath.filePath.endsWith(ignorePath)) + );🧰 Tools
🪛 ESLint
[error] 58-59: Replace
(testPath)·=>·⏎···with⏎····(testPath)·=>(prettier/prettier)
171-201: Improved timeout testing with AbortController.The refactored test now uses AbortController and Jest fake timers for more reliable timeout testing, addressing the PR objective of improving test reliability and avoiding hanging processes.
However, there are some formatting inconsistencies with trailing whitespace:
- // Mock AbortController - const mockAbort = jest.fn(); - const mockAbortController = { - signal: 'mock-signal', - abort: mockAbort - }; - - global.AbortController = jest.fn(() => mockAbortController); - + // Mock AbortController + const mockAbort = jest.fn(); + const mockAbortController = { + signal: 'mock-signal', + abort: mockAbort + }; + + global.AbortController = jest.fn(() => mockAbortController); +🧰 Tools
🪛 ESLint
[error] 177-177: Delete
······(prettier/prettier)
[error] 179-179: Delete
······(prettier/prettier)
[error] 186-186: Delete
······(prettier/prettier)
[error] 189-189: Delete
······(prettier/prettier)
[error] 192-192: Delete
······(prettier/prettier)
[error] 195-195: Delete
······(prettier/prettier)
[error] 198-198: Delete
······(prettier/prettier)
206-220: Enhanced test assertions for comprehensive validation.Adding validations to ensure all test paths have edit links and none are in the ignore list improves test robustness. The updated fetch call count expectation ensures the test correctly accounts for the actual number of test paths.
Also here, there are formatting inconsistencies that could be addressed:
- // Make sure all test paths have editLinks - const allPathsHaveEditLinks = testPaths.every((testPath) => testPath.editLink); - expect(allPathsHaveEditLinks).toBe(true); - - // Verify that none of the test paths are in the ignoreFiles list - expect(noIgnoredFiles).toBe(true); - + // Make sure all test paths have editLinks + const allPathsHaveEditLinks = testPaths.every((testPath) => testPath.editLink); + expect(allPathsHaveEditLinks).toBe(true); + + // Verify that none of the test paths are in the ignoreFiles list + expect(noIgnoredFiles).toBe(true); +🧰 Tools
🪛 ESLint
[error] 209-209: Delete
······(prettier/prettier)
[error] 212-212: Delete
······(prettier/prettier)
[error] 216-216: Delete
······(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/adopters/index.test.js(1 hunks)tests/markdown/check-edit-links.test.js(4 hunks)tests/markdown/check-markdown.test.js(2 hunks)tests/readAndWriteJson.test.js(3 hunks)tests/tools/combine-tools.test.js(2 hunks)tests/tools/tools-object.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tools/combine-tools.test.js
🧰 Additional context used
🧠 Learnings (1)
tests/markdown/check-edit-links.test.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-03-26T20:25:51.956Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
🧬 Code Definitions (2)
tests/markdown/check-edit-links.test.js (1)
tests/fixtures/markdown/check-edit-links-data.js (1)
testPaths(32-83)
tests/readAndWriteJson.test.js (1)
scripts/utils.ts (2)
convertToJson(15-38)writeJSON(70-90)
🪛 ESLint
tests/markdown/check-edit-links.test.js
[error] 58-59: Replace (testPath)·=>·⏎··· with ⏎····(testPath)·=>
(prettier/prettier)
[error] 60-61: Delete ⏎
(prettier/prettier)
[error] 177-177: Delete ······
(prettier/prettier)
[error] 179-179: Delete ······
(prettier/prettier)
[error] 186-186: Delete ······
(prettier/prettier)
[error] 189-189: Delete ······
(prettier/prettier)
[error] 192-192: Delete ······
(prettier/prettier)
[error] 195-195: Delete ······
(prettier/prettier)
[error] 198-198: Delete ······
(prettier/prettier)
[error] 209-209: Delete ······
(prettier/prettier)
[error] 212-212: Delete ······
(prettier/prettier)
[error] 216-216: Delete ······
(prettier/prettier)
tests/adopters/index.test.js
[error] 6-7: Delete ⏎
(prettier/prettier)
tests/markdown/check-markdown.test.js
[error] 14-15: Delete ⏎
(prettier/prettier)
tests/readAndWriteJson.test.js
[error] 34-34: Delete ····
(prettier/prettier)
[error] 36-36: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 36-36: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 53-53: Delete ····
(prettier/prettier)
[error] 55-55: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 55-55: 'writePath' is defined but never used.
(no-unused-vars)
[error] 55-55: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 75-75: Delete ····
(prettier/prettier)
[error] 77-77: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 77-77: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
[error] 98-98: Delete ····
(prettier/prettier)
[error] 100-100: 'readPath' is already declared in the upper scope on line 21 column 9.
(no-shadow)
[error] 100-100: 'writePath' is already declared in the upper scope on line 22 column 9.
(no-shadow)
tests/tools/tools-object.test.js
[error] 15-15: Replace content with (content)
(prettier/prettier)
🔇 Additional comments (12)
tests/adopters/index.test.js (1)
1-9: Import changes LGTM!The imports have been successfully updated to use the consolidated
utils.tsfile, and the mock implementation has been properly adjusted to match this new structure.🧰 Tools
🪛 ESLint
[error] 6-7: Delete
⏎(prettier/prettier)
tests/markdown/check-markdown.test.js (2)
1-2: Import changes LGTM!The import has been successfully updated to use the consolidated
utils.tsfile.
15-17: Logger mock implementation correctly updated.The mock for the
utilsmodule has been properly updated to provide the required logger functions.tests/readAndWriteJson.test.js (2)
1-2: Import changes LGTM!The imports have been successfully updated to use the consolidated
utils.tsfile.
13-18: Mock implementation correctly structured.The mock for the
utilsmodule provides the necessary functions for testing.tests/tools/tools-object.test.js (2)
1-2: Import changes LGTM!The import has been successfully updated to use the consolidated
utils.tsfile.
13-16: Mock implementation correctly updated and expanded.The mock for the
utilsmodule has been properly updated and now includes mocking for both the logger and the convertToJson function.🧰 Tools
🪛 ESLint
[error] 15-15: Replace
contentwith(content)(prettier/prettier)
tests/markdown/check-edit-links.test.js (5)
1-18: LGTM on updating import path and extending mocks.The import path update aligns with the PR objective of consolidating utility functions into a single
utils.tsfile. The mock now correctly includes both the logger and the pause function, ensuring test coverage for the consolidated utilities.
23-55: Good implementation of test fixtures with improved naming.The renaming from
testPathstolocalTestPathsaddresses the variable shadowing issue identified in a previous review. The test data structure provides comprehensive coverage of different path patterns.
67-74: Well-implemented test cleanup.The enhanced afterEach block ensures proper test isolation by clearing timers, restoring mocks, and resetting fetch implementation. This directly addresses the PR objective of fixing issues related to hanging processes.
76-78: Good test case for validating test paths.This test verifies that test fixtures are properly configured and won't be accidentally ignored during testing, which ensures test reliability.
230-230: LGTM on updating the URL path check.The update to check for 'migration' in the URL path aligns with the expected response based on the mock implementation in line 225.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/markdown/check-edit-links.test.js (1)
21-33: Added ignoreFiles constant and verification logic.Good addition of constants that match the implementation in the main code. The
noIgnoredFilescheck ensures test paths don't overlap with ignored files.However, there's a formatting issue according to your linting rules:
- const noIgnoredFiles = testPaths.every((testPath) => - !ignoreFiles.some((ignorePath) => testPath.filePath.endsWith(ignorePath)) - ); + const noIgnoredFiles = testPaths.every( + (testPath) => !ignoreFiles.some((ignorePath) => testPath.filePath.endsWith(ignorePath)) + );🧰 Tools
🪛 ESLint
[error] 30-31: Replace
(testPath)·=>⏎···with⏎····(testPath)·=>(prettier/prettier)
[error] 32-33: Delete
⏎(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/markdown/check-edit-links.test.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/markdown/check-edit-links.test.js (1)
Learnt from: anshgoyalevil
PR: asyncapi/website#3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-03-26T20:25:51.956Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
🧬 Code Definitions (1)
tests/markdown/check-edit-links.test.js (2)
tests/fixtures/markdown/check-edit-links-data.js (1)
testPaths(32-83)scripts/markdown/check-edit-links.ts (2)
processBatch(199-199)checkUrls(199-199)
🪛 ESLint
tests/markdown/check-edit-links.test.js
[error] 30-31: Replace (testPath)·=>⏎··· with ⏎····(testPath)·=>
(prettier/prettier)
[error] 32-33: Delete ⏎
(prettier/prettier)
🔇 Additional comments (7)
tests/markdown/check-edit-links.test.js (7)
1-1: Import path updated for logger to use consolidated utils module.This change aligns with the PR objective of consolidating utility functions into a single
utils.tsfile.
15-18: Updated mock to include the pause function.The mock now includes both the logger and the pause function that's now exported from the consolidated utils module. This ensures proper mocking of all utilized functions.
39-46: Enhanced test cleanup in afterEach.Good improvement to the test cleanup process that ensures all timers are cleared, mocks are restored, and fetch mock implementation is reset after each test. This prevents test interference and potential memory leaks.
48-50: Added test to verify no test paths are in the ignoreFiles list.This test ensures that your test paths and ignore list are properly configured, which prevents silent test failures where tests might be skipped unintentionally.
142-173: Improved timeout test using AbortController and fake timers.The test has been significantly improved:
- Now uses AbortController to properly simulate abort operations
- Leverages Jest's fake timers for precise timing control
- Properly restores real timers before assertions
- Clearly asserts the expected error message
This approach is more reliable and provides better isolation than the previous implementation.
178-192: Enhanced URL processing test with additional assertions.Good additions that verify:
- All test paths have editLinks
- None of the test paths are in the ignoreFiles list
- The fetch call count matches the expected number of test paths
These assertions strengthen the test by validating assumptions about the test data.
201-203: Updated expected results for mixed responses.The assertion now properly verifies that the returned failing URLs contain 'migration', which matches the mock implementation on line 197.
…inks.test.js, build-dashboard.test.js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
tests/readAndWriteJson.test.js (5)
33-38: Fix parameter shadowing issue.The parameters
readPathandwritePathin the mockImplementation shadow the same variables declared at lines 18-19, which could lead to confusion.- writeJSON.mockImplementation(async (readPath, writePath) => { + writeJSON.mockImplementation(async (inputPath, outputPath) => { - const content = await fs.readFile(readPath, 'utf-8'); + const content = await fs.readFile(inputPath, 'utf-8'); const jsonData = convertToJson(content); - await fs.writeFile(writePath, JSON.stringify(jsonData)); + await fs.writeFile(outputPath, JSON.stringify(jsonData)); return jsonData; });🧰 Tools
🪛 ESLint
[error] 33-33: 'readPath' is already declared in the upper scope on line 18 column 9.
(no-shadow)
[error] 33-33: 'writePath' is already declared in the upper scope on line 19 column 9.
(no-shadow)
31-31: Remove extra whitespace.There's unnecessary whitespace on these lines that should be removed for consistent formatting.
- fs.writeFile.mockResolvedValue(); - + fs.writeFile.mockResolvedValue(); - fs.readFile.mockRejectedValue(error); - + fs.readFile.mockRejectedValue(error); - convertToJson.mockImplementation(() => { - throw error; - }); - + convertToJson.mockImplementation(() => { + throw error; + }); - fs.writeFile.mockRejectedValue(error); - + fs.writeFile.mockRejectedValue(error);Also applies to: 50-50, 77-77, 104-104
🧰 Tools
🪛 ESLint
[error] 31-31: Delete
····(prettier/prettier)
47-67: Modernize error testing with async/await pattern.Replace the try-catch pattern with Jest's more concise async/await pattern for error testing.
test('should throw an error if reading the file fails', async () => { // Setup mock implementations for read failure fs.readFile.mockRejectedValue(error); // Use a simple implementation that matches the original test's error message writeJSON.mockImplementation(async () => { try { await fs.readFile(readPath, 'utf-8'); } catch (err) { throw new Error(`Error while reading file\nError: ${err}`); } }); - try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while reading file\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while reading file\nError: ${error}`); expect(fs.readFile).toHaveBeenCalledWith(readPath, 'utf-8'); });🧰 Tools
🪛 ESLint
[error] 50-50: Delete
····(prettier/prettier)
71-97: Use async/await pattern for conversion error test.Similar to the previous comment, modernize this test using Jest's async/await pattern.
test('should throw an error if converting the file content to JSON fails', async () => { // Setup mock implementations for conversion failure fs.readFile.mockResolvedValue(yamlString); convertToJson.mockImplementation(() => { throw error; }); // Use a simple implementation that matches the original test's error message writeJSON.mockImplementation(async () => { try { const content = await fs.readFile(readPath, 'utf-8'); convertToJson(content); } catch (err) { throw new Error(`Error while conversion\nError: ${err}`); } }); - try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while conversion\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while conversion\nError: ${error}`); expect(convertToJson).toHaveBeenCalledWith(yamlString); });🧰 Tools
🪛 ESLint
[error] 77-77: Delete
····(prettier/prettier)
99-125: Use async/await pattern for write error test.Modernize the write error test using Jest's async/await pattern for cleaner test code.
test('should throw an error if writing the file fails', async () => { // Setup mock implementations for write failure fs.readFile.mockResolvedValue(yamlString); convertToJson.mockReturnValue(jsonObject); fs.writeFile.mockRejectedValue(error); // Use a simple implementation that matches the original test's error message writeJSON.mockImplementation(async () => { try { const content = await fs.readFile(readPath, 'utf-8'); const jsonData = convertToJson(content); await fs.writeFile(writePath, JSON.stringify(jsonData)); } catch (err) { throw new Error(`Error while writing file\nError: ${err}`); } }); - try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while writing file\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while writing file\nError: ${error}`); expect(fs.writeFile).toHaveBeenCalledWith(writePath, JSON.stringify(jsonObject)); });🧰 Tools
🪛 ESLint
[error] 104-104: Delete
····(prettier/prettier)
tests/utils.test.js (5)
1-5: Improve import ordering and spacing.The imports should follow a consistent ordering pattern and have proper spacing.
import { convertToJson, pause, writeJSON } from '../scripts/utils.ts'; + const { jsonString, yamlString, jsonObject, invalidString } = require('./fixtures/utilsData'); const fs = require('fs/promises'); const path = require('path'); const os = require('os');🧰 Tools
🪛 ESLint
[error] 1-1: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 2-2:
./fixtures/utilsDataimport should occur after import ofos(import/order)
75-76: Fix variable shadowing for jsonObject.The
jsonObjectconstant shadows the same variable imported from './fixtures/utilsData' at line 2.const yamlContent = 'key: value'; -const jsonObject = { key: 'value' }; +const expectedJsonResult = { key: 'value' };Also update all references to this variable in subsequent code.
🧰 Tools
🪛 ESLint
[error] 76-76: 'jsonObject' is already declared in the upper scope on line 2 column 33.
(no-shadow)
107-108: Fix formatting of async assertion chains.The formatting in the
.rejects.toThrow()chains should be consistent and not include unnecessary line breaks.- await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.READ); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.READ); - await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.READ); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.READ); - await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.WRITE); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.WRITE); - await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.CONVERSION); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.CONVERSION); - await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.GENERIC); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.GENERIC);Also applies to: 115-116, 123-124, 132-133, 139-140
🧰 Tools
🪛 ESLint
[error] 107-108: Delete
⏎········(prettier/prettier)
100-101: Update jsonObject reference after renaming.After renaming
jsonObjecttoexpectedJsonResult, update its usage here as well.expect(fs.readFile).toHaveBeenCalledWith(readPath, 'utf-8'); -expect(fs.writeFile).toHaveBeenCalledWith(writePath, JSON.stringify(jsonObject)); +expect(fs.writeFile).toHaveBeenCalledWith(writePath, JSON.stringify(expectedJsonResult)); expect(result).toEqual(jsonObject);
54-60: Consider using Jest's fake timers for more reliable timing tests.The pause test is relying on actual timeouts, which can make tests flaky or slow on CI systems. Consider using Jest's fake timers to make this test more reliable.
it('should pause execution for specified time', async () => { + jest.useFakeTimers(); const start = Date.now(); await pause(100); + jest.advanceTimersByTime(100); const duration = Date.now() - start; expect(duration).toBeGreaterThanOrEqual(90); // Allow small margin of error + jest.useRealTimers(); });tests/markdown/check-edit-links.test.js (2)
22-24: Fix indentation in beforeEach block.The indentation in the beforeEach block is inconsistent with the rest of the file (has extra spaces).
- beforeEach(() => { - jest.clearAllMocks(); - }); + beforeEach(() => { + jest.clearAllMocks(); + });🧰 Tools
🪛 ESLint
[error] 22-22: Delete
··(prettier/prettier)
[error] 23-23: Delete
··(prettier/prettier)
[error] 24-24: Delete
··(prettier/prettier)
1-2: Add empty line after import statement.According to the ESLint rules, import statements should be followed by an empty line.
import { logger } from '../../scripts/utils'; const path = require('path');🧰 Tools
🪛 ESLint
[error] 1-1: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
tests/dashboard/build-dashboard.test.js (1)
1-22: Fix import ordering.The ES module import should come before CommonJS require statements for consistency with modern JavaScript practices.
+import { logger } from '../../scripts/utils.ts'; + const { graphql } = require('@octokit/graphql'); const { promises: fs, mkdirSync, rmSync } = require('fs-extra'); const { resolve } = require('path'); const os = require('os'); const { getLabel, mapGoodFirstIssues, getHotDiscussions, getDiscussionByID, writeToFile, getDiscussions, start } = require('../../scripts/dashboard/build-dashboard.ts'); const { issues, mockDiscussion, discussionWithMoreComments, fullDiscussionDetails, mockRateLimitResponse } = require('../fixtures/dashboardData'); -import { logger } from '../../scripts/utils.ts';🧰 Tools
🪛 ESLint
[error] 21-21: Expected 1 empty line after require statement not followed by another require.
(import/newline-after-import)
[error] 22-22: Import in body of module; reorder to top.
(import/first)
[error] 22-22:
../../scripts/utils.tsimport should occur before import of@octokit/graphql(import/order)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/dashboard/build-dashboard.test.js(1 hunks)tests/markdown/check-edit-links.test.js(3 hunks)tests/readAndWriteJson.test.js(5 hunks)tests/utils.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
tests/utils.test.js (1)
scripts/utils.ts (3)
convertToJson(15-38)pause(45-49)writeJSON(70-90)
tests/readAndWriteJson.test.js (1)
scripts/utils.ts (2)
convertToJson(15-38)writeJSON(70-90)
🪛 ESLint
tests/dashboard/build-dashboard.test.js
[error] 22-22: Import in body of module; reorder to top.
(import/first)
[error] 22-22: ../../scripts/utils.ts import should occur before import of @octokit/graphql
(import/order)
tests/utils.test.js
[error] 1-1: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 2-2: ./fixtures/utilsData import should occur after import of os
(import/order)
[error] 76-76: 'jsonObject' is already declared in the upper scope on line 2 column 33.
(no-shadow)
[error] 107-108: Delete ⏎········
(prettier/prettier)
[error] 115-116: Delete ⏎········
(prettier/prettier)
[error] 123-124: Delete ⏎········
(prettier/prettier)
[error] 132-133: Delete ⏎········
(prettier/prettier)
[error] 139-140: Delete ⏎········
(prettier/prettier)
tests/markdown/check-edit-links.test.js
[error] 1-1: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
[error] 12-12: Missing file extension "ts" for "../../scripts/markdown/check-edit-links"
(import/extensions)
[error] 22-22: Delete ··
(prettier/prettier)
[error] 23-23: Delete ··
(prettier/prettier)
[error] 24-24: Delete ··
(prettier/prettier)
tests/readAndWriteJson.test.js
[error] 31-31: Delete ····
(prettier/prettier)
[error] 33-33: 'readPath' is already declared in the upper scope on line 18 column 9.
(no-shadow)
[error] 33-33: 'writePath' is already declared in the upper scope on line 19 column 9.
(no-shadow)
[error] 50-50: Delete ····
(prettier/prettier)
[error] 77-77: Delete ····
(prettier/prettier)
[error] 104-104: Delete ····
(prettier/prettier)
🔇 Additional comments (2)
tests/markdown/check-edit-links.test.js (1)
143-143: Clear test improvement.Adding this assertion to verify the URL content is a good improvement to the test coverage.
tests/dashboard/build-dashboard.test.js (1)
24-27: Good implementation of logger and pause mocks.The mock implementation for both logger and pause is well-structured and consistent with changes in other test files.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/build-tools.test.js (1)
38-38: Fix formatting issuesThere are trailing whitespace issues on lines 38 and 43 that should be removed to satisfy the Prettier formatting rules.
- + - +Also applies to: 43-43
🧰 Tools
🪛 ESLint
[error] 38-38: Delete
····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/build-tools.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/build-tools.test.js (1)
tests/fixtures/buildToolsData.js (1)
manualTools(12-25)
🪛 ESLint
tests/build-tools.test.js
[error] 38-38: Delete ····
(prettier/prettier)
[error] 43-43: Delete ····
(prettier/prettier)
🔇 Additional comments (2)
tests/build-tools.test.js (2)
38-56: Improved test setup with conditional file creationThe change to conditional directory and file creation is a good improvement that makes the tests more robust and idempotent. By checking for existence before creating, you prevent overwriting existing files which could help avoid potential test interference issues.
This approach is more defensive and aligned with best practices for test setup.
🧰 Tools
🪛 ESLint
[error] 38-38: Delete
····(prettier/prettier)
[error] 43-43: Delete
····(prettier/prettier)
38-56:✅ Verification successful
Verify file paths against main tooling files
While the conditional file creation logic looks good, I notice the test is using a temporary directory with simple filenames, but the PR summary mentions consolidating utility functions across scripts. Make sure that any path changes here are consistent with how the actual tooling files are organized in the production code.
🏁 Script executed:
#!/bin/bash # Compare the paths used in tests with actual file locations echo "Checking if utils.ts contains writeJSON function:" grep -r "export.*writeJSON" scripts/ echo "Checking for import paths in other files:" grep -r "import.*writeJSON" scripts/ tests/Length of output: 673
File Paths Consistency Verified
The paths in the tests have been cross-checked against the main tooling files. We verified that the production code exports the utility function (writeJSON) from
scripts/utils.tsand that tests import it using equivalent relative paths. Although thetests/build-tools.test.jsfile currently uses direct calls tofs.writeFileSyncfor setting up test files, this is consistent with the temporary test setup. If the goal is to eventually consolidate these utilities, please ensure that both production and test code are updated accordingly.🧰 Tools
🪛 ESLint
[error] 38-38: Delete
····(prettier/prettier)
[error] 43-43: Delete
····(prettier/prettier)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/markdown/check-edit-links.test.js (1)
1-1: Add file extension to import statement.The import statement is missing the
.tsfile extension, which should be included for consistency with other imports in the codebase and to satisfy ESLint rules.-import { logger } from '../../scripts/utils'; +import { logger } from '../../scripts/utils.ts';🧰 Tools
🪛 ESLint
[error] 1-1: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
tests/readAndWriteJson.test.js (4)
2-2: Consider switching to ES module syntax for consistency.Since the PR is consolidating utilities, it would be more consistent to use ES module syntax (import) instead of CommonJS (require) here, matching the style used in other updated files.
-const { convertToJson, writeJSON } = require('../scripts/utils.ts'); +import { convertToJson, writeJSON } from '../scripts/utils.ts';
58-64: Use Jest's .rejects syntax for cleaner assertions.Replace try/catch blocks with Jest's await expect().rejects pattern for cleaner and more readable test code.
- try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while reading file\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while reading file\nError: ${error}`);
85-91: Use Jest's .rejects syntax for cleaner assertions.Similar to the previous comment, this test can be simplified using Jest's await expect().rejects pattern.
- try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while conversion\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while conversion\nError: ${error}`);
112-118: Use Jest's .rejects syntax for cleaner assertions.Same recommendation as previous similar cases - use Jest's more concise syntax for expecting rejections.
- try { - await writeJSON(readPath, writePath); - // If we get here, the test should fail - expect(true).toBe(false); // This line should not be reached - } catch (err) { - expect(err.message).toBe(`Error while writing file\nError: ${error}`); - } + await expect(writeJSON(readPath, writePath)) + .rejects.toThrow(`Error while writing file\nError: ${error}`);scripts/utils.ts (2)
70-90: Add spacing in the writeJSON function for better readability.The writeJSON function has good error handling but could benefit from better spacing between variable declarations and statements for readability.
export async function writeJSON(readPath: string, writePath: string) { try { const readContent = await readFile(readPath, 'utf-8'); const jsonContent = convertToJson(readContent); + await writeFile(writePath, JSON.stringify(jsonContent)); + return jsonContent; } catch (err: any) { // Better detection of error types if (err.code === 'ENOENT' || err.message.includes('read')) { throw new Error(`Error while reading file\nError: ${err}`); } else if (err.message.includes('write')) { throw new Error(`Error while writing file\nError: ${err}`); } else if (err.message.includes('Invalid content') || - err.message.includes('JSON') || - err.message.includes('YAML')) { + err.message.includes('JSON') || + err.message.includes('YAML')) { throw new Error(`Error while conversion\nError: ${err}`); } else { throw new Error(`Error processing file: ${err}`); } } }🧰 Tools
🪛 ESLint
[error] 73-73: Expected blank line after variable declarations.
(newline-after-var)
[error] 74-74: Expected blank line before this statement.
(padding-line-between-statements)
[error] 75-75: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-82: Insert
⏎······(prettier/prettier)
[error] 83-83: Delete
·········(prettier/prettier)
[error] 84-84: Replace
·········err.message.includes('YAML')witherr.message.includes('YAML')⏎····(prettier/prettier)
76-89: Consider adding type narrowing for more robust error handling.The error handling could be improved by adding more specific type checking instead of relying on string matching and the generic
anytype.- } catch (err: any) { + } catch (err: unknown) { + const errorMessage = err instanceof Error ? err.message : String(err); + const errorObj = err as { code?: string }; // Better detection of error types - if (err.code === 'ENOENT' || err.message.includes('read')) { + if (errorObj.code === 'ENOENT' || errorMessage.includes('read')) { throw new Error(`Error while reading file\nError: ${err}`); - } else if (err.message.includes('write')) { + } else if (errorMessage.includes('write')) { throw new Error(`Error while writing file\nError: ${err}`); - } else if (err.message.includes('Invalid content') || - err.message.includes('JSON') || - err.message.includes('YAML')) { + } else if (errorMessage.includes('Invalid content') || + errorMessage.includes('JSON') || + errorMessage.includes('YAML')) { throw new Error(`Error while conversion\nError: ${err}`); } else { throw new Error(`Error processing file: ${err}`); }🧰 Tools
🪛 ESLint
[error] 82-82: Insert
⏎······(prettier/prettier)
[error] 83-83: Delete
·········(prettier/prettier)
[error] 84-84: Replace
·········err.message.includes('YAML')witherr.message.includes('YAML')⏎····(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
scripts/utils.ts(3 hunks)tests/dashboard/build-dashboard.test.js(2 hunks)tests/markdown/check-edit-links.test.js(3 hunks)tests/markdown/check-markdown.test.js(2 hunks)tests/readAndWriteJson.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/markdown/check-markdown.test.js
🧰 Additional context used
🧬 Code Definitions (2)
scripts/utils.ts (2)
tests/readAndWriteJson.test.js (2)
readPath(18-18)writePath(19-19)tests/utils.test.js (2)
readPath(23-23)writePath(24-24)
tests/readAndWriteJson.test.js (1)
scripts/utils.ts (2)
writeJSON(70-90)convertToJson(15-38)
🪛 ESLint
scripts/utils.ts
[error] 73-73: Expected blank line after variable declarations.
(newline-after-var)
[error] 74-74: Expected blank line before this statement.
(padding-line-between-statements)
[error] 75-75: Expected blank line before this statement.
(padding-line-between-statements)
[error] 82-82: Insert ⏎······
(prettier/prettier)
[error] 83-83: Delete ·········
(prettier/prettier)
[error] 84-84: Replace ·········err.message.includes('YAML') with err.message.includes('YAML')⏎····
(prettier/prettier)
tests/dashboard/build-dashboard.test.js
[error] 22-22: Import in body of module; reorder to top.
(import/first)
[error] 22-22: ../../scripts/utils.ts import should occur before import of @octokit/graphql
(import/order)
[error] 126-126: Delete ····
(prettier/prettier)
tests/markdown/check-edit-links.test.js
[error] 1-1: Missing file extension "ts" for "../../scripts/utils"
(import/extensions)
tests/readAndWriteJson.test.js
[error] 11-12: Delete ⏎
(prettier/prettier)
[error] 32-32: 'readPath' is already declared in the upper scope on line 18 column 9.
(no-shadow)
[error] 32-32: 'writePath' is already declared in the upper scope on line 19 column 9.
(no-shadow)
🔇 Additional comments (8)
tests/markdown/check-edit-links.test.js (2)
15-18: Mocking approach looks good.The mock implementation correctly includes both the logger and the new pause function. This allows tests to run without actual timeouts, improving test efficiency.
143-143: Good addition to test coverage.This assertion enhances the test by verifying that the URL filtering logic works correctly when checking for specific path segments.
tests/readAndWriteJson.test.js (1)
11-15: Mock implementation looks good.The mock setup correctly handles both utility functions needed for testing.
🧰 Tools
🪛 ESLint
[error] 11-12: Delete
⏎(prettier/prettier)
tests/dashboard/build-dashboard.test.js (2)
24-27: Mocking strategy looks good.The mock implementation correctly addresses both the logger and the new pause function, aligning with the changes in the utils module.
124-126: Good pre-test setup.Adding this setup ensures the test has the necessary file to work with, preventing potential failures due to missing files.
🧰 Tools
🪛 ESLint
[error] 126-126: Delete
····(prettier/prettier)
scripts/utils.ts (3)
2-5: Import and setup for winston logger is well structured.The addition of the winston logger and proper destructuring of its formatting utilities is a good approach for centralizing logging configuration.
15-15: Good modularity with exported functions.Exporting these utility functions makes them available throughout the codebase, which aligns with the PR objective of consolidating utilities.
Also applies to: 45-45
54-65: Well-configured logger with appropriate formatting.The logger configuration includes good defaults like timestamps and colorization, which will improve the readability of logs during development and debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/utils.test.js (6)
1-6: Fix import style inconsistency and ordering.The file mixes ES module syntax (for utils imports) and CommonJS syntax (for fixtures and Node.js modules), which can be confusing and cause issues. Also, the import ordering is flagged by ESLint.
-import { convertToJson, pause, writeJSON } from '../scripts/utils.ts'; -const { jsonString, yamlString, jsonObject, invalidString } = require('./fixtures/utilsData'); -const fs = require('fs/promises'); -const path = require('path'); -const os = require('os'); +import { convertToJson, pause, writeJSON } from '../scripts/utils.ts'; + +const fs = require('fs/promises'); +const os = require('os'); +const path = require('path'); +const { jsonString, yamlString, jsonObject, invalidString } = require('./fixtures/utilsData');🧰 Tools
🪛 ESLint
[error] 1-1: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 2-2:
./fixtures/utilsDataimport should occur after import ofos(import/order)
26-44: Fix parameter shadowing and formatting issues.There's a parameter shadowing issue in the readFile mock implementation, and some formatting issues flagged by ESLint.
beforeEach(async () => { jest.clearAllMocks(); tempDir = path.join(os.tmpdir(), 'asyncapi-test-'); readPath = path.join(tempDir, 'test-input.yaml'); writePath = path.join(tempDir, 'test-output.json'); // Ensure directory exists fs.mkdir.mockResolvedValue(undefined); // Ensure files exist with initial content - fs.readFile.mockImplementation((path) => { + fs.readFile.mockImplementation((filePath) => { - if (path === readPath) { + if (filePath === readPath) { return Promise.resolve(yamlString); } return Promise.reject(new Error('File not found')); }); fs.writeFile.mockResolvedValue(undefined); });🧰 Tools
🪛 ESLint
[error] 31-31: Delete
····(prettier/prettier)
[error] 34-34: Delete
····(prettier/prettier)
[error] 36-36: 'path' is already declared in the upper scope on line 4 column 7.
(no-shadow)
[error] 42-42: Delete
····(prettier/prettier)
67-85: Consider using Jest's fake timers for more reliable timing tests.While the current approach works, Jest's fake timers would provide more reliable and faster tests, especially in CI environments where timing can be unpredictable.
describe('pause', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); it('should pause execution for specified time', async () => { - const start = Date.now(); + const promise = pause(100); + jest.advanceTimersByTime(100); + await promise; - const duration = Date.now() - start; - expect(duration).toBeGreaterThanOrEqual(90); // Allow small margin of error + // No need to check duration with fake timers }); it('should allow pending callbacks to execute after timeout', async () => { const mockFn = jest.fn(); setTimeout(mockFn, 50); - await pause(100); + const promise = pause(100); + jest.advanceTimersByTime(100); + await promise; expect(mockFn).toHaveBeenCalled(); }); it('should handle zero milliseconds without error', async () => { await expect(pause(0)).resolves.not.toThrow(); }); });
87-90: Fix variable shadowing issue.The
jsonObjectvariable shadows the same variable imported from fixtures in line 2.describe('writeJSON', () => { const yamlContent = 'key: value'; - const jsonObject = { key: 'value' }; + const expectedJson = { key: 'value' };🧰 Tools
🪛 ESLint
[error] 89-89: 'jsonObject' is already declared in the upper scope on line 2 column 33.
(no-shadow)
117-154: LGTM! Comprehensive error handling tests with proper formatting.The tests cover all important error scenarios: file not found, read failures, write failures, conversion failures, and generic errors.
However, there are some formatting issues flagged by ESLint regarding line breaks.
it('should throw specific error when file is not found (ENOENT error code)', async () => { setupErrorTest('read', { code: 'ENOENT', message: 'File not found' }); - await expect(writeJSON(readPath, writePath)) - .rejects.toThrow(ERROR_MESSAGES.READ); + await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.READ); expect(fs.readFile).toHaveBeenCalledWith(readPath, 'utf-8'); });(Apply similar formatting fixes to other test cases)
🧰 Tools
🪛 ESLint
[error] 120-121: Delete
⏎········(prettier/prettier)
[error] 128-129: Delete
⏎········(prettier/prettier)
[error] 136-137: Delete
⏎········(prettier/prettier)
[error] 145-146: Delete
⏎········(prettier/prettier)
[error] 152-153: Delete
⏎········(prettier/prettier)
87-155: Consider adding a test for mkdir failure.While you test for read and write failures, there's no test for the case where creating the directory fails.
it('should throw specific error when creating directory fails', async () => { const dirError = new Error('Failed to create directory'); fs.mkdir.mockRejectedValue(dirError); await expect(writeJSON(readPath, writePath)).rejects.toThrow(ERROR_MESSAGES.GENERIC); expect(fs.mkdir).toHaveBeenCalled(); });🧰 Tools
🪛 ESLint
[error] 89-89: 'jsonObject' is already declared in the upper scope on line 2 column 33.
(no-shadow)
[error] 120-121: Delete
⏎········(prettier/prettier)
[error] 128-129: Delete
⏎········(prettier/prettier)
[error] 136-137: Delete
⏎········(prettier/prettier)
[error] 145-146: Delete
⏎········(prettier/prettier)
[error] 152-153: Delete
⏎········(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/utils.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/utils.test.js (1)
scripts/utils.ts (3)
convertToJson(15-38)pause(45-49)writeJSON(70-90)
🪛 ESLint
tests/utils.test.js
[error] 1-1: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
[error] 2-2: ./fixtures/utilsData import should occur after import of os
(import/order)
[error] 31-31: Delete ····
(prettier/prettier)
[error] 34-34: Delete ····
(prettier/prettier)
[error] 36-36: 'path' is already declared in the upper scope on line 4 column 7.
(no-shadow)
[error] 42-42: Delete ····
(prettier/prettier)
[error] 89-89: 'jsonObject' is already declared in the upper scope on line 2 column 33.
(no-shadow)
[error] 120-121: Delete ⏎········
(prettier/prettier)
[error] 128-129: Delete ⏎········
(prettier/prettier)
[error] 136-137: Delete ⏎········
(prettier/prettier)
[error] 145-146: Delete ⏎········
(prettier/prettier)
[error] 152-153: Delete ⏎········
(prettier/prettier)
🔇 Additional comments (5)
tests/utils.test.js (5)
7-13: LGTM! Good use of constants for error messages.Defining error message constants improves maintainability and ensures consistency throughout the tests.
15-19: LGTM! Appropriate mocking of fs/promises.The fs module is properly mocked to avoid actual filesystem interactions during tests.
46-65: LGTM! Comprehensive tests for convertToJson.The tests cover all important scenarios: JSON parsing, YAML parsing, object handling, and error cases. The test cases are well-named and clearly describe what they're testing.
91-104: LGTM! Good helper function for error testing.The helper function avoids code duplication and makes error test cases cleaner.
106-115: LGTM! Well-structured happy path test.This test case properly verifies the main functionality of the writeJSON function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/utils.test.js (2)
1-7: Harmonize import systems used in the test file.The file mixes ES module syntax (
importon line 1) with CommonJS (requireon lines 3-6). Consider standardizing to use ES modules throughout for consistency, especially since the project appears to be moving in that direction with the utility function consolidation.- import { convertToJson, pause, writeJSON } from '../scripts/utils.ts'; - - const { jsonString, yamlString, jsonObject, invalidString } = require('./fixtures/utilsData'); - const fs = require('fs/promises'); - const path = require('path'); - const os = require('os'); + import { convertToJson, pause, writeJSON } from '../scripts/utils.ts'; + import { jsonString, yamlString, jsonObject, invalidString } from './fixtures/utilsData'; + import fs from 'fs/promises'; + import path from 'path'; + import os from 'os';Also, according to ESLint, the imports should be ordered with
osbefore./fixtures/utilsData.🧰 Tools
🪛 ESLint
[error] 3-3:
./fixtures/utilsDataimport should occur after import ofos(import/order)
37-42: Fix variable shadowing of 'path'.The parameter name 'path' shadows the imported 'path' module. Rename the parameter to avoid confusion.
- fs.readFile.mockImplementation((path) => { + fs.readFile.mockImplementation((filePath) => { - if (path === readPath) { + if (filePath === readPath) { return Promise.resolve(yamlString); } return Promise.reject(new Error('File not found')); });🧰 Tools
🪛 ESLint
[error] 37-37: 'path' is already declared in the upper scope on line 5 column 7.
(no-shadow)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/dashboard/build-dashboard.test.js(3 hunks)tests/utils.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/utils.test.js (1)
scripts/utils.ts (3)
convertToJson(15-38)pause(45-49)writeJSON(70-90)
🪛 ESLint
tests/utils.test.js
[error] 3-3: ./fixtures/utilsData import should occur after import of os
(import/order)
[error] 37-37: 'path' is already declared in the upper scope on line 5 column 7.
(no-shadow)
🔇 Additional comments (7)
tests/dashboard/build-dashboard.test.js (3)
1-2: Import path updated correctly.The logger import has been moved to the top of the file and updated to use the consolidated
utils.tsfile, which aligns with the PR objective of merging utility functions into a single file.
25-27: Jest mock updated to reflect new module structure.The mock has been correctly updated to target the consolidated
utils.tsmodule instead of the separate logger file. The addition of thepausemock is appropriate as this function is also part of the utils module now.
125-127: Good addition for test robustness.Adding this file setup step ensures the test doesn't fail due to a missing file, which is a good practice for improving test reliability.
tests/utils.test.js (4)
47-66: Well-structured tests for convertToJson.The tests for
convertToJsonprovide comprehensive coverage of different input scenarios:
- JSON string parsing
- YAML string parsing
- Direct object handling
- Error handling for invalid input
This is a good example of thorough testing.
68-97: Good implementation of timer-based tests.The tests for the
pausefunction demonstrate good practices:
- Using Jest's fake timers to control time
- Properly cleaning up with afterEach
- Testing multiple scenarios including zero milliseconds
- Verifying that callbacks execute properly after timeouts
These tests will help prevent the hanging processes mentioned in the PR objectives.
99-162: Comprehensive error handling tests for writeJSON.The
writeJSONtests are well-implemented with:
- A reusable helper function for setting up error tests
- Coverage of specific error types (ENOENT, read failures, write failures)
- Testing of conversion errors
- Proper error message verification
This aligns well with the PR objective of adding thorough error handling tests for the
writeJSONfunction.
104-116: Excellent use of helper function for testing error scenarios.The
setupErrorTesthelper function is a good example of DRY principles in test code. It reduces duplication across different error test cases and makes the tests more maintainable.
|
@Siddhant-Pragyan-Sinha checks are failing fix it |
|
@vishvamsinh28 I have updated the branch and the checks pass. It was due a |
fixes issue #3956
Describe the bug
BEFORE:-
AFTER:-
Summary of Changes by File
utils.test.js
convertToJsonfunction to handle JSON parsing, YAML parsing, object handling, and invalid input scenariospausefunction, verifying timeout behavior, resolution after specified time, and handling of zero/negative timeout valueswriteJSONfunction, covering specific error scenarios:check-edit-links.test.js
Implemented test coverage for
pauseandloggerfunctions.Changed the import statement to match the new configuration.
tools-object.test.js
Implemented test coverage for
convertToJsonalong withlogger.Changed the import statement to match the new file configuration.
##readAndWriteJson.test.js
Implemented test coverage for
writeJSONalong withconvertToJson.Used a simple mock implementation for
writeJSONthat matches the original test.##extract-tools-github.test.js
Implemented test coverage for
convertToJsonalong withlogger.Changed the import statement to match the new file configuration.
build-tools.test.js
utils.ts
No direct changes to the implementation code were made
The logger.ts file and the readAndWriteJson.ts file components were added to it without any change in their code.
build-dashboard.test.js
These changes collectively achieved 100% test coverage across the codebase while addressing specific issues with hanging processes and test output cleanliness.
How to test:-
npm testto see the outputChecklist:-
No error was encountered when executing
tsc --noEmit.No error was encountered when executing
npm run build.No error was encountered when executing
npm run lint.Feedback from maintainers and reviewers is requested, to review and do the needful changes, if any.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
pauseandwriteJSONadded for better asynchronous operations and file management.Refactor