feat: Refactor assert_tx_failed into a context #3706

DanielSchiavini · 2023-12-21T19:34:33Z

PR based on #3704

What I did

Converted asset_tx_failed into a context
Updated all tests using it

How I did it

Replacing a callback by a context

How to verify it

Tests should continue passing

Commit message

Convert assert_tx_failed into tx_failed context

Description for the changelog

n/a

Cute Animal Picture

…ncies

…iled

codecov-commenter · 2023-12-21T20:09:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (88c09a2) 83.92% compared to head (219bfce) 83.95%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3706      +/-   ##
==========================================
+ Coverage   83.92%   83.95%   +0.03%     
==========================================
  Files          92       92              
  Lines       13065    13065              
  Branches     2934     2934              
==========================================
+ Hits        10965    10969       +4     
+ Misses       1666     1662       -4     
  Partials      434      434

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

charles-cooper · 2023-12-23T15:51:07Z

tests/functional/builtins/codegen/test_minmax.py

@@ -198,7 +198,7 @@ def foo() -> uint256:


 def test_minmax_var_uint256_negative_int128(
-    get_contract_with_gas_estimation, assert_tx_failed, assert_compile_failed
+    get_contract_with_gas_estimation, tx_failed, assert_compile_failed


note: looks like the fixture isn't actually used in this test

charles-cooper · 2023-12-23T15:51:50Z

tests/functional/builtins/codegen/test_raw_call.py



-def test_checkable_raw_call(get_contract, assert_tx_failed):
+def test_checkable_raw_call(get_contract, tx_failed):


looks like the fixture is not actually used

charles-cooper · 2023-12-23T15:56:24Z

tests/functional/codegen/calling_convention/test_external_contract_calls.py

+    with tx_failed(exception=StructureException):
+        get_contract(contract)


could be

Suggested change

with tx_failed(exception=StructureException):

get_contract(contract)

with pytest.raises(StructureException):

compile_code(contract)

charles-cooper · 2023-12-23T16:02:47Z

tests/functional/codegen/features/decorators/test_private.py

@@ -449,7 +449,7 @@ def whoami() -> address:
    assert logged_addr == addr, "oh no"


-def test_nested_static_params_only(get_contract, assert_tx_failed):
+def test_nested_static_params_only(get_contract, tx_failed):


note: unused fixture

charles-cooper · 2023-12-23T16:08:17Z

tests/functional/codegen/features/test_logging.py

@@ -193,7 +194,7 @@ def bar():


 def test_event_logging_cannot_have_more_than_three_topics(
-    assert_tx_failed, get_contract_with_gas_estimation
+    tx_failed, get_contract_with_gas_estimation


note: get_contract_with_gas_estimation no longer used

tx_failed is also no logner used

charles-cooper · 2023-12-23T16:10:03Z

tests/functional/codegen/features/test_logging.py

@@ -962,7 +966,7 @@ def set_list():
    ]


-def test_logging_fails_when_input_is_too_big(assert_tx_failed, get_contract_with_gas_estimation):
+def test_logging_fails_when_input_is_too_big(tx_failed, get_contract_with_gas_estimation):


most of the tests in this file can be rewritten with with pytest.raises(...): compile_code(...)

charles-cooper · 2023-12-23T16:10:31Z

tests/functional/codegen/features/test_reverting.py

-        TransactionFailed,
-        exc_text=f"execution reverted: {revert_bytes}",
-    )
+    with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):


unnecessary specification of TransactionFailed

charles-cooper · 2023-12-23T16:10:52Z

tests/functional/codegen/features/test_reverting.py

-        TransactionFailed,
-        exc_text=f"execution reverted: {revert_bytes}",
-    )
+    with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):


unnecessary specification of TransactionFailed

charles-cooper · 2023-12-23T16:12:24Z

tests/functional/codegen/features/test_reverting.py

-        exc_text=f"execution reverted: {revert_bytes}",
-    )
+    with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):
+        get_contract_with_gas_estimation(reverty_code).foo(transact={})


this would be better written over multiple lines --

c = get_contract(reverty_code) with tx_failed(exc_text=...): c.foo(transact={})

charles-cooper · 2023-12-23T16:12:46Z

tests/functional/codegen/features/test_reverting.py

-        exc_text=f"execution reverted: {revert_bytes}",
-    )
+    with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):
+        get_contract_with_gas_estimation(reverty_code).foo(transact={})


would be clearer if rewritten over multiple lines

charles-cooper · 2023-12-23T16:13:39Z

tests/functional/codegen/features/test_reverting.py

+    with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):
+        get_contract_with_gas_estimation(reverty_code).foo(transact={})


TransactionFailed + multiple side effects on one line

charles-cooper · 2023-12-23T16:18:01Z

tests/functional/codegen/types/numbers/test_signed_ints.py

            assert_compile_failed(lambda code=code_2: get_contract(code), ZeroDivisionException)
-            assert_tx_failed(lambda p=y, code=code_3: get_contract(code).foo(p))
+            with tx_failed():
+                get_contract(code_3).foo(y)


multiple side effects on one line

charles-cooper · 2023-12-23T16:18:09Z

tests/functional/codegen/types/numbers/test_signed_ints.py

+            with tx_failed():
+                c.foo(x, y)
+            with tx_failed():
+                get_contract(code_2).foo(x)


multiple side effects on one line

charles-cooper · 2023-12-23T16:32:57Z

tests/functional/examples/safe_remote_purchase/test_safe_remote_purchase.py

    # Check if buyer can call receive
    c.received(transact={"from": a1})
    # Final check if everything worked. 1 value has been transferred
    assert get_balance() == (init_bal_a0 + 1, init_bal_a1 - 1)


-def test_received_reentrancy(w3, get_contract, assert_tx_failed, get_balance, contract_code):
+def test_received_reentrancy(w3, get_contract, tx_failed, get_balance, contract_code):


unused fixture

charles-cooper · 2023-12-23T16:38:27Z

tests/functional/examples/tokens/test_erc1155.py

-    )
-    assert_tx_failed(
-        lambda: erc1155.balanceOfBatch([a2, a2, a2], [21, 22], transact={"from": owner})
-        == [1, 1, 1]


kind of weird stray check, nice catch

charles-cooper · 2023-12-23T16:43:21Z

tests/functional/examples/tokens/test_erc1155.py


-    assert lambda: erc1155.balanceOfBatch([a3, a3, a3], [1, 2, 3]) == [0, 0, 1]
+    assert erc1155.balanceOfBatch([a3, a3, a3], [1, 2, 3]) == [0, 0, 0]


hmm, interesting catch

yeah, good catch, will check the rest of the codebase that we don't have such uncalled lambdas elsewhere

charles-cooper · 2023-12-23T16:53:10Z

tests/functional/builtins/codegen/test_as_wei_value.py



 @pytest.mark.parametrize("denom,multiplier", wei_denoms.items())
 @pytest.mark.parametrize("data_type", ["decimal", "int128", "uint256"])
-def test_zero_value(get_contract, assert_tx_failed, denom, multiplier, data_type):
+def test_zero_value(get_contract, tx_failed, denom, multiplier, data_type):


unused fixture

charles-cooper · 2023-12-23T16:57:08Z

tests/functional/codegen/features/iteration/test_range_in.py

@@ -154,4 +155,5 @@ def testin(x: address) -> bool:
        return True
    return False
 """
-    assert_tx_failed(lambda: get_contract_with_gas_estimation(code), TypeMismatch)
+    with tx_failed(TypeMismatch):


with pytest.raises(...): compile_code(...)

charles-cooper · 2023-12-23T16:57:23Z

tests/functional/codegen/features/test_assert.py

@@ -8,7 +8,7 @@ def _fixup_err_str(s):
    return s.replace("execution reverted: ", "")


-def test_assert_refund(w3, get_contract_with_gas_estimation, assert_tx_failed):
+def test_assert_refund(w3, get_contract_with_gas_estimation, tx_failed):


unused fixture

charles-cooper · 2023-12-23T16:57:36Z

tests/functional/codegen/features/test_assert.py

@@ -26,7 +26,7 @@ def foo():
    assert tx_receipt["gasUsed"] < gas_sent


-def test_assert_reason(w3, get_contract_with_gas_estimation, assert_tx_failed, memory_mocker):
+def test_assert_reason(w3, get_contract_with_gas_estimation, tx_failed, memory_mocker):


unused fixture?

charles-cooper · 2023-12-23T16:59:02Z

tests/functional/codegen/features/test_logging.py

@@ -565,10 +565,11 @@ def foo_():
    log MyLog(b'yo')
 """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), InvalidType)
+    with tx_failed(InvalidType):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:14Z

tests/functional/codegen/features/test_logging.py

@@ -579,12 +580,11 @@ def foo():
    log MyLog(b'bars')
 """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), InvalidType)
+    with tx_failed(InvalidType):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:21Z

tests/functional/codegen/features/test_logging.py

@@ -594,10 +594,11 @@ def foo(arg1: Bytes[4]):
    log MyLog(arg1)
 """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), TypeMismatch)
+    with tx_failed(TypeMismatch):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:28Z

tests/functional/codegen/features/test_logging.py

@@ -607,12 +608,11 @@ def foo():
    log MyLog(b'bars')
 """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), InvalidType)
+    with tx_failed(InvalidType):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:35Z

tests/functional/codegen/features/test_logging.py

@@ -622,7 +622,8 @@ def foo(arg1: Bytes[4]):
    log MyLog(arg1)
 """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), TypeMismatch)
+    with tx_failed(TypeMismatch):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:47Z

tests/functional/codegen/features/test_logging.py

-    assert_tx_failed(
-        lambda: get_contract_with_gas_estimation(loggy_code), EventDeclarationException
-    )
+    with tx_failed(EventDeclarationException):


pytest.raises/compile_code

charles-cooper · 2023-12-23T16:59:56Z

tests/functional/codegen/features/test_logging.py

@@ -665,23 +665,23 @@ def foo():
    log MyLog()
    """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), NamespaceCollision)
+    with tx_failed(NamespaceCollision):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:03Z

tests/functional/codegen/features/test_logging.py

    loggy_code = """

 @external
 def foo():
    log MyLog()
    """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), UndeclaredDefinition)
+    with tx_failed(UndeclaredDefinition):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:09Z

tests/functional/codegen/features/test_logging.py

@@ -691,10 +691,11 @@ def foo():
    log MyLog(self)
    """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), TypeMismatch)
+    with tx_failed(TypeMismatch):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:16Z

tests/functional/codegen/features/test_logging.py

@@ -704,11 +705,12 @@ def foo():
    log MyLog(self)
    """

-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), TypeMismatch)
+    with tx_failed(TypeMismatch):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:24Z

tests/functional/codegen/features/test_logging.py

@@ -718,11 +720,12 @@ def test_logging_fails_when_number_of_arguments_is_greater_than_declaration(
 def foo():
    log MyLog(1, 2)
 """
-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), ArgumentException)
+    with tx_failed(ArgumentException):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:32Z

tests/functional/codegen/features/test_logging.py

@@ -733,7 +736,8 @@ def test_logging_fails_when_number_of_arguments_is_less_than_declaration(
 def foo():
    log MyLog(1)
 """
-    assert_tx_failed(lambda: get_contract_with_gas_estimation(loggy_code), ArgumentException)
+    with tx_failed(ArgumentException):


pytest.raises/compile_code

charles-cooper · 2023-12-23T17:00:39Z

tests/functional/codegen/features/test_logging.py

@@ -971,7 +975,8 @@ def test_logging_fails_when_input_is_too_big(assert_tx_failed, get_contract_with
 def foo(inp: Bytes[33]):
    log Bar(inp)
 """
-    assert_tx_failed(lambda: get_contract_with_gas_estimation(code), TypeMismatch)
+    with tx_failed(TypeMismatch):


pytest.raises/compile_code

charles-cooper

approving and merging this -- i reviewed for semantic changes to the tests. i found two examples (#3706 (comment), #3706 (comment)), but these seem to be fixes to existing mistakes in the tests. otherwise, everything looks clean.

@tserg and @cyberthirst if you could look over this PR for any potential breaking semantic changes to the tests, that would be very helpful.

i also found a bunch of potential style/performance improvements, e.g. there are a lot of places where with tx_failed(<compiler exception>): get_contract(code) can be rewritten with with pytest.raises(<compiler exception>): compile_code(code). however, since this PR was somewhat hard to review (and also to prevent potential merge conflicts / blocking other PRs) i'd rather have these fixed in a follow-up PR please @DanielSchiavini.

tserg

I had a look through and did not spot any other breaking semantic changes to the tests.

tserg · 2023-12-24T01:43:54Z

tests/functional/builtins/codegen/test_as_wei_value.py



 @pytest.mark.parametrize("denom,multiplier", wei_denoms.items())
-def test_wei_int128(get_contract, assert_tx_failed, denom, multiplier):
+def test_wei_int128(get_contract, tx_failed, denom, multiplier):


unused fixture

tserg · 2023-12-24T01:44:01Z

tests/functional/builtins/codegen/test_as_wei_value.py

@@ -54,7 +55,7 @@ def foo(a: int128) -> uint256:


 @pytest.mark.parametrize("denom,multiplier", wei_denoms.items())
-def test_wei_decimal(get_contract, assert_tx_failed, denom, multiplier):
+def test_wei_decimal(get_contract, tx_failed, denom, multiplier):


unused fixture

tserg · 2023-12-24T01:49:29Z

tests/functional/codegen/calling_convention/test_default_function.py



-def test_another_zero_method_id(w3, get_logs, get_contract, assert_tx_failed):
+def test_another_zero_method_id(w3, get_logs, get_contract, tx_failed):


unused fixture

tserg · 2023-12-24T01:58:21Z

tests/functional/codegen/features/test_logging.py

@@ -193,7 +194,7 @@ def bar():


 def test_event_logging_cannot_have_more_than_three_topics(
-    assert_tx_failed, get_contract_with_gas_estimation
+    tx_failed, get_contract_with_gas_estimation


tx_failed is also no logner used

tserg · 2023-12-24T02:10:50Z

tests/functional/examples/tokens/test_erc1155.py

    assert erc1155.balanceOf(a1, 21) == 0


 # TODO: mint 20 NFTs [1:20] and check the balance for each
-def test_mintBatch_balanceOf(erc1155, w3, assert_tx_failed):  # test_mint_batch
+def test_mintBatch_balanceOf(erc1155, w3, tx_failed):  # test_mint_batch


unused fixture

cyberthirst · 2023-12-26T13:04:03Z

tests/functional/builtins/folding/test_abs.py



-def test_abs_lower_bound_folded(get_contract, assert_tx_failed):
+def test_abs_lower_bound_folded(get_contract, tx_failed):


unused fixture

cyberthirst · 2023-12-26T13:33:30Z

tests/functional/codegen/calling_convention/test_return_tuple.py

@@ -152,11 +153,11 @@ def test3() -> (address, int128):
    assert c.test3() == [c.out_literals()[2], 1]


-def test_tuple_return_typecheck(assert_tx_failed, get_contract_with_gas_estimation):
+def test_tuple_return_typecheck(tx_failed, get_contract_with_gas_estimation):


unused fixture

cyberthirst · 2023-12-27T08:13:21Z

Didn't spot any additional semantics-breaking changes.

rename `assert_tx_failed` to `tx_failed` and change it into a context manager which has a similar API to `pytest.raises()`. --------- Co-authored-by: Charles Cooper <[email protected]>

DanielSchiavini added 12 commits December 20, 2023 16:25

Update lint dependencies

b6a63ad

Merge branch 'master' of github.com:vyperlang/vyper into lint-depende…

b028b0c

…ncies

Fix tests and review comment

0920eb6

Refactor assert_tx_failed

19aff28

Inline search_for_sublist

7147671

Rename fixture

a0d1d58

Lint fix

be72dec

Review

cd07723

Review comments

0abe8cc

Review comments

e4182f2

Review comments

cde70ba

Review comments

3abae3d

DanielSchiavini mentioned this pull request Dec 21, 2023

feat: Refactor assert_tx_failed into a context DanielSchiavini/vyper#2

Closed

DanielSchiavini added 2 commits December 21, 2023 20:36

Merge branch 'master' of github.com:vyperlang/vyper into assert_tx_fa…

87c220d

…iled

Linting

5a46f88

Update expectation

09f6009

charles-cooper reviewed Dec 23, 2023

View reviewed changes

charles-cooper approved these changes Dec 23, 2023

View reviewed changes

charles-cooper added 2 commits December 23, 2023 12:09

Merge branch 'master' into assert_tx_failed

0b5e2b7

remove remaining assert_tx_failed

219bfce

charles-cooper enabled auto-merge (squash) December 23, 2023 17:15

charles-cooper merged commit 2e41873 into vyperlang:master Dec 23, 2023
84 checks passed

tserg reviewed Dec 24, 2023

View reviewed changes

cyberthirst reviewed Dec 26, 2023

View reviewed changes



		def test_checkable_raw_call(get_contract, assert_tx_failed):
		def test_checkable_raw_call(get_contract, tx_failed):

		with tx_failed(exception=StructureException):
		get_contract(contract)

		with tx_failed(TransactionFailed, exc_text=f"execution reverted: {revert_bytes}"):
		get_contract_with_gas_estimation(reverty_code).foo(transact={})


		assert lambda: erc1155.balanceOfBatch([a3, a3, a3], [1, 2, 3]) == [0, 0, 1]
		assert erc1155.balanceOfBatch([a3, a3, a3], [1, 2, 3]) == [0, 0, 0]



		def test_another_zero_method_id(w3, get_logs, get_contract, assert_tx_failed):
		def test_another_zero_method_id(w3, get_logs, get_contract, tx_failed):



		def test_abs_lower_bound_folded(get_contract, assert_tx_failed):
		def test_abs_lower_bound_folded(get_contract, tx_failed):

feat: Refactor assert_tx_failed into a context #3706

feat: Refactor assert_tx_failed into a context #3706

Conversation

DanielSchiavini commented Dec 21, 2023

What I did

How I did it

How to verify it

Commit message

Description for the changelog

Cute Animal Picture

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charles-cooper left a comment

Choose a reason for hiding this comment

tserg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberthirst commented Dec 27, 2023

codecov-commenter commented Dec 21, 2023 •

edited

Loading