Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stabilized HBAR Rate Limit acceptance tests #3189

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

quiet-node
Copy link
Member

@quiet-node quiet-node commented Oct 31, 2024

Description:
With the new implementation of HbarLimitService enabled, the HBAR Limiter test has become highly flaky. Since adding expenses and updating spending plans are executed asynchronously, and the suite includes exhaustion tests with numerous consecutive calls to the network, the test client sometimes fails to retrieve the correct amounts.

The tests are flaky to the extent that they either fail or only pass after multiple retries. To address this, this PR includes measures to stabilize the tests by:
- Adding polling methods to verify the remaining HBARs and amounts spent by hbarSpendingPlans.
- Increasing tolerance in verifyRemainingLimit().
- Adding a 1500ms wait time after each test case to allow asynchronous expense updates to complete in the background.
- Introducing a TEST_TRANSACTION_RECORD_COST_TOLERANCE configuration variable for tolerance, allowing for reuse throughout the test and facilitating easy adjustments without requiring code changes.

Related issue(s):

Fixes #3188

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@quiet-node quiet-node added the Internal For changes that affect the project's internal workings but not its outward-facing functionality. label Oct 31, 2024
@quiet-node quiet-node added this to the 0.60.0 milestone Oct 31, 2024
@quiet-node quiet-node self-assigned this Oct 31, 2024
@quiet-node quiet-node linked an issue Oct 31, 2024 that may be closed by this pull request
@quiet-node quiet-node force-pushed the 3188-hbarlimiter-tests-are-very-flaky branch from 3ce1052 to be75fdd Compare October 31, 2024 00:11
Copy link

github-actions bot commented Oct 31, 2024

Test Results

    4 files   -  18    419 suites  +151   19s ⏱️ - 35m 30s
1 474 tests +896  1 473 ✅ +910  1 💤  - 3  0 ❌  - 11 
1 483 runs  +789  1 482 ✅ +808  1 💤  - 3  0 ❌  - 16 

Results for commit c8076f8. ± Comparison against base commit f189801.

This pull request removes 578 and adds 1474 tests. Note that renamed tests count towards both.
"before all" hook in "@api-batch-2 RPC Server Acceptance Tests" ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests "before all" hook in "@api-batch-2 RPC Server Acceptance Tests"
"eth_call" for non-existing contract address returns 0x ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call "eth_call" for non-existing contract address returns 0x
001 Should call pureMultiply ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With evm address 001 Should call pureMultiply
001 Should call pureMultiply ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With long-zero address 001 Should call pureMultiply
002 Should call msgSender ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With evm address 002 Should call msgSender
002 Should call msgSender ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With long-zero address 002 Should call msgSender
003 Should call txOrigin ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With evm address 003 Should call txOrigin
003 Should call txOrigin ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With long-zero address 003 Should call txOrigin
004 Should call msgSig ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With evm address 004 Should call msgSig
004 Should call msgSig ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With long-zero address 004 Should call msgSig
…
"eth_blockNumber" return the latest block number on second try ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" return the latest block number on second try
"eth_blockNumber" should return the latest block number using cache ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should return the latest block number using cache
"eth_blockNumber" should return the latest block number ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should return the latest block number
"eth_blockNumber" should throw an error if no blocks are found after third try ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should throw an error if no blocks are found after third try
"eth_blockNumber" should throw an error if no blocks are found ‑ @ethGetBlockByNumber using MirrorNode "eth_blockNumber" should throw an error if no blocks are found
Adds a revertReason field for receipts with errorMessage ‑ @ethGetTransactionReceipt eth_getTransactionReceipt tests Adds a revertReason field for receipts with errorMessage
BLOCK_HASH filter timeouts and throws the expected error ‑ @ethGetLogs using MirrorNode timeout BLOCK_HASH filter timeouts and throws the expected error
BLOCK_HASH filter ‑ @ethGetLogs using MirrorNode BLOCK_HASH filter
Can extract the account number out of an account pagination next link url ‑ MirrorNodeClient Can extract the account number out of an account pagination next link url
Can extract the evm address out of an account pagination next link url ‑ MirrorNodeClient Can extract the evm address out of an account pagination next link url
…
This pull request removes 4 skipped tests and adds 1 skipped test. Note that renamed tests count towards both.
011 Should fail when calling msgValue with more value than available balance ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With evm address 011 Should fail when calling msgValue with more value than available balance
011 Should fail when calling msgValue with more value than available balance ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-3 RPC Server Acceptance Tests eth_call Caller contract With long-zero address 011 Should fail when calling msgValue with more value than available balance
from/to Addresses in transaction to a contract (deployed through HAPI tx) are in evm and long-zero format ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests Formats of addresses in Transaction and Receipt results from/to Addresses in transaction to a contract (deployed through HAPI tx) are in evm and long-zero format
should not return contract bytecode after sefldestruct ‑ RPC Server Acceptance Tests Acceptance tests @api-batch-2 RPC Server Acceptance Tests eth_getCode should not return contract bytecode after sefldestruct
supports optionality of request id when configured ‑ RPC Server given REQUEST_ID_IS_OPTIONAL=true are set supports optionality of request id when configured

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments.
I understand the unblocking driver but let's capture all these needed waits and come back in the future to remove them as this likely just pushes the issue down the road

@quiet-node quiet-node force-pushed the 3188-hbarlimiter-tests-are-very-flaky branch from 1269b74 to 2bf2bf2 Compare October 31, 2024 20:47
Nana-EC
Nana-EC previously approved these changes Oct 31, 2024
docs/configuration.md Show resolved Hide resolved
Copy link
Contributor

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question.

Signed-off-by: Logan Nguyen <[email protected]>
Copy link

sonarcloud bot commented Oct 31, 2024

@quiet-node quiet-node merged commit 7fea98c into main Nov 1, 2024
45 checks passed
@quiet-node quiet-node deleted the 3188-hbarlimiter-tests-are-very-flaky branch November 1, 2024 15:24
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.35%. Comparing base (f189801) to head (c8076f8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3189   +/-   ##
=======================================
  Coverage   83.35%   83.35%           
=======================================
  Files          66       66           
  Lines        4290     4290           
  Branches      837      837           
=======================================
  Hits         3576     3576           
  Misses        473      473           
  Partials      241      241           
Flag Coverage Δ
config-service 98.14% <ø> (ø)
relay 85.54% <ø> (ø)
server 83.52% <ø> (ø)
ws-server 36.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ckages/config-service/src/services/globalConfig.ts 100.00% <ø> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal For changes that affect the project's internal workings but not its outward-facing functionality. P1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HbarLimiter tests are very flaky
4 participants