Refactor error unpacking in test_validate_accounts_tx.

[ArniStarkware]

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a03627e) 70.61% compared to head (8f2c952) 70.61%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1218   +/-   ##
=======================================
  Coverage   70.61%   70.61%           
=======================================
  Files          56       56           
  Lines        7066     7066           
  Branches     7066     7066           
=======================================
  Hits         4990     4990           
  Misses       1658     1658           
  Partials      418      418           

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

[dorimedini-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @MohammadNassar1, and @noaov1)

---

crates/blockifier/src/test_utils.rs line 219 at r1 (raw file):

    } else {
        panic!("Unexpected structure for error: {:?}", error);
    }

does this work? error has the same type

Suggestion:

    match error {
        TransactionExecutionError::ValidateTransactionError(error)
        | TransactionExecutionError::ContractConstructorExecutionFailed(error) => {
            check_entry_point_execution_error_for_custom_hint(error, expected_hint);
        }
        _ => panic!("Unexpected structure for error: {:?}", error);
    }

---

crates/blockifier/src/test_utils.rs line 258 at r1 (raw file):

    ) = error
    {
    } else {

damn, this is the best we can do

Code quote:

    {
    } else {

---

crates/blockifier/src/transaction/transactions_test.rs line 1110 at r1 (raw file):

        create_account_tx_for_validate_test(tx_type, INVALID, None, &mut NonceManager::default());
    let error = account_tx.execute(state, block_context, true, true).unwrap_err();
    check_transaction_execution_error_for_diff_assert_values(&error);

the problem with this is: when you run a test and fail on this, the line number you'll see is in the test_utils module.
if this were a macro, you would see the more interesting line number (here).
Maybe turn this into a macro?

Code quote:

check_transaction_execution_error_for_diff_assert_values(

---

crates/blockifier/src/transaction/transactions_test.rs line 1119 at r1 (raw file):

        &mut NonceManager::default(),
    );
    let error: TransactionExecutionError =

delete annotation

[ArniStarkware]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @MohammadNassar1, and @noaov1)

---

crates/blockifier/src/test_utils.rs line 219 at r1 (raw file):

Previously, dorimedini-starkware wrote…

does this work? error has the same type

Done.

---

crates/blockifier/src/test_utils.rs line 258 at r1 (raw file):

Previously, dorimedini-starkware wrote…

damn, this is the best we can do

Indeed, :\

We could make it into a more generic function that checks what type of inner_exc we have - and checks it matches the return type.
Kind of share code between this function and check_entry_point_execution_error_for_custom_hint.
This is killing two birds with one stone. However, they were two birds, each chilling on a different tree.
Something like: #1220. (I don't like it - it seems like over-design)

---

crates/blockifier/src/transaction/transactions_test.rs line 1119 at r1 (raw file):

Previously, dorimedini-starkware wrote…

delete annotation

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @MohammadNassar1, and @noaov1)

---

crates/blockifier/src/test_utils.rs line 258 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Indeed, :\

We could make it into a more generic function that checks what type of inner_exc we have - and checks it matches the return type.
Kind of share code between this function and check_entry_point_execution_error_for_custom_hint.
This is killing two birds with one stone. However, they were two birds, each chilling on a different tree.
Something like: #1220. (I don't like it - it seems like over-design)

yeah I wouldn't jump through hoops here, the ugliness comes from not having shared error scheme with the VM lib IMO

[ArniStarkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware, @MohammadNassar1, and @noaov1)

---

crates/blockifier/src/transaction/transactions_test.rs line 1110 at r1 (raw file):

Previously, dorimedini-starkware wrote…

the problem with this is: when you run a test and fail on this, the line number you'll see is in the test_utils module.
if this were a macro, you would see the more interesting line number (here).
Maybe turn this into a macro?

Done.
Macros are beautiful, and my tiny brain can't handle them.

It makes the imports kind of confusing and repetitive IMO - but maybe it is just because I am not used to it.

[dorimedini-starkware]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1 and @noaov1)

---

crates/blockifier/src/test_utils.rs line 220 at r3 (raw file):

#[macro_export]
macro_rules! check_transaction_execution_error_for_custom_hint {
    ($error:expr, $expected_hint:expr) => {

you can add this if you want to be fancy and allow trailing commas when you call these macros.

Suggestion:

$error:expr, $expected_hint:expr $(,)?

---

crates/blockifier/src/transaction/transactions_test.rs line 1110 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Done.
Macros are beautiful, and my tiny brain can't handle them.

It makes the imports kind of confusing and repetitive IMO - but maybe it is just because I am not used to it.

to "solve" the invisible import issue you could use a full path to the objects you use, in the macro definition.
not sure which method I prefer, but looks good :)

[noaov1]

Reviewed 1 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @MohammadNassar1)

---

crates/blockifier/src/transaction/transactions_test.rs line 1171 at r3 (raw file):

        let account_tx = AccountTransaction::DeployAccount(deploy_account_tx);
        let error = account_tx.execute(state, block_context, true, true).unwrap_err();
        check_transaction_execution_error_for_custom_hint!(

I wonder if we care for the specific error type. Now, the test can pass even if we throw here TransactionExecutionError::ValidateTransactionError.

Code quote:

check_transaction_execution_error_for_custom_hint!(

[ArniStarkware]

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @dorimedini-starkware, @MohammadNassar1, and @noaov1)

---

crates/blockifier/src/test_utils.rs line 220 at r3 (raw file):

Previously, dorimedini-starkware wrote…

you can add this if you want to be fancy and allow trailing commas when you call these macros.

Done.

---

crates/blockifier/src/transaction/transactions_test.rs line 1171 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I wonder if we care for the specific error type. Now, the test can pass even if we throw here TransactionExecutionError::ValidateTransactionError.

Done.

[dorimedini-starkware]

Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

[dorimedini-starkware]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)