tests(ethexe/processor): test that injected vs events tasks are processed in correct order#5248
tests(ethexe/processor): test that injected vs events tasks are processed in correct order#5248
Conversation
…ssed in correct order
playX18
added
A0-pleasereview
|
Review changes with |
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 introduces a critical new test to the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request reorders the processing steps within the ethexe/processor module, specifically ensuring that injected transactions and block events are handled before scheduled tasks. A new test was introduced to verify the execution order of injected transactions and block events. However, the new test has a verification gap as it directly calls process_queues instead of the higher-level process_programs function, which orchestrates the reordering logic. This means the test does not fully cover the behavior change introduced in lib.rs, which is a violation of the repository's style guide regarding verification expectations for behavior changes.
| async fn injected_and_block_events_execute_in_correct_order() { | ||
| const INJECTED_COUNT: usize = 10; | ||
| const CANONICAL_COUNT: usize = 10; | ||
|
|
||
| init_logger(); | ||
|
|
||
| let (promise_out_tx, mut promise_receiver) = mpsc::unbounded_channel(); | ||
| let (mut processor, chain, [code_id]) = setup_test_env_and_load_codes([demo_ping::WASM_BINARY]); | ||
| let block1 = chain.blocks[1].to_simple(); | ||
|
|
||
| let canonical_user = ActorId::from(10); | ||
| let injected_user = ActorId::from(20); | ||
| let actor_id = ActorId::from(0x10000); | ||
|
|
||
| // -- Step 1: create and initialise the program -- | ||
| let mut handler = setup_handler(processor.db.clone(), block1); | ||
|
|
||
| handler | ||
| .handle_router_event(RouterRequestEvent::ProgramCreated(ProgramCreatedEvent { | ||
| actor_id, | ||
| code_id, | ||
| })) | ||
| .expect("failed to create new program"); | ||
|
|
||
| handler | ||
| .handle_mirror_event( | ||
| actor_id, | ||
| MirrorRequestEvent::ExecutableBalanceTopUpRequested( | ||
| ExecutableBalanceTopUpRequestedEvent { | ||
| value: 500_000_000_000_000, | ||
| }, | ||
| ), | ||
| ) | ||
| .expect("failed to top up balance"); | ||
|
|
||
| handler | ||
| .handle_mirror_event( | ||
| actor_id, | ||
| MirrorRequestEvent::MessageQueueingRequested(MessageQueueingRequestedEvent { | ||
| id: H256::random().0.into(), | ||
| source: canonical_user, | ||
| payload: b"INIT".to_vec(), | ||
| value: 0, | ||
| call_reply: false, | ||
| }), | ||
| ) | ||
| .expect("failed to send init message"); | ||
|
|
||
| handler.transitions = processor | ||
| .process_queues(handler.transitions, block1, DEFAULT_BLOCK_GAS_LIMIT, None) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // -- Step 2: queue block events (canonical) BEFORE injected -- | ||
| // Even though canonical events are queued first, injected messages | ||
| // must still be processed before canonical ones. | ||
| for _ in 0..CANONICAL_COUNT { | ||
| handler | ||
| .handle_mirror_event( | ||
| actor_id, | ||
| MirrorRequestEvent::MessageQueueingRequested(MessageQueueingRequestedEvent { | ||
| id: H256::random().0.into(), | ||
| source: canonical_user, | ||
| payload: b"PING".to_vec(), | ||
| value: 0, | ||
| call_reply: false, | ||
| }), | ||
| ) | ||
| .expect("failed to queue canonical message"); | ||
| } | ||
|
|
||
| let mut tx_hashes = vec![]; | ||
| for _ in 0..INJECTED_COUNT { | ||
| let tx = injected(actor_id, b"PING", 0); | ||
| tx_hashes.push(tx.to_hash()); | ||
| handler | ||
| .handle_injected_transaction(injected_user, tx) | ||
| .expect("failed to queue injected transaction"); | ||
| } | ||
|
|
||
| // -- Step 3: process all queues -- | ||
| let transitions = processor | ||
| .process_queues( | ||
| handler.transitions, | ||
| block1, | ||
| DEFAULT_BLOCK_GAS_LIMIT, | ||
| Some(promise_out_tx), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // -- Step 4: verify promises for every injected transaction -- | ||
| for tx_hash in &tx_hashes { | ||
| let promise = promise_receiver | ||
| .recv() | ||
| .await | ||
| .expect("expected promise for injected transaction"); | ||
|
|
||
| assert_eq!(&promise.tx_hash, tx_hash); | ||
| assert_eq!( | ||
| promise.reply.code, | ||
| ReplyCode::Success(SuccessReplyReason::Manual), | ||
| ); | ||
| assert_eq!(promise.reply.payload, b"PONG"); | ||
| } | ||
|
|
||
| // -- Step 5: verify ordering in outgoing messages -- | ||
| // Skip the very first message which is the INIT reply. | ||
| // All injected replies (destination == injected_user) must appear | ||
| // before any canonical reply (destination == canonical_user). | ||
| let messages: Vec<_> = transitions.current_messages().into_iter().skip(1).collect(); | ||
|
|
||
| let mut injected_count = 0; | ||
| let mut canonical_count = 0; | ||
| let mut seen_canonical = false; | ||
|
|
||
| for (_, message) in &messages { | ||
| if message.destination == injected_user { | ||
| injected_count += 1; | ||
| assert!( | ||
| !seen_canonical, | ||
| "Injected reply appeared after a canonical reply — \ | ||
| injected messages must be fully processed first" | ||
| ); | ||
| } else if message.destination == canonical_user { | ||
| canonical_count += 1; | ||
| seen_canonical = true; | ||
| } | ||
| } | ||
|
|
||
| assert_eq!( | ||
| injected_count, INJECTED_COUNT, | ||
| "Expected {INJECTED_COUNT} injected replies" | ||
| ); | ||
| assert_eq!( | ||
| canonical_count, CANONICAL_COUNT, | ||
| "Expected {CANONICAL_COUNT} canonical replies" | ||
| ); | ||
| } |
There was a problem hiding this comment.
This test is intended to verify the correct processing order of injected transactions and block events. However, it calls processor.process_queues directly, bypassing the main processor.process_programs function where the ordering logic between handle_injected_and_events, process_tasks, and process_queues is defined.
The recent change in ethexe/processor/src/lib.rs reordered process_tasks and handle_injected_and_events within process_programs. This test does not cover that change, creating a verification gap, which is against the repository's style guide (see rule on Verification Expectations).
To properly test the end-to-end processing order, this test should be refactored to call processor.process_programs. This would involve:
- Collecting the
BlockRequestEvents and signedInjectedTransactions instead of callinghandler.handle_mirror_eventandhandler.handle_injected_transaction. - Constructing an
ExecutableDatastruct with these events and transactions. - Calling
processor.process_programswith thisExecutableData. - Asserting on the resulting
FinalizedBlockTransitions.
This would ensure that the test validates the overall processing logic, including the recent reordering, and not just the behavior of process_queues in isolation.
References
- The style guide requires that behavior changes are accompanied by tests. The logic change in
lib.rsis a behavior change that is not covered by the new test, creating a verification gap. (link)
2 participants
No description provided.