Problem: update max-tx-gas-wanted is redundant in subscribe test #1238
Conversation
default max-tx-gas-wanted is 0
mmsqe
requested review from
yihuang and
leejw51crypto
and removed request for
a team
WalkthroughThe changes across the Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_subscribe.py (3 hunks)
Additional comments: 3
integration_tests/test_subscribe.py (3)
- 16-21: ```
The removal of themodify_command_in_supervisor_configfunction is not shown in this hunk, but it is mentioned in the summary. Ensure that its removal does not affect other tests or parts of the code that might be relying on it. If this function was used elsewhere, those usages need to be refactored or removed as well.---end hunk 0---
---start hunk 1---
* 86-91: ``` The code here appears to be unchanged and is likely provided for context. Since there are no modifications in this hunk, there's nothing to review. ---end hunk 1--- ---start hunk 2---
- 155-165: ```
The reordering of function calls within theasync_testfunction seems to be done to ensure that the `transfer_test` and `logs_test` are called after the `subscriber_test`. This change could be for clarity or optimization. However, without additional context on the reason for the reordering, it's difficult to assess the impact. Ensure that the reordering does not affect the test outcomes or introduce race conditions.---end hunk 2---
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_subscribe.py (3 hunks)
Additional comments: 3
integration_tests/test_subscribe.py (3)
15-20: The removal of
modify_command_in_supervisor_configfunction usage is noted. Ensure that this function is not used elsewhere in the codebase or by other tests that may rely on its functionality. If it is still used elsewhere, consider whether it should be deprecated entirely or if its removal here is a special case.84-89: The documentation for the
test_subscribe_basicfunction is clear and concise. The setup for the test environment, including waiting for the port to be ready, is appropriate.153-163: The change from
asyncio.gatherto individualawaitcalls fortransfer_testandlogs_testfunctions is a significant alteration in how these asynchronous tasks are being executed. This change could potentially alter the timing and order of operations, which may affect the test outcomes. Verify that the tests still pass and that the change in concurrency does not introduce any new issues or race conditions.Additionally, the cancellation of the
receive_looptask and the handling ofCancelledErroris a good practice to ensure that resources are cleaned up properly when the test is finished or if it times out.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- integration_tests/test_basic.py (3 hunks)
Additional comments: 5
integration_tests/test_basic.py (5)
785-791: The introduction of the
max_gas_wantedparameter and the early return when it isNoneis a good approach to handle default behavior. However, ensure that the default behavior (whenmax_gas_wantedisNone) is indeed to perform no action, as this changes the previous behavior of the function.805-805: The
modify_command_in_supervisor_configfunction is being removed as per the PR summary. Ensure that all references to this function are removed and that the removal does not affect other parts of the test suite that might rely on the modified command behavior.809-810: The call to
cronos.supervisorctl("update")andwait_for_portseems to be related to the configuration change made by the now-removedmodify_command_in_supervisor_config. If this function is removed, verify that these calls are still necessary and that they function correctly without the configuration modification.813-814: The early return when
max_gas_wantedisNoneis clear and concise. This is a good use of a guard clause to simplify the flow of the function.816-818: The
block_gas_limitis hardcoded here. Ensure that this value is consistent with the actual block gas limit configured in the blockchain node. If the block gas limit is subject to change, consider fetching it dynamically from the node's configuration or state.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
===========================================
- Coverage 36.89% 16.67% -20.23%
===========================================
Files 115 79 -36
Lines 10255 5786 -4469
===========================================
- Hits 3784 965 -2819
+ Misses 6097 4743 -1354
+ Partials 374 78 -296 |
607fa83
default max-tx-gas-wanted is 0
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
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
Refactor
modify_command_in_supervisor_configfunction and its usage.asyncio.gatherwith individualawaitcalls.Tests
modify_command_in_supervisor_configfunction.test_tx_inclusionto handle a newmax_gas_wantedparameter, enhancing test flexibility.