backport fix for: 1826 #1837
backport fix for: 1826 #1837
Conversation
WalkthroughThis pull request updates the testing and build configurations, as well as refines functionality in the eth_estimateGas method and its related tests. The Jest configuration now uses a new environment variable ( Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Test Script
participant JestConf as Jest Config
Script->>JestConf: Run tests with APPLYCODECOVLIMITS env var
JestConf->>JestConf: Evaluate APPLYCODECOVLIMITS value
alt Value is 'true'
JestConf-->>Script: Apply coverage thresholds
else Value is not 'true'
JestConf-->>Script: Skip coverage thresholds
end
sequenceDiagram
participant Client as Caller
participant EthGas as ethEstimateGas Function
Client->>EthGas: Invoke with transaction parameters
alt Data field is undefined
EthGas-->>EthGas: Set default data to '0x'
end
EthGas-->>Client: Return estimated gas as hex value
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
![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 (4)
packages/network/package.json (4)
35-35: Explicit Unit Test Coverage Configuration
AddingAPPLYCODECOVLIMITS=falseclearly designates that unit tests should not enforce coverage limits.Suggestion: Consider using a tool like
cross-envfor setting environment variables so that this script works seamlessly across different operating systems.
36-36: Explicit Integration Test Coverage Configuration
The integration test command now correctly setsAPPLYCODECOVLIMITS=false, making the intended test behavior explicit.Suggestion: Similar to the unit tests, wrapping the environment variable assignment with
cross-envmight help maintain cross-platform consistency.
37-37: Integration Solo Test Command Logic Review
Thetest:integration:soloscript includes the environment variableAPPLYCODECOVLIMITS=false. The chaining of commands with&&followed by an||to runyarn stop-thor-soloensures that the stopping command executes even if earlier steps fail.Note: Double-check that this command grouping aligns with your error handling expectations. Consider grouping commands in parentheses if a more explicit order of operations is desired.
39-39: Global Test Command Coverage Enforcement
Updating the maintestscript withAPPLYCODECOVLIMITS=trueenforces the coverage limits appropriately when running all tests together.Suggestion: Using
cross-envhere as well could improve cross-platform compatibility, ensuring consistent behavior across different development environments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/network/jest.config.js(2 hunks)packages/network/package.json(1 hunks)packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts(2 hunks)packages/network/tests/provider/rpc-mapper/methods/eth_estimateGas/fixture.ts(3 hunks)turbo.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: rpc-proxy / test / test
- GitHub Check: unit-integration-test-browser / Build & Lint (latest)
- GitHub Check: unit-integration-test-browser / Build & Lint (lts/*)
- GitHub Check: unit-integration-test-browser / Build & Lint (18)
- GitHub Check: unit-integration-test / Build & Lint (latest)
- GitHub Check: unit-integration-test / Build & Lint (lts/*)
- GitHub Check: unit-integration-test / Build & Lint (18)
- GitHub Check: test-apps / Install and test example apps
- GitHub Check: install-build / Build & Lint
- GitHub Check: Execute doc examples
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/network/jest.config.js (1)
3-3: LGTM! Clear and explicit coverage threshold control.The change from
UNITtoAPPLYCODECOVLIMITSmakes the coverage threshold control more explicit and self-documenting.Also applies to: 13-13
packages/network/src/provider/utils/rpc-mapper/methods/eth_estimateGas/eth_estimateGas.ts (1)
55-55: LGTM! Improved Ethereum compatibility and code clarity.Two good improvements:
- Using '0x' as default data value aligns with Ethereum's empty data representation
- Simplified return statement while maintaining Promise functionality
Also applies to: 65-65
packages/network/tests/provider/rpc-mapper/methods/eth_estimateGas/fixture.ts (1)
1-1: LGTM! Enhanced test coverage with clearer test cases.The changes improve the test suite by:
- Simplifying imports to essential dependencies
- Using plain objects for test inputs
- Adding coverage for optional 'from' parameter
Also applies to: 10-14, 31-41
turbo.json (1)
5-6: Verify if build cache disabling is necessary.Disabling the build cache could impact build performance. Please confirm if this is necessary for the backport fix.
Could you explain why the build cache needs to be disabled? If it's not strictly required for the backport fix, consider keeping the cache enabled for better build performance.
Test CoverageSummary
|
Description
Backport to main of fix for #1826 thats been fixed on 1.0.0 release branch
Fixes #1826
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist:
Summary by CodeRabbit