test(starknet_mempool_p2p): test gateway client error on adding tx from p2p #2138

AlonLStarkWare · 2024-11-18T12:35:39Z

test(mempool_p2p): change p2p runner test to use automock for gateway client
test(mempool_p2p): test gateway client error on adding tx from p2p

reviewable-StarkWare · 2024-11-18T12:35:49Z

This change is

ShahakShama

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @AlonLStarkWare)

crates/starknet_mempool_p2p/src/runner/test.rs line 81 at r2 (raw file):

        mock_register_broadcast_topic().expect("Failed to create mock network");
    let BroadcastTopicChannels { broadcasted_messages_receiver, broadcast_topic_client } =
        subscriber_channels; // used to created our node's p2p runner below, which will listen for incoming txs over broadcasted_messages_receiver.

Same as before, try to reduce the amount of comments

crates/starknet_mempool_p2p/src/runner/test.rs line 92 at r2 (raw file):

    // send an empty message on this channel to indicate that the tx reached the gateway client.
    let (add_tx_indicator_sender, add_tx_indicator_receiver) = futures::channel::oneshot::channel();

You don't need this. You can wait for the peer report instead

crates/starknet_mempool_p2p/src/runner/test.rs line 124 at r2 (raw file):

    // if the runner fails, there was a network issue => panic.
    // if the runner returns successfully, we panic because the runner should never terminate.
    res = tokio::time::timeout(Duration::from_secs(5), mempool_p2p_runner.start()) => {res.expect("Test timed out").expect("Runner failed - network stopped unexpectedly"); panic!("Runner terminated")},

Same as before. newlines and MempoolP2pRunner

crates/starknet_mempool_p2p/src/runner/test.rs line 130 at r2 (raw file):

    res.unwrap();
    // After gateway client fails to add the tx, the p2p runner should have reported the peer.
    let peer_reported = mock_reported_messages_receiver.next().await.expect("Failed to receive message");

As I said before, put this future in the select arm instead

crates/starknet_mempool_p2p/src/runner/test.rs line 130 at r2 (raw file):

    res.unwrap();
    // After gateway client fails to add the tx, the p2p runner should have reported the peer.
    let peer_reported = mock_reported_messages_receiver.next().await.expect("Failed to receive message");

Failed to receive report

crates/starknet_mempool_p2p/src/runner/test.rs line 131 at r2 (raw file):

    // After gateway client fails to add the tx, the p2p runner should have reported the peer.
    let peer_reported = mock_reported_messages_receiver.next().await.expect("Failed to receive message");
    assert_eq!(peer_reported, message_metadata.originator_id.private_get_peer_id())

add a TODO to add this functionality to network manager test utils

crates/starknet_mempool_p2p/src/runner/test.rs line 29 at r1 (raw file):

        mock_register_broadcast_topic().expect("Failed to create mock network");
    let BroadcastTopicChannels { broadcasted_messages_receiver, broadcast_topic_client } =
        subscriber_channels; // used to created our node's p2p runner below, which will listen for incoming txs over broadcasted_messages_receiver.

This comment seems unnecessary

crates/starknet_mempool_p2p/src/runner/test.rs line 33 at r1 (raw file):

        broadcasted_messages_sender: mut mock_broadcasted_messages_sender,
        ..
    } = mock_network; // other node sending tx to our p2p runner

This comment seems unnecessary

crates/starknet_mempool_p2p/src/runner/test.rs line 38 at r1 (raw file):

    let placeholder_network_manager = NetworkManager::new(NetworkConfig::default(), None);

    // send an empty message on this channel to indicate that the tx reached the gateway client.

This comment is incorrect. Change it to "Creating channels for sending ..."

crates/starknet_mempool_p2p/src/runner/test.rs line 42 at r1 (raw file):

    let mut mock_gateway_client = MockGatewayClient::new();
    mock_gateway_client.expect_add_tx().return_once(move |_| {

You should assert that the transaction you've received is the one you've gotten from p2p. You can use automock's with function for that: https://docs.rs/mockall/latest/mockall/index.html#getting-started

crates/starknet_mempool_p2p/src/runner/test.rs line 48 at r1 (raw file):

    let mut mempool_p2p_runner = MempoolP2pRunner::new(
        Some(placeholder_network_manager),
        broadcasted_messages_receiver, // listen to incoming tx

This comment seems unnecessary

crates/starknet_mempool_p2p/src/runner/test.rs line 49 at r1 (raw file):

        Some(placeholder_network_manager),
        broadcasted_messages_receiver, // listen to incoming tx
        broadcast_topic_client,        // broadcast tx or report peer

This comment seems unnecessary

crates/starknet_mempool_p2p/src/runner/test.rs line 64 at r1 (raw file):

    tokio::select! {
        // if the runner takes longer than 5 seconds to start, we panic.

This comment seems unnecessary. You have the expect for that

crates/starknet_mempool_p2p/src/runner/test.rs line 67 at r1 (raw file):

        // if the runner fails, there was a network issue => panic.
        // if the runner returns successfully, we panic because the runner should never terminate.
        res = tokio::time::timeout(Duration::from_secs(5), mempool_p2p_runner.start()) => {res.expect("Test timed out").expect("Runner failed - network stopped unexpectedly"); panic!("Runner terminated")},

Runner -> MempoolP2pRunner

crates/starknet_mempool_p2p/src/runner/test.rs line 67 at r1 (raw file):

        // if the runner fails, there was a network issue => panic.
        // if the runner returns successfully, we panic because the runner should never terminate.
        res = tokio::time::timeout(Duration::from_secs(5), mempool_p2p_runner.start()) => {res.expect("Test timed out").expect("Runner failed - network stopped unexpectedly"); panic!("Runner terminated")},

add newlines after the { and after each ;

crates/starknet_mempool_p2p/src/runner/test.rs line 68 at r1 (raw file):

        // if the runner returns successfully, we panic because the runner should never terminate.
        res = tokio::time::timeout(Duration::from_secs(5), mempool_p2p_runner.start()) => {res.expect("Test timed out").expect("Runner failed - network stopped unexpectedly"); panic!("Runner terminated")},
        // if a message was received on this oneshot channel, the gateway client received the tx.

... and the test succeeded

github-actions · 2024-11-21T11:05:21Z

Artifacts upload triggered. View details here

github-actions · 2024-11-21T11:05:29Z

Artifacts upload triggered. View details here

AlonLStarkWare

Reviewable status: 0 of 3 files reviewed, 16 unresolved discussions (waiting on @ShahakShama)

crates/starknet_mempool_p2p/src/runner/test.rs line 29 at r1 (raw file):