feat(ethexe/rpc): black-box tests for RPC service#5399
feat(ethexe/rpc): black-box tests for RPC service#5399
Conversation
Changed Files
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the reliability of the RPC service by implementing a comprehensive black-box testing suite. By utilizing property-based testing, the new tests ensure that the RPC interface correctly handles various inputs and edge cases, improving overall system stability and API contract adherence. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces black-box integration tests for the ethexe RPC server, including property-based tests for transaction retrieval. It also updates the workspace hack configuration. Key feedback includes a warning that removing wasm32 target restrictions in Cargo.toml may break WASM contract compilation due to incompatible dependencies. In the new tests, issues were identified regarding a port-binding race condition, the use of sleep for synchronization, inefficient server restarts within property test loops, and a potential logic error in SCALE-encoded bytecode assertions.
|
|
||
| ### BEGIN HAKARI SECTION | ||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
| [dependencies] |
There was a problem hiding this comment.
Removing the cfg(not(target_arch = "wasm32")) target restriction from the workspace hack is likely to break the compilation of WASM smart contracts. Many of the dependencies listed here (e.g., alloy, tokio, aes) are not no_std compatible and should not be included when building for the wasm32-unknown-unknown target. Since this file is managed by hakari, please verify if the hakari configuration was unintentionally altered or if this manual change is necessary.
| [dependencies] | |
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] |
|
|
||
| // The RPC method registers its promise waiter after the service accepts | ||
| // the transaction, so publish from the next task turn. | ||
| tokio::time::sleep(Duration::from_millis(10)).await; |
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| fn unused_local_addr() -> SocketAddr { |
There was a problem hiding this comment.
The unused_local_addr function has a race condition: it binds to a port and then immediately drops the listener, releasing the port. Another process could bind to the same port before the RPC server starts. A more reliable approach is to let the server bind to port 0 and then query the assigned address, or pass a pre-bound TcpListener to the server.
| .await | ||
| .expect("stored original code must be returned"); | ||
|
|
||
| assert_eq!(returned_code.0, code.to_vec().encode()); |
There was a problem hiding this comment.
The assertion assert_eq!(returned_code.0, code.to_vec().encode()) might be incorrect. Encode::encode on a Vec<u8> adds a SCALE length prefix. If the RPC method code_getOriginal returns the raw WASM bytecode, the assertion will fail due to this extra prefix. It should likely be compared against code directly.
| assert_eq!(returned_code.0, code.to_vec().encode()); | |
| assert_eq!(returned_code.0, code.to_vec()); |
| }) | ||
| .collect(); | ||
|
|
||
| let rpc = BlackBoxRpc::start_with_db(db).await; |
There was a problem hiding this comment.
Starting and stopping a full RpcServer inside every iteration of a proptest is extremely inefficient and will significantly slow down the test suite. Consider initializing the server once outside the runner.run loop and reusing it, or using a more lightweight approach for property-based testing of the RPC logic.
No description provided.