refactor/lockfile #377
refactor/lockfile #377
Conversation
fcanela
requested review from
jaybuidl,
mani99brar and
alcercu
as code owners
WalkthroughThis pull request introduces several changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2).eslintrc.json (2)
The addition of the Jest environment aligns with the PR's objective of introducing Jest testing framework.
Both Let's verify if Mocha is still in use: 🪧 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
CodeRabbit Configuration File (
|
❌ Deploy Preview for veascan failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
relayer-cli/package.json (1)
27-31: Review the necessity of addingyarnas a devDependency.Including
"yarn"indevDependenciesis uncommon, asyarnis typically installed globally. This addition may cause version conflicts or redundancy.Apply this diff to remove
"yarn"fromdevDependencies:{ ... "devDependencies": { "@types/jest": "^29.5.14", "jest": "^29.7.0", "ts-jest": "^29.2.5", "ts-node": "^10.9.2", - "yarn": "^1.22.22" } }relayer-cli/src/utils/shutdownManager.test.ts (1)
33-35: Implement the pending test fortriggerShutdownmethod.The test marked as
it.todois pending implementation. Completing this test will enhance test coverage and ensure thattriggerShutdownfunctions as expected.relayer-cli/src/utils/lock.ts (1)
17-22: Pass error message to the superclass constructor and ensure consistent error naming.Passing the error message to
super()ensures proper error initialization. Also, setting thenameproperty to match the class name improves error handling consistency.Apply this diff to improve the error class:
export class LockfileExistsError extends Error { constructor(path: string) { - super(); - this.message = `The application tried to claim the lockfile ${path} but it already exists. Please ensure no other instance is running and delete the lockfile before starting a new one.`; - this.name = "OnlyOneProcessError"; + const message = `The application tried to claim the lockfile ${path} but it already exists. Please ensure no other instance is running and delete the lockfile before starting a new one.`; + super(message); + this.name = "LockfileExistsError"; } }relayer-cli/src/utils/lock.test.ts (2)
4-20: Consider adding edge cases to getLockFilePath testsThe current tests cover the happy path and case sensitivity, but consider adding tests for:
- Invalid network names (e.g., empty string, special characters)
- Invalid chain IDs (e.g., negative numbers, zero)
- Path traversal attempts
Example test cases:
it("should handle invalid network names", () => { expect(() => getLockFilePath("", 1)).toThrow(); expect(() => getLockFilePath("../malicious", 1)).toThrow(); }); it("should handle invalid chain IDs", () => { expect(() => getLockFilePath("mainnet", -1)).toThrow(); expect(() => getLockFilePath("mainnet", 0)).toThrow(); });🧰 Tools
🪛 eslint
[error] 10-10: 'expect' is not defined.
(no-undef)
[error] 18-18: 'expect' is not defined.
(no-undef)
1-84: Well-structured tests supporting the Lockfile extractionThe test organization effectively supports the PR's objective of extracting Lockfile logic. The dependency injection pattern used throughout makes the tests maintainable and the lock management logic easily portable.
A few architectural suggestions:
- Consider moving the common test setup (network, chainId) to beforeEach blocks
- Consider adding integration tests that verify the complete lock lifecycle
- Document the expected filesystem structure in the test file
🧰 Tools
🪛 eslint
[error] 10-10: 'expect' is not defined.
(no-undef)
[error] 18-18: 'expect' is not defined.
(no-undef)
[error] 29-29: 'jest' is not defined.
(no-undef)
[error] 32-32: 'expect' is not defined.
(no-undef)
[error] 37-37: 'jest' is not defined.
(no-undef)
[error] 38-38: 'jest' is not defined.
(no-undef)
[error] 43-43: 'expect' is not defined.
(no-undef)
[error] 44-44: 'expect' is not defined.
(no-undef)
[error] 47-47: 'expect' is not defined.
(no-undef)
[error] 48-48: 'expect' is not defined.
(no-undef)
[error] 59-59: 'jest' is not defined.
(no-undef)
[error] 60-60: 'jest' is not defined.
(no-undef)
[error] 65-65: 'expect' is not defined.
(no-undef)
[error] 66-66: 'expect' is not defined.
(no-undef)
[error] 69-69: 'expect' is not defined.
(no-undef)
[error] 74-74: 'jest' is not defined.
(no-undef)
[error] 75-75: 'jest' is not defined.
(no-undef)
[error] 80-80: 'expect' is not defined.
(no-undef)
[error] 81-81: 'expect' is not defined.
(no-undef)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
relayer-cli/.gitconfig(1 hunks)relayer-cli/jest.config.ts(1 hunks)relayer-cli/package.json(2 hunks)relayer-cli/src/utils/lock.test.ts(1 hunks)relayer-cli/src/utils/lock.ts(1 hunks)relayer-cli/src/utils/relayerHelpers.ts(2 hunks)relayer-cli/src/utils/shutdownManager.test.ts(1 hunks)relayer-cli/src/utils/shutdownManager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- relayer-cli/.gitconfig
- relayer-cli/jest.config.ts
🧰 Additional context used
🪛 eslint
relayer-cli/src/utils/shutdownManager.test.ts
[error] 7-7: 'expect' is not defined.
(no-undef)
[error] 12-12: 'expect' is not defined.
(no-undef)
[error] 17-17: 'expect' is not defined.
(no-undef)
[error] 24-24: 'expect' is not defined.
(no-undef)
[error] 29-29: 'expect' is not defined.
(no-undef)
relayer-cli/src/utils/lock.test.ts
[error] 10-10: 'expect' is not defined.
(no-undef)
[error] 18-18: 'expect' is not defined.
(no-undef)
[error] 29-29: 'jest' is not defined.
(no-undef)
[error] 32-32: 'expect' is not defined.
(no-undef)
[error] 37-37: 'jest' is not defined.
(no-undef)
[error] 38-38: 'jest' is not defined.
(no-undef)
[error] 43-43: 'expect' is not defined.
(no-undef)
[error] 44-44: 'expect' is not defined.
(no-undef)
[error] 47-47: 'expect' is not defined.
(no-undef)
[error] 48-48: 'expect' is not defined.
(no-undef)
[error] 59-59: 'jest' is not defined.
(no-undef)
[error] 60-60: 'jest' is not defined.
(no-undef)
[error] 65-65: 'expect' is not defined.
(no-undef)
[error] 66-66: 'expect' is not defined.
(no-undef)
[error] 69-69: 'expect' is not defined.
(no-undef)
[error] 74-74: 'jest' is not defined.
(no-undef)
[error] 75-75: 'jest' is not defined.
(no-undef)
[error] 80-80: 'expect' is not defined.
(no-undef)
[error] 81-81: 'expect' is not defined.
(no-undef)
🔇 Additional comments (3)
relayer-cli/src/utils/relayerHelpers.ts (1)
2-3: Imports updated to reflect new module structure.
The import statements correctly include claimLock, releaseLock, and ShutdownManager from their respective modules, aligning with the refactored codebase.
relayer-cli/src/utils/shutdownManager.ts (1)
1-15: Class implementation is clean and straightforward.
The ShutdownManager class correctly encapsulates the shutdown state with appropriate methods.
relayer-cli/package.json (1)
13-13: Testing script added successfully.
The "test" script enables running Jest with coverage, aligning with the testing additions in the project.
|
Extracts the Lockfile logic so it is easier to remove once we normalize the repository to be deployable to our infrastructure. Provides tests.
This PR is based in #375, so the diff may contain content from that PR if it has not been merged yet.
PR-Codex overview
This PR focuses on implementing a locking mechanism to prevent multiple instances of a process from running simultaneously. It adds functions for claiming and releasing locks, along with corresponding tests to ensure functionality.
Detailed summary
claimLockandreleaseLockfunctions inlock.ts.getLockFilePathto generate lock file paths.LockfileExistsErrorfor error handling.initializefunction inrelayerHelpers.tsto use locking functions.lock.test.ts.Summary by CodeRabbit
New Features
package.jsonfor running tests with coverage.Bug Fixes
ShutdownManagerclass and integrating lock management functions.Tests