Problem: test_tx_inclusion is flaky when nodes halt due to different gas_wanted #1657
Conversation
…gas_wanted wait longer to avoid nodes start with different max_gas_wanted
mmsqe
requested review from
JayT106 and
leejw51crypto
and removed request for
a team
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
integration_tests/utils.py (1)
125-132: LGTM! Consider enhancing the timeout error message.The timeout implementation is well-structured and consistent with other timeout handling in the codebase. The default timeout value of 240 seconds aligns with other similar functions.
Consider making the timeout error message more descriptive by including the current height:
- raise TimeoutError(f"wait for block {begin_height + n} timeout") + raise TimeoutError(f"wait for block {begin_height + n} timeout (current height: {cur_height})")integration_tests/test_basic.py (4)
Line range hint
875-876: Avoid using hard-coded sleep intervals; use explicit synchronization insteadUsing
time.sleep(2)can lead to flaky tests due to variable execution times. Consider waiting for the service to become available by polling or using explicit synchronization methods to ensure reliability.
Line range hint
883-883: Rename variablesended_hash_settosent_hash_setThe variable name
sended_hash_setis grammatically incorrect. The correct past tense of "send" is "sent".Apply this diff to correct the variable name:
- _, sended_hash_set = send_txs(w3, cli, to, list(KEYS.values())[0:4], params) + _, sent_hash_set = send_txs(w3, cli, to, list(KEYS.values())[0:4], params)
Line range hint
872-876: Avoid using global variables; encapsulateorigin_cmdwithin the test functionUsing global variables like
origin_cmdcan lead to unintended side effects, especially when tests are run concurrently. Consider refactoring to manageorigin_cmdwithin the function scope or by using a fixture for better test isolation.
Line range hint
870-906: Ensure division by zero is avoided inmax_tx_in_blockcalculationWhen calculating
max_tx_in_block, there is a potential for division by zero ifmax_gas_wantedis zero. Although current parameters avoid zero, adding a check can prevent future errors.Apply this diff to add a safeguard:
+ if max_gas_wanted == 0: + raise ValueError("max_gas_wanted must be greater than zero") max_tx_in_block = block_gas_limit // min(max_gas_wanted, tx_gas_limit)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- integration_tests/test_basic.py (1 hunks)
- integration_tests/utils.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
integration_tests/test_basic.py (1)
Line range hint
782-817: Testtest_failed_transfer_txcorrectly verifies inclusion of failed transactionsThe test effectively demonstrates that a failed transfer transaction within a batch is still included in the block. Assertions and checks are properly implemented to validate the behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1657 +/- ##
===========================================
- Coverage 34.94% 16.64% -18.30%
===========================================
Files 123 72 -51
Lines 11803 6212 -5591
===========================================
- Hits 4124 1034 -3090
+ Misses 7265 5055 -2210
+ Partials 414 123 -291 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
integration_tests/test_basic.py (2)
869-874: Synchronization improvements look good, but consider adding error handling.The added synchronization logic helps address the flakiness issue by ensuring proper timing between operations. However, consider adding error handling for the port wait operation.
Consider adding a timeout and error handling:
- wait_for_port(ports.evmrpc_port(cronos.base_port(0))) + try: + wait_for_port(ports.evmrpc_port(cronos.base_port(0)), timeout=30) + except TimeoutError: + raise TimeoutError("EVM RPC port not available after supervisor update")
Line range hint
832-838: Consider enhancing test documentation.While the test implementation is solid, the docstring could be more descriptive about the test's purpose and the significance of the
max_gas_wantedparameter.Consider expanding the docstring:
def test_tx_inclusion(cronos, max_gas_wanted): """ - - send multiple heavy transactions at the same time. - - check they are included in consecutively blocks without failure. - - test against different max-gas-wanted configuration. + Test transaction inclusion behavior with different max-gas-wanted configurations. + + Args: + cronos: The Cronos test fixture + max_gas_wanted: Maximum gas wanted per transaction. This parameter affects + how many transactions can be included in a single block. + + The test: + 1. Sends multiple heavy transactions concurrently + 2. Verifies they are included in consecutive blocks without failure + 3. Validates block inclusion patterns based on max-gas-wanted limits """
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
572aae7
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
Closes: #1656
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit