test(mempool): add tests for commit block method

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.21%. Comparing base (93de0bd) to head (aba4a78).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #396   +/-   ##
=======================================
  Coverage   81.21%   81.21%           
=======================================
  Files          42       42           
  Lines        1826     1826           
  Branches     1826     1826           
=======================================
  Hits         1483     1483           
  Misses        269      269           
  Partials       74       74           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

a discussion (no related file):
This PR adds tests for the commit_block method across various scenarios.
Since the method is not yet implemented, the tests are currently marked with #[ignore] to prevent them from running.

---

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @elintul)

a discussion (no related file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

This PR adds tests for the commit_block method across various scenarios.
Since the method is not yet implemented, the tests are currently marked with #[ignore] to prevent them from running.

Now, it adds a single test: a test without rejected or reverted transactions.

---

[elintul]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 427 at r3 (raw file):

#[rstest]
#[ignore]
fn test_commit_block_includes_all_transactions() {

Not sure the test case is clear for me from the name. Do you mean you test the case where the Q shouldn't be changed during commit block?

Code quote:

test_commit_block_includes_all_transactions

---

crates/mempool/src/mempool_test.rs line 428 at r3 (raw file):

#[ignore]
fn test_commit_block_includes_all_transactions() {
    let tx_address0_nonce4 = add_tx_input!(tip: 4, tx_hash: 1, sender_address: "0x0", tx_nonce: 4_u8, account_nonce: 4_u8).tx;

Suggestion:

fn test_commit_block_includes_all_transactions() {
    // Setup.
    let tx_address0_nonce4 = add_tx_input!(tip: 4, tx_hash: 1, sender_address: "0x0", tx_nonce: 4_u8, account_nonce: 4_u8).tx;

---

crates/mempool/src/mempool_test.rs line 431 at r3 (raw file):

    let tx_address0_nonce5 = add_tx_input!(tip: 3, tx_hash: 2, sender_address: "0x0", tx_nonce: 5_u8, account_nonce: 4_u8).tx;
    let tx_address1_nonce3 = add_tx_input!(tip: 2, tx_hash: 3, sender_address: "0x1", tx_nonce: 3_u8, account_nonce: 3_u8).tx;
    let tx_address2_nonce3 = add_tx_input!(tip: 1, tx_hash: 4, sender_address: "0x2", tx_nonce: 1_u8, account_nonce: 1_u8).tx;

Suggestion:

tx_address2_nonce1

---

crates/mempool/src/mempool_test.rs line 445 at r3 (raw file):

    let txs_iterator = pool_txs.iter().cloned();

    let mut mempool: Mempool = MempoolState::new(txs_iterator, tx_references_iterator).into();

Why is .cloned() needed for txs_iterator? Can it be avoided?

Suggestion:

    ];
    let tx_references_iterator = queued_txs.iter().map(TransactionReference::new);
    let txs_iterator = pool_txs.iter().cloned();
    let mut mempool: Mempool = MempoolState::new(txs_iterator, tx_references_iterator).into();

---

crates/mempool/src/mempool_test.rs line 447 at r3 (raw file):

    let mut mempool: Mempool = MempoolState::new(txs_iterator, tx_references_iterator).into();

    let commit_state = HashMap::from([

Suggestion:

// Test.
let state_changes = HashMap::from([

---

crates/mempool/src/mempool_test.rs line 453 at r3 (raw file):

    assert!(mempool.commit_block(commit_state).is_ok());

    assert_eq_mempool_queue(&mempool, &queued_txs)

Suggestion:

// Assert.
assert_eq_mempool_queue(&mempool, &queued_txs)

[elintul]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 445 at r3 (raw file):

Previously, elintul (Elin) wrote…

Why is .cloned() needed for txs_iterator? Can it be avoided?

In other words: how can we reduce transaction cloning to once per test?

[MohammadNassar1]

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @elintul)

---

crates/mempool/src/mempool_test.rs line 427 at r3 (raw file):

Previously, elintul (Elin) wrote…

Not sure the test case is clear for me from the name. Do you mean you test the case where the Q shouldn't be changed during commit block?

Exactly!
If the Batcher includes all the sent transactions, the block state would be equal to the mempool state, and thus, the Q wouldn't be changed.

---

crates/mempool/src/mempool_test.rs line 453 at r3 (raw file):

    assert!(mempool.commit_block(commit_state).is_ok());

    assert_eq_mempool_queue(&mempool, &queued_txs)

Done.

[elintul]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 447 at r3 (raw file):

    let mut mempool: Mempool = MempoolState::new(txs_iterator, tx_references_iterator).into();

    let commit_state = HashMap::from([

Please add the Test and Assert comments?
For structural consistency between tests.

[MohammadNassar1]

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

---

crates/mempool/src/mempool_test.rs line 445 at r3 (raw file):

Previously, elintul (Elin) wrote…

In other words: how can we reduce transaction cloning to once per test?

Done.

[elintul]

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 438 at r5 (raw file):

        TransactionReference::new(&tx_address1_nonce3),
        TransactionReference::new(&tx_address2_nonce1),
    ];

Can map be used?

Code quote:

    let queue_txs = [
        TransactionReference::new(&tx_address0_nonce4),
        TransactionReference::new(&tx_address1_nonce3),
        TransactionReference::new(&tx_address2_nonce1),
    ];

[elintul]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MohammadNassar1)

[elintul]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MohammadNassar1)

---

crates/mempool/src/mempool_test.rs line 427 at r5 (raw file):

#[rstest]
#[ignore]
fn test_commit_block_includes_all_transactions() {

Suggestion:

all_txs

[elintul]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

a discussion (no related file):
Please update commit message.

---