Add file utility functions with unit tests #48
Conversation
- Introduced `fileUtils.ts` containing `readJsonFile` and `writeJsonFile` functions for reading and writing JSON files, respectively. - Implemented error handling using `FileUtilsError` for invalid paths and file operation failures. - Created `fileUtils.test.ts` to validate the functionality of the file utility functions, ensuring proper error handling and file operations. These additions enhance file management capabilities and improve test coverage for file-related operations.
📝 Walkthrough""" WalkthroughA new utility module for JSON file operations was added, providing asynchronous functions to read and write JSON files with robust error handling via a custom error class. Comprehensive unit tests were introduced to validate these functions, covering both success and failure scenarios using mocked filesystem operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fileUtils
participant fs
participant path
Caller->>fileUtils: readJsonFile(filePath)
fileUtils->>fs: readFile(filePath, 'utf-8')
fs-->>fileUtils: fileContent
fileUtils->>fileUtils: JSON.parse(fileContent)
fileUtils-->>Caller: parsedObject
Caller->>fileUtils: writeJsonFile(filePath, data)
fileUtils->>path: dirname(filePath)
fileUtils->>fs: mkdir(dir, { recursive: true })
fileUtils->>fs: writeFile(filePath, JSON.stringify(data, null, 2))
fileUtils-->>Caller: (void)
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:773391c4310bd825f0daf6a2015fa8b33014b1bcf21bdd4e2db36070d92a9b15 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 243 MB |
| packages | 1628 |
📦 Base Image node:20-alpine
| also known as |
|
| digest | sha256:37a5a350292926f98d48de9af160b0a3f7fcb141566117ee452742739500a5bd |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
workers/main/src/common/fileUtils.test.ts (4)
1-7: Consider improving mock type safety.The mock setup works correctly but could benefit from better TypeScript support.
Consider using a more type-safe mock approach:
-vi.mock('fs', () => ({ - promises: { - readFile: vi.fn(), - writeFile: vi.fn(), - mkdir: vi.fn(), - }, -})); +vi.mock('fs', () => ({ + promises: { + readFile: vi.fn<typeof import('fs').promises.readFile>(), + writeFile: vi.fn<typeof import('fs').promises.writeFile>(), + mkdir: vi.fn<typeof import('fs').promises.mkdir>(), + }, +}));
21-23: Simplify mock cleanup.The
vi.restoreAllMocks()inafterEachis redundant sincevi.clearAllMocks()inbeforeEachis sufficient for this use case.Apply this diff to remove redundant cleanup:
- afterEach(() => { - vi.restoreAllMocks(); - });
25-51: Enhance test coverage with additional edge cases.The current tests cover the main scenarios well, but consider adding tests for:
- Whitespace-only file paths
- Non-string file paths (null, undefined, numbers)
- Specific error message validation
Consider adding these additional test cases:
+ test('throws FileUtilsError for whitespace-only path', async () => { + await expect(readJsonFile(' ')).rejects.toBeInstanceOf(FileUtilsError); + }); + + test('throws FileUtilsError for non-string path', async () => { + // @ts-expect-error Testing runtime behavior + await expect(readJsonFile(null)).rejects.toBeInstanceOf(FileUtilsError); + // @ts-expect-error Testing runtime behavior + await expect(readJsonFile(123)).rejects.toBeInstanceOf(FileUtilsError); + }); + + test('throws FileUtilsError with specific message for invalid JSON', async () => { + vi.mocked(fs.readFile).mockResolvedValueOnce('not-json'); + await expect(readJsonFile('bad.json')).rejects.toThrow( + 'Failed to read or parse JSON file at "bad.json"' + ); + });
53-91: Add test for non-serializable data.The tests thoroughly cover file system operations but miss testing data that cannot be JSON serialized.
Consider adding a test for circular references or other non-serializable data:
+ test('throws FileUtilsError for non-serializable data', async () => { + const circularData: any = { a: 1 }; + circularData.self = circularData; // Create circular reference + + await expect(writeJsonFile('test.json', circularData)).rejects.toBeInstanceOf( + FileUtilsError, + ); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workers/main/src/common/fileUtils.test.ts(1 hunks)workers/main/src/common/fileUtils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
workers/main/src/common/fileUtils.ts (1)
workers/main/src/common/errors/FileUtilsError.ts (1)
FileUtilsError(3-7)
workers/main/src/common/fileUtils.test.ts (2)
workers/main/src/common/fileUtils.ts (2)
readJsonFile(6-19)writeJsonFile(21-36)workers/main/src/common/errors/FileUtilsError.ts (1)
FileUtilsError(3-7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Service Availability Check
- Removed redundant checks for invalid file paths in `readJsonFile` and `writeJsonFile` functions in `fileUtils.ts`. - Deleted corresponding unit tests for invalid path scenarios in `fileUtils.test.ts`, streamlining error handling. - This refactor simplifies the codebase and focuses on handling errors during file operations rather than validating input paths.
- Updated the `readJsonFile` function to store the parsed JSON content in a variable before returning, improving readability. - Simplified the `writeJsonFile` function by using a variable for the JSON string content, enhancing code clarity. These changes streamline the file utility functions, making them easier to maintain and understand.
- Deleted the `fetchFinancialData.ts` file as it was no longer needed. - Introduced `getProjectUnits.ts` to fetch project units and return a file link for the weekly financial reports. - Updated the `weeklyFinancialReports` workflow to utilize the new `getProjectUnits` function. - Added unit tests for the new workflow to ensure proper functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts (2)
19-31: Simplify mock interfaces.The custom mock interfaces add unnecessary complexity. Vitest's built-in mocking capabilities can handle this more simply.
Consider using Vitest's native mock types instead:
-interface TargetUnitRepositoryMock { - mockClear: () => void; - mockImplementation: ( - impl: () => { getTargetUnits: () => Promise<TargetUnit[]> }, - ) => void; -} - -interface RedminePoolMock { - mockClear: () => void; - mockImplementation: ( - impl: () => { getPool: () => string; endPool: () => void }, - ) => void; -}You can use
vi.mocked()directly without custom interfaces.
48-59: Consider simplifying the repository mock setup.The
vi.importMockapproach adds complexity. You can use simpler mocking patterns.-async function setupTargetUnitRepositoryMock(): Promise<TargetUnitRepositoryMock> { - const imported = await vi.importMock( - '../../services/TargetUnit/TargetUnitRepository', - ); - const repo = vi.mocked( - imported.TargetUnitRepository, - ) as TargetUnitRepositoryMock; - - repo.mockClear(); - - return repo; -}Consider using the mocked constructor directly in your tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
workers/main/src/activities/fetchFinancialData.ts(0 hunks)workers/main/src/activities/index.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts(1 hunks)workers/main/src/activities/weeklyFinancialReports/index.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/index.test.ts(0 hunks)workers/main/src/workflows/weeklyFinancialReports/index.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts(1 hunks)workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts(1 hunks)
💤 Files with no reviewable changes (2)
- workers/main/src/workflows/weeklyFinancialReports/index.test.ts
- workers/main/src/activities/fetchFinancialData.ts
✅ Files skipped from review due to trivial changes (3)
- workers/main/src/activities/index.ts
- workers/main/src/activities/weeklyFinancialReports/index.ts
- workers/main/src/workflows/weeklyFinancialReports/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (1)
getTargetUnits(11-31)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (1)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (1)
weeklyFinancialReportsWorkflow(9-13)
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (5)
workers/main/src/common/RedminePool.ts (1)
RedminePool(4-46)workers/main/src/configs/redmineDatabase.ts (1)
redmineDatabaseConfig(3-8)workers/main/src/services/TargetUnit/TargetUnitRepository.ts (1)
TargetUnitRepository(9-52)workers/main/src/common/fileUtils.ts (1)
writeJsonFile(19-32)workers/main/src/common/errors/AppError.ts (1)
AppError(1-9)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docker Security Scanning (temporal, Dockerfile.temporal, temporal-test:latest)
- GitHub Check: Docker Security Scanning (n8n, Dockerfile.n8n, n8n-test:latest)
- GitHub Check: Service Availability Check
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.ts (2)
5-7: LGTM! Proper activity configuration.The activity proxy configuration with a 10-minute timeout is appropriate for database operations and file I/O.
9-13: LGTM! Clean workflow implementation.The workflow correctly calls the activity and returns the expected fileLink. The implementation follows Temporal best practices by keeping the workflow logic simple and delegating the heavy lifting to activities.
workers/main/src/workflows/weeklyFinancialReports/weeklyFinancialReports.workflow.test.ts (2)
5-11: LGTM! Proper workflow testing approach.The mock correctly stubs the
proxyActivitiesfunction and returns a realistic fileLink value for testing.
13-19: LGTM! Comprehensive workflow test.The test verifies that the workflow correctly returns the fileLink from the activity result, which covers the core workflow logic.
workers/main/src/activities/weeklyFinancialReports/getTargetUnits.ts (2)
11-13: LGTM! Proper resource initialization.The RedminePool is correctly initialized with the database configuration.
24-31: LGTM! Excellent error handling and resource cleanup.The error handling properly wraps exceptions in a descriptive
AppError, and thefinallyblock ensures the database pool is always closed, preventing resource leaks.workers/main/src/activities/weeklyFinancialReports/getTargetUnits.test.ts (1)
115-147: LGTM! Excellent test coverage.The test cases comprehensively cover:
- Success scenario with proper file writing
- Error handling for repository failures
- Error handling for file writing failures
- Resource cleanup verification
The tests ensure the activity behaves correctly in all scenarios.
|
fileUtils.tscontainingreadJsonFileandwriteJsonFilefunctions for reading and writing JSON files, respectively.FileUtilsErrorfor invalid paths and file operation failures.fileUtils.test.tsto validate the functionality of the file utility functions, ensuring proper error handling and file operations.These additions enhance file management capabilities and improve test coverage for file-related operations.