From cadf7f8215e69f085cb20a3ee2d54a1cf8509559 Mon Sep 17 00:00:00 2001 From: fselmo Date: Wed, 24 Jul 2024 12:02:57 -0600 Subject: [PATCH 1/6] Add ``@flaky_geth_dev_mining`` to receipt unmined tests --- web3/_utils/module_testing/eth_module.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/web3/_utils/module_testing/eth_module.py b/web3/_utils/module_testing/eth_module.py index 0a7e92bad0..b8f87d498a 100644 --- a/web3/_utils/module_testing/eth_module.py +++ b/web3/_utils/module_testing/eth_module.py @@ -1692,6 +1692,7 @@ async def test_async_eth_get_transaction_receipt_mined( assert isinstance(effective_gas_price, int) assert effective_gas_price > 0 + @flaky_geth_dev_mining @pytest.mark.asyncio async def test_async_eth_get_transaction_receipt_unmined( self, @@ -1757,6 +1758,7 @@ async def test_async_eth_wait_for_transaction_receipt_mined( assert isinstance(effective_gas_price, int) assert effective_gas_price > 0 + @flaky_geth_dev_mining @pytest.mark.asyncio async def test_async_eth_wait_for_transaction_receipt_unmined( self, @@ -3596,7 +3598,6 @@ def test_eth_replace_transaction_gas_price_defaulting_strategy_lower( self, w3: "Web3", keyfile_account_address: ChecksumAddress ) -> None: gas_price = w3.to_wei(2, "gwei") - txn_params: TxParams = { "from": keyfile_account_address, "to": keyfile_account_address, @@ -4360,6 +4361,7 @@ def test_eth_get_transaction_receipt_mined( assert isinstance(effective_gas_price, int) assert effective_gas_price > 0 + @flaky_geth_dev_mining def test_eth_get_transaction_receipt_unmined( self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress ) -> None: @@ -4417,6 +4419,7 @@ def test_eth_wait_for_transaction_receipt_mined( assert isinstance(effective_gas_price, int) assert effective_gas_price > 0 + @flaky_geth_dev_mining def test_eth_wait_for_transaction_receipt_unmined( self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress ) -> None: From 899a909b7b9a6b1aeea23c2bbbb795b66e410c37 Mon Sep 17 00:00:00 2001 From: fselmo Date: Wed, 24 Jul 2024 15:34:29 -0600 Subject: [PATCH 2/6] Mark certain very flaky tests as xfail, strict=False --- web3/_utils/module_testing/eth_module.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/web3/_utils/module_testing/eth_module.py b/web3/_utils/module_testing/eth_module.py index b8f87d498a..bb25e192a9 100644 --- a/web3/_utils/module_testing/eth_module.py +++ b/web3/_utils/module_testing/eth_module.py @@ -2358,7 +2358,9 @@ async def test_async_eth_replace_transaction_gas_price_too_low( with pytest.raises(Web3ValueError): await async_w3.eth.replace_transaction(txn_hash, txn_params) - @flaky_geth_dev_mining + @pytest.mark.xfail( + reason="Very flaky on CI runs, hard to reproduce locally", strict=False + ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress @@ -2382,7 +2384,9 @@ async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( gas_price * 1.125 ) # minimum gas price - @flaky_geth_dev_mining + @pytest.mark.xfail( + reason="Very flaky on CI runs, hard to reproduce locally", strict=False + ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_higher( self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress @@ -2411,7 +2415,9 @@ def higher_gas_price_strategy(async_w3: "AsyncWeb3", txn: TxParams) -> Wei: ) # Strategy provides higher gas price async_w3.eth.set_gas_price_strategy(None) # reset strategy - @flaky_geth_dev_mining + @pytest.mark.xfail( + reason="Very flaky on CI runs, hard to reproduce locally", strict=False + ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_lower( self, async_w3: "AsyncWeb3", async_keyfile_account_address: ChecksumAddress From 8965da1a163071cf411d0f4f064e75be5d682e0b Mon Sep 17 00:00:00 2001 From: fselmo Date: Thu, 25 Jul 2024 12:12:55 -0600 Subject: [PATCH 3/6] Explicitly raise ``RequestTimedOut`` on timed out requests: - Introduce ``RequestTimedOut`` and raise this for requests with timeout messaging. - This allows us to be more specific on flaky tests that fail with node timeout errors. Specify ``RequestTimedOut`` as the expected exception in relevant tests marked with ``@pytest.mark.xfail``. --- newsfragments/3440.feature.rst | 1 + newsfragments/3440.internal.rst | 1 + .../core/manager/test_response_formatters.py | 10 ++++++ web3/_utils/module_testing/eth_module.py | 13 +++++-- web3/exceptions.py | 6 ++++ web3/manager.py | 35 +++++++++++++++---- 6 files changed, 57 insertions(+), 9 deletions(-) create mode 100644 newsfragments/3440.feature.rst create mode 100644 newsfragments/3440.internal.rst diff --git a/newsfragments/3440.feature.rst b/newsfragments/3440.feature.rst new file mode 100644 index 0000000000..0693febdcc --- /dev/null +++ b/newsfragments/3440.feature.rst @@ -0,0 +1 @@ +Implement a ``RequestTimedOut`` exception, extending from ``Web3RPCError``, for when requests to the node time out. diff --git a/newsfragments/3440.internal.rst b/newsfragments/3440.internal.rst new file mode 100644 index 0000000000..311ff918a2 --- /dev/null +++ b/newsfragments/3440.internal.rst @@ -0,0 +1 @@ +Mitigate inconsistently failing tests for CI runs with appropriate ``flaky`` or ``pytest.mark.xfail()`` decorators. diff --git a/tests/core/manager/test_response_formatters.py b/tests/core/manager/test_response_formatters.py index e906e5170a..331d25ceca 100644 --- a/tests/core/manager/test_response_formatters.py +++ b/tests/core/manager/test_response_formatters.py @@ -16,6 +16,7 @@ BlockNotFound, ContractLogicError, MethodUnavailable, + RequestTimedOut, TransactionNotFound, Web3RPCError, ) @@ -77,6 +78,10 @@ VALID_ERROR_RESPONSE, {"error": {"code": -32601, "message": (METHOD_UNAVAILABLE_MSG)}}, ) +ERROR_RESPONSE_REQUEST_TIMED_OUT = merge( + VALID_ERROR_RESPONSE, + {"error": {"code": -32002, "message": "Request timed out."}}, +) ERROR_RESPONSE_INVALID_ID = merge(VALID_ERROR_RESPONSE, {"id": b"invalid"}) ERROR_RESPONSE_INVALID_CODE = merge(VALID_ERROR_RESPONSE, {"error": {"code": "-32601"}}) @@ -190,6 +195,11 @@ def test_formatted_response_invalid_response_object(w3, response, error, error_m MethodUnavailable, METHOD_UNAVAILABLE_MSG, ), + ( + ERROR_RESPONSE_REQUEST_TIMED_OUT, + RequestTimedOut, + f'{ERROR_RESPONSE_REQUEST_TIMED_OUT["error"]}', + ), ), ) def test_formatted_response_valid_error_object(response, w3, error, error_message): diff --git a/web3/_utils/module_testing/eth_module.py b/web3/_utils/module_testing/eth_module.py index bb25e192a9..3432c0c3f4 100644 --- a/web3/_utils/module_testing/eth_module.py +++ b/web3/_utils/module_testing/eth_module.py @@ -76,6 +76,7 @@ MultipleFailedRequests, NameNotFound, OffchainLookup, + RequestTimedOut, TimeExhausted, TooManyRequests, TransactionNotFound, @@ -2359,7 +2360,9 @@ async def test_async_eth_replace_transaction_gas_price_too_low( await async_w3.eth.replace_transaction(txn_hash, txn_params) @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", strict=False + reason="Very flaky on CI runs, hard to reproduce locally", + strict=False, + raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( @@ -2385,7 +2388,9 @@ async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( ) # minimum gas price @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", strict=False + reason="Very flaky on CI runs, hard to reproduce locally", + strict=False, + raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_higher( @@ -2416,7 +2421,9 @@ def higher_gas_price_strategy(async_w3: "AsyncWeb3", txn: TxParams) -> Wei: async_w3.eth.set_gas_price_strategy(None) # reset strategy @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", strict=False + reason="Very flaky on CI runs, hard to reproduce locally", + strict=False, + raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_lower( diff --git a/web3/exceptions.py b/web3/exceptions.py index 6b62b7fd2a..2ffe290de4 100644 --- a/web3/exceptions.py +++ b/web3/exceptions.py @@ -364,6 +364,12 @@ class MethodUnavailable(Web3RPCError): """ +class RequestTimedOut(Web3RPCError): + """ + Raised when a request to the node times out. + """ + + class TransactionNotFound(Web3RPCError): """ Raised when a tx hash used to look up a tx in a jsonrpc call cannot be found. diff --git a/web3/manager.py b/web3/manager.py index f85e83adac..0a980ee1d3 100644 --- a/web3/manager.py +++ b/web3/manager.py @@ -37,6 +37,7 @@ BadResponseFormat, MethodUnavailable, ProviderConnectionError, + RequestTimedOut, TaskNotRunning, Web3RPCError, Web3TypeError, @@ -89,6 +90,13 @@ NULL_RESPONSES = [None, HexBytes("0x"), "0x"] +KNOWN_REQUEST_TIMEOUT_MESSAGING = { + # Note: It's important to be very explicit here and not too broad. We don't want + # to accidentally catch a message that is not for a request timeout. In the worst + # case, we raise something more generic like `Web3RPCError`. JSON-RPC unfortunately + # has not standardized error codes for request timeouts. + "request timed out", # go-ethereum +} METHOD_NOT_FOUND = -32601 @@ -185,6 +193,7 @@ def _validate_response( response, 'Response must include either "error" or "result".' ) elif "error" in response: + web3_rpc_error: Optional[Web3RPCError] = None error = response["error"] # raise the error when the value is a string @@ -202,7 +211,7 @@ def _validate_response( response, 'error["code"] is required and must be an integer value.' ) elif code == METHOD_NOT_FOUND: - exception = MethodUnavailable( + web3_rpc_error = MethodUnavailable( repr(error), rpc_response=response, user_message=( @@ -211,9 +220,6 @@ def _validate_response( "currently enabled." ), ) - logger.error(exception.user_message) - logger.debug(f"RPC error response: {response}") - raise exception # errors must include a message error_message = error.get("message") @@ -222,9 +228,26 @@ def _validate_response( response, 'error["message"] is required and must be a string value.' ) - apply_error_formatters(error_formatters, response) + if any( + # parse specific timeout messages + timeout_str in error_message.lower() + for timeout_str in KNOWN_REQUEST_TIMEOUT_MESSAGING + ): + web3_rpc_error = RequestTimedOut( + repr(error), + rpc_response=response, + user_message=( + "The request timed out. Check the connection to your node and " + "try again." + ), + ) + + if web3_rpc_error is None: + # if no condition was met above, raise a more generic `Web3RPCError` + web3_rpc_error = Web3RPCError(repr(error), rpc_response=response) + + response = apply_error_formatters(error_formatters, response) - web3_rpc_error = Web3RPCError(repr(error), rpc_response=response) logger.error(web3_rpc_error.user_message) logger.debug(f"RPC error response: {response}") raise web3_rpc_error From 6d80681d58818222b948b51563ae1d09ea211910 Mon Sep 17 00:00:00 2001 From: fselmo Date: Thu, 25 Jul 2024 16:11:41 -0600 Subject: [PATCH 4/6] Add decorator that xfails on select exception(s), retrying otherwise. --- web3/_utils/module_testing/eth_module.py | 22 +++++----- .../module_testing/module_testing_utils.py | 44 ++++++++++++++++++- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/web3/_utils/module_testing/eth_module.py b/web3/_utils/module_testing/eth_module.py index 3432c0c3f4..904ffa5820 100644 --- a/web3/_utils/module_testing/eth_module.py +++ b/web3/_utils/module_testing/eth_module.py @@ -58,6 +58,7 @@ assert_contains_log, async_mock_offchain_lookup_request_response, flaky_geth_dev_mining, + flaky_with_xfail_on_exception, mock_offchain_lookup_request_response, ) from web3._utils.module_testing.utils import ( @@ -2359,10 +2360,9 @@ async def test_async_eth_replace_transaction_gas_price_too_low( with pytest.raises(Web3ValueError): await async_w3.eth.replace_transaction(txn_hash, txn_params) - @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", - strict=False, - raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), + @flaky_with_xfail_on_exception( + reason="Very flaky on CI runs, hard to reproduce locally.", + exception=RequestTimedOut, ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( @@ -2387,10 +2387,9 @@ async def test_async_eth_replace_transaction_gas_price_defaulting_minimum( gas_price * 1.125 ) # minimum gas price - @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", - strict=False, - raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), + @flaky_with_xfail_on_exception( + reason="Very flaky on CI runs, hard to reproduce locally.", + exception=RequestTimedOut, ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_higher( @@ -2420,10 +2419,9 @@ def higher_gas_price_strategy(async_w3: "AsyncWeb3", txn: TxParams) -> Wei: ) # Strategy provides higher gas price async_w3.eth.set_gas_price_strategy(None) # reset strategy - @pytest.mark.xfail( - reason="Very flaky on CI runs, hard to reproduce locally", - strict=False, - raises=(RequestTimedOut, asyncio.TimeoutError, Web3ValueError), + @flaky_with_xfail_on_exception( + reason="Very flaky on CI runs, hard to reproduce locally.", + exception=RequestTimedOut, ) @pytest.mark.asyncio async def test_async_eth_replace_transaction_gas_price_defaulting_strategy_lower( diff --git a/web3/_utils/module_testing/module_testing_utils.py b/web3/_utils/module_testing/module_testing_utils.py index 0cf4d2d358..35a5e33d11 100644 --- a/web3/_utils/module_testing/module_testing_utils.py +++ b/web3/_utils/module_testing/module_testing_utils.py @@ -1,12 +1,17 @@ import asyncio +import functools +import pytest from typing import ( TYPE_CHECKING, Any, + Callable, Collection, Dict, Generator, Literal, Sequence, + Tuple, + Type, Union, ) @@ -55,7 +60,44 @@ for the duration of the test. This behavior can be flaky due to timing of the test running as a block is mined. """ -flaky_geth_dev_mining = flaky(max_runs=3) +flaky_geth_dev_mining = flaky(max_runs=3, min_passes=1) + + +def flaky_with_xfail_on_exception( + reason: str, + exception: Union[Type[Exception], Tuple[Type[Exception], ...]], + max_runs: int = 3, + min_passes: int = 1, +) -> Callable[[Any], Any]: + """ + Some tests inconsistently fail hard with a particular exception and retrying + these tests often times does not get them "unstuck". If we've exhausted all flaky + retries and this expected exception is raised, `xfail` the test with the given + reason. + """ + runs = max_runs + + def decorator(func: Any) -> Any: + @flaky(max_runs=max_runs, min_passes=min_passes) + @functools.wraps(func) + async def wrapper(self: Any, *args: Any, **kwargs: Any) -> Any: + nonlocal runs + try: + return await func(self, *args, **kwargs) + except exception: + # xfail the test only if the exception is raised and we have exhausted + # all flaky retries + if runs == 1: + pytest.xfail(reason) + runs -= 1 + pytest.fail(f"xfailed but {runs} run(s) remaining with flaky...") + except Exception as e: + # let flaky handle it + raise e + + return wrapper + + return decorator def assert_contains_log( From 5c581e47bc1308ce124856f4998b546bfca20fb9 Mon Sep 17 00:00:00 2001 From: fselmo Date: Fri, 26 Jul 2024 10:50:38 -0600 Subject: [PATCH 5/6] Effective gas price can be lower than the ``maxFeePerGas`` --- web3/_utils/module_testing/eth_module.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/web3/_utils/module_testing/eth_module.py b/web3/_utils/module_testing/eth_module.py index 904ffa5820..87a2afa5ae 100644 --- a/web3/_utils/module_testing/eth_module.py +++ b/web3/_utils/module_testing/eth_module.py @@ -500,6 +500,7 @@ async def test_eth_send_transaction( "maxFeePerGas": async_w3.to_wei(3, "gwei"), "maxPriorityFeePerGas": async_w3.to_wei(1, "gwei"), } + txn_hash = await async_w3.eth.send_transaction(txn_params) txn = await async_w3.eth.get_transaction(txn_hash) @@ -509,7 +510,7 @@ async def test_eth_send_transaction( assert txn["gas"] == 21000 assert txn["maxFeePerGas"] == txn_params["maxFeePerGas"] assert txn["maxPriorityFeePerGas"] == txn_params["maxPriorityFeePerGas"] - assert txn["gasPrice"] == txn_params["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price @pytest.mark.asyncio async def test_eth_send_transaction_default_fees( @@ -523,6 +524,7 @@ async def test_eth_send_transaction_default_fees( "value": Wei(1), "gas": 21000, } + txn_hash = await async_w3.eth.send_transaction(txn_params) txn = await async_w3.eth.get_transaction(txn_hash) @@ -532,7 +534,7 @@ async def test_eth_send_transaction_default_fees( assert txn["gas"] == 21000 assert is_integer(txn["maxPriorityFeePerGas"]) assert is_integer(txn["maxFeePerGas"]) - assert txn["gasPrice"] == txn["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price @pytest.mark.asyncio async def test_eth_send_transaction_hex_fees( @@ -809,7 +811,7 @@ def gas_price_strategy(w3: "Web3", txn: TxParams) -> Wei: else 2 * latest_block["baseFeePerGas"] + max_priority_fee ) assert txn["maxPriorityFeePerGas"] == max_priority_fee - assert txn["gasPrice"] == txn["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price async_w3.eth.set_gas_price_strategy(None) # reset strategy @@ -3114,6 +3116,7 @@ def test_eth_send_transaction( "maxFeePerGas": w3.to_wei(3, "gwei"), "maxPriorityFeePerGas": w3.to_wei(1, "gwei"), } + txn_hash = w3.eth.send_transaction(txn_params) txn = w3.eth.get_transaction(txn_hash) @@ -3123,7 +3126,7 @@ def test_eth_send_transaction( assert txn["gas"] == 21000 assert txn["maxFeePerGas"] == txn_params["maxFeePerGas"] assert txn["maxPriorityFeePerGas"] == txn_params["maxPriorityFeePerGas"] - assert txn["gasPrice"] == txn_params["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price def test_eth_send_transaction_with_nonce( self, w3: "Web3", keyfile_account_address: ChecksumAddress @@ -3174,7 +3177,7 @@ def test_eth_send_transaction_default_fees( assert txn["gas"] == 21000 assert is_integer(txn["maxPriorityFeePerGas"]) assert is_integer(txn["maxFeePerGas"]) - assert txn["gasPrice"] == txn["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price def test_eth_send_transaction_hex_fees( self, w3: "Web3", keyfile_account_address_dual_type: ChecksumAddress @@ -3340,7 +3343,7 @@ def gas_price_strategy(_w3: "Web3", _txn: TxParams) -> Wei: else 2 * latest_block["baseFeePerGas"] + max_priority_fee ) assert txn["maxPriorityFeePerGas"] == max_priority_fee - assert txn["gasPrice"] == txn["maxFeePerGas"] + assert txn["gasPrice"] <= txn["maxFeePerGas"] # effective gas price w3.eth.set_gas_price_strategy(None) # reset strategy From 4694dd6cc8c519607b34dd1f819580731b73dce6 Mon Sep 17 00:00:00 2001 From: fselmo Date: Fri, 26 Jul 2024 11:21:50 -0600 Subject: [PATCH 6/6] Small refactor for readability; fix test case --- tests/core/manager/test_response_formatters.py | 4 +++- web3/manager.py | 17 ++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/core/manager/test_response_formatters.py b/tests/core/manager/test_response_formatters.py index 331d25ceca..73bd1982d1 100644 --- a/tests/core/manager/test_response_formatters.py +++ b/tests/core/manager/test_response_formatters.py @@ -84,7 +84,9 @@ ) ERROR_RESPONSE_INVALID_ID = merge(VALID_ERROR_RESPONSE, {"id": b"invalid"}) -ERROR_RESPONSE_INVALID_CODE = merge(VALID_ERROR_RESPONSE, {"error": {"code": "-32601"}}) +ERROR_RESPONSE_INVALID_CODE = merge( + VALID_ERROR_RESPONSE, {"error": {"code": "-32601", "message": ""}} +) ERROR_RESPONSE_INVALID_MISSING_CODE = merge( VALID_ERROR_RESPONSE, {"error": {"message": "msg"}} ) diff --git a/web3/manager.py b/web3/manager.py index 0a980ee1d3..888855e0b1 100644 --- a/web3/manager.py +++ b/web3/manager.py @@ -204,6 +204,13 @@ def _validate_response( "JSON-RPC 2.0 specification.", ) + # errors must include a message + error_message = error.get("message") + if not isinstance(error_message, str): + _raise_bad_response_format( + response, 'error["message"] is required and must be a string value.' + ) + # errors must include an integer code code = error.get("code") if not isinstance(code, int): @@ -220,15 +227,7 @@ def _validate_response( "currently enabled." ), ) - - # errors must include a message - error_message = error.get("message") - if not isinstance(error_message, str): - _raise_bad_response_format( - response, 'error["message"] is required and must be a string value.' - ) - - if any( + elif any( # parse specific timeout messages timeout_str in error_message.lower() for timeout_str in KNOWN_REQUEST_TIMEOUT_MESSAGING