test(mempool): refactor test_add_tx_with_duplicate_tx to use mempool …

[ayeletstarkware]

…state

---

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.21%. Comparing base (f1d94c0) to head (4dca6f6).

Additional details and impacted files

@@                          Coverage Diff                           @@
##           ayelet/mempool/mempool-state/refactor     #444   +/-   ##
======================================================================
  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.

[giladchase]

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

---

crates/mempool/src/mempool_test.rs line 213 at r1 (raw file):

    // Assert that the original tx remains in Mempool after the failed attempt.
    let queue_txs = [TransactionReference::new(&input.tx)];

I don't think asserting the queue is relevant for this test.

If the duplicate tx isn't in the pool then we can assume it's not queued, and whether or not the non-dup tx is in the queue shouldn't matter for a dup-check test.

There should be an easy way of only checking the pool.

Code quote:

    let queue_txs = [TransactionReference::new(&input.tx)];

[ayeletstarkware]

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

---

crates/mempool/src/mempool_test.rs line 213 at r1 (raw file):

Previously, giladchase wrote…

I don't think asserting the queue is relevant for this test.

If the duplicate tx isn't in the pool then we can assume it's not queued, and whether or not the non-dup tx is in the queue shouldn't matter for a dup-check test.

There should be an easy way of only checking the pool.

I see your point, and it's easy to implement, but what is the issue with checking the entire mempool state?

[giladchase]

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

---

crates/mempool/src/mempool_test.rs line 213 at r1 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

I see your point, and it's easy to implement, but what is the issue with checking the entire mempool state?

Why use a complicated assertion when a simple one suffices?

A more complicated solution to a simple problem can confuse readers, who might think that indeed there is a case in which tx is not in the pool but is in the queue, otherwise, why check both?

[giladchase]

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

---

crates/mempool/src/mempool_test.rs line 213 at r1 (raw file):

Previously, giladchase wrote…

Why use a complicated assertion when a simple one suffices?

A more complicated solution to a simple problem can confuse readers, who might think that indeed there is a case in which tx is not in the pool but is in the queue, otherwise, why check both?

There is also consistency to consider.
If you do it for a test that doesn't need it, the principle of consistency will encourage future test writers to also assert this.
This might end up costing us with doesn't of extra assertions in our test files that are essentially tautologies.

[ayeletstarkware]

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

---

crates/mempool/src/mempool_test.rs line 213 at r1 (raw file):

Previously, giladchase wrote…

There is also consistency to consider.
If you do it for a test that doesn't need it, the principle of consistency will encourage future test writers to also assert this.
This might end up costing us with doesn't of extra assertions in our test files that are essentially tautologies.

Done.

[elintul]

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

a discussion (no related file):
Please impl. all my comments from other test( PR)s here.

---

[elintul]

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware, @giladchase, and @MohammadNassar1)

a discussion (no related file):
Can you please go over all tests that use MempoolState and check that they are tight? I.e., do not check already covered cases.

---

OOO

[ayeletstarkware]

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

a discussion (no related file):

Previously, elintul (Elin) wrote…

Can you please go over all tests that use MempoolState and check that they are tight? I.e., do not check already covered cases.

I wrote myself to do as a next task.

---

[MohammadNassar1]

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

[elintul]

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

---

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

#[rstest]
fn test_add_tx_with_duplicate_tx(mut mempool: Mempool) {
    // Setup

Suggestion:

// Setup.

---

crates/mempool/src/mempool_test.rs line 305 at r4 (raw file):

    let duplicate_input = input.clone();

    // Test: assert that the duplicate tx is not added to the mempool.

Code is clear enough.

Suggestion:

// Test.

---

crates/mempool/src/mempool_test.rs line 312 at r4 (raw file):

    );

    // Assert: the original tx remains in Mempool after the failed attempt.

Suggestion:

// Assert: the original tx remains.

[elintul]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

---

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

    );

    // Assert: the original tx remains.

No abbreviations in documentation.

Suggestion:

transaction

[elintul]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

[elintul]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)