test(relayer): adds initial testing base #375
Conversation
fcanela
requested review from
jaybuidl,
mani99brar and
alcercu
as code owners
WalkthroughThe changes in this pull request introduce several updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (4)
relayer-cli/package.json (1)
13-13: Consider expanding test scripts for better developer experience.While the basic test script with coverage is good, consider adding more specialized scripts:
"scripts": { "test": "jest --coverage", + "test:watch": "jest --watch", + "test:ci": "jest --ci --coverage --coverageThreshold='{\"global\":{\"branches\":80,\"functions\":80,\"lines\":80,\"statements\":80}}'", "start-devnet-relayer": "npx ts-node ./src/devnetRelayExample.ts",relayer-cli/src/utils/shutdownManager.ts (1)
1-15: LGTM with suggestions for enhanced functionalityThe implementation follows good practices with proper encapsulation and immutable state management. Consider these enhancements:
- Add JSDoc documentation to describe the class purpose and methods
- Consider adding event emission on state change for better integration
- Consider adding a reset capability for testing scenarios
Example enhancement:
+import { EventEmitter } from 'events'; + +/** + * Manages the shutdown state of the relayer + * @emits shutdown - Emitted when shutdown is triggered + */ -export default class ShutdownManager { +export default class ShutdownManager extends EventEmitter { private isShuttingDown: boolean; constructor(initialState: boolean = false) { + super(); this.isShuttingDown = initialState; } + /** + * Returns the current shutdown state + */ public getIsShuttingDown(): boolean { return this.isShuttingDown; } + /** + * Triggers the shutdown process + * @emits shutdown + */ public triggerShutdown() { this.isShuttingDown = true; + this.emit('shutdown'); } + /** + * Resets the shutdown state (useful for testing) + */ + public reset() { + this.isShuttingDown = false; + } }relayer-cli/src/utils/relayerHelpers.ts (2)
Line range hint
47-76: Fix potential issues in exit handlersThere are several concerns in the exit handling logic:
- Async cleanup in 'exit' event won't work
- Duplicate process.exit calls
- Potential race condition in cleanup
- Inconsistent error handling
Suggested improvements:
async function setupExitHandlers(chainId: number, shutdownManager: ShutdownManager, network: string) { const cleanup = async () => { console.log("exit"); const lockFileName = "./state/" + network + "_" + chainId + ".pid"; if (fs.existsSync(lockFileName)) { - await fs.promises.unlink(lockFileName); + try { + // Use sync version for cleanup to ensure completion + fs.unlinkSync(lockFileName); + } catch (error) { + console.error('Failed to cleanup:', error); + } } }; const handleExit = async (exitCode: number = 0) => { shutdownManager.triggerShutdown(); - await cleanup(); + cleanup(); - process.exit(0); + process.exit(exitCode); }; ["SIGINT", "SIGTERM", "SIGQUIT"].forEach((signal) => process.on(signal, async () => { - await handleExit(0); - process.exit(0); + await handleExit(0); }) ); // 'exit' event cannot handle async operations process.on("exit", () => { - await handleExit(); + cleanup(); }); process.on("uncaughtException", async (err) => { console.error("Uncaught exception:", err); await handleExit(1); }); process.on("unhandledRejection", async (reason, promise) => { console.error("Unhandled promise rejection:", reason, "at", promise); await handleExit(1); }); }
2-2: Consider using more specific importsImport only the required ShutdownManager type to improve bundle size.
-import ShutdownManager from "./shutdownManager"; +import type { ShutdownManager } from "./shutdownManager";
📜 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 (6)
relayer-cli/.gitconfig(1 hunks)relayer-cli/jest.config.ts(1 hunks)relayer-cli/package.json(2 hunks)relayer-cli/src/utils/relayerHelpers.ts(1 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)
🔇 Additional comments (1)
relayer-cli/package.json (1)
27-31: 🛠️ Refactor suggestion
Review dependency management practices.
A few observations about the dependencies:
- The explicit
yarndevDependency might be redundant since the package manager is already specified inpackageManagerfield - Using caret ranges (
^) for all dependencies could lead to unexpected breaks. Consider using exact versions for test framework dependencies to ensure consistent behavior across environments.
"devDependencies": {
- "@types/jest": "^29.5.14",
- "jest": "^29.7.0",
- "ts-jest": "^29.2.5",
+ "@types/jest": "29.5.14",
+ "jest": "29.7.0",
+ "ts-jest": "29.2.5",
"ts-node": "^10.9.2",
- "yarn": "^1.22.22"
}Let's verify if yarn is used elsewhere in the project:
✅ Verification successful
Remove redundant yarn dependency and consider fixing version ranges
The verification shows that:
- Yarn is already specified as the package manager (
[email protected]) in thepackageManagerfield, making the explicit yarn dependency redundant - Multiple yarn-specific scripts are used throughout the project, confirming yarn as the primary package manager
- The project heavily relies on yarn for its build, deployment, and development workflows
The original review suggestions are valid:
- Remove the redundant
yarndevDependency - Consider using exact versions for test framework dependencies to ensure consistent behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if yarn is explicitly required in any source files
rg -l "require\(['\"]yarn['\"]|import.*from.*['\"]yarn['\"]"
Length of output: 70
Script:
#!/bin/bash
# Check for packageManager field in package.json files
fd package.json --exec jq -r '.packageManager // empty'
# Check for yarn-specific files and configs
fd -g 'yarn.lock|.yarnrc|.yarnrc.yml'
# Check for yarn-specific scripts in package.json
fd package.json --exec jq -r '.scripts | to_entries | .[] | select(.value | contains("yarn")) | .key + ": " + .value'
Length of output: 1653
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.eslintrc.json (1)
Line range hint
6-11: Consider cleaning up environment configurationsThere are a few potential inconsistencies in the environment setup:
- Both "es6" and "es2020" environments are enabled, which is redundant as "es2020" includes all "es6" features
- Both "mocha" and "jest" environments are enabled, but the PR indicates a transition to Jest for testing
Consider applying this cleanup:
"env": { "browser": true, - "es6": true, "node": true, - "mocha": true, "es2020": true, "jest": true },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.eslintrc.json(1 hunks)
🔇 Additional comments (1)
.eslintrc.json (1)
11-11: LGTM: Jest environment enabled
The addition of the Jest environment is appropriate for the introduction of Jest-based testing infrastructure.
|
This PR introduces a foundation consisting of basic setup and an initial test for relayer.
PR-Codex overview
This PR introduces testing capabilities using
jest, adds a newShutdownManagerclass, and updates configuration files to support TypeScript and ESLint. It also enhances coverage reporting and updates various package dependencies.Detailed summary
ShutdownManagerclass inshutdownManager.ts.ShutdownManagerinshutdownManager.test.ts..eslintrc.jsonto includejestandts-jest.jestand@types/jesttodevDependencies.jestinjest.config.tsfor TypeScript.package.jsonto include a test script for coverage.yarn.lock.Summary by CodeRabbit
Release Notes
New Features
ShutdownManagerclass to manage system shutdown states.package.jsonfor running tests with coverage.Bug Fixes
ShutdownManagerclass.Tests
ShutdownManagerclass, covering instance creation and shutdown status retrieval.getIsShuttingDownmethod and a placeholder for thetriggerShutdownmethod.