[LoadTest] Revamp load test suite #1002
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@red-0ne Lmk if you need me to take a look at this as well. |
Olshansk
requested changes
Dec 20, 2024
There was a problem hiding this comment.
@red-0ne Partial review.
This is great work but load-testing/tests/relays_stress_helpers_test.go is really hard to review right now with lots of big inline functions.
Do you mind doing a self passthrough and finding ways to make it a bit more readable & maintainable for others?
| @@ -2,16 +2,16 @@ | |||
| # It is intended to target a remote environment, such as a devnet or testnet. | |||
| is_ephemeral_chain: false | |||
|
|
|||
| # testnet_node is the URL of the node that the load test will use to query the | |||
| # pocket_node is the URL of the node that the load test will use to query the | |||
There was a problem hiding this comment.
Can we rename this to full_node or rpc_node?
|
|
||
| # The service ID to request relays from. | ||
| service_id: "anvil" | ||
|
|
||
| # The address of the account that will be used to fund the application accounts | ||
| # so that they can stake on the network. | ||
| funding_account_address: pokt1awtlw5sjmw2f5lgj8ekdkaqezphgz88rdk93sk # address for faucet account | ||
| funding_account_address: pokt1eeeksh2tvkh7wzmfrljnhw4wrhs55lcuvmekkw # address for faucet account |
There was a problem hiding this comment.
#PUC which network this is the faucet on. Alpha? Beta? Local?
| @@ -3,12 +3,16 @@ | |||
|
|
|||
| is_ephemeral_chain: true # This should be `true` for LocalNet as it is an ephemeral network | |||
|
|
|||
| # pocket_node is the URL of the node that the load test will use to query the | |||
| # chain and submit transactions. | |||
| pocket_node: http://localhost:26657 | |||
| # The service ID to use for the load test. | ||
| service_id: anvil | ||
|
|
||
| # The address of the account that will be used to fund the application, | ||
| # gateway and supplier accounts so that they can stake on the network. | ||
| funding_account_address: pokt1awtlw5sjmw2f5lgj8ekdkaqezphgz88rdk93sk # address for faucet account | ||
| funding_account_address: pokt1eeeksh2tvkh7wzmfrljnhw4wrhs55lcuvmekkw # address for faucet account |
| # TODO_FOLLOWUP(@red-0ne): Implement the following steps | ||
| # Then "0" over servicing events are observed | ||
| # And "0" slashing events are observed |
There was a problem hiding this comment.
Can you describe (for future implementation potential by an AI agent):
- A test w/ over servicing?
- A test w/ slashing?
| @@ -631,7 +651,8 @@ func (s *relaysSuite) getAppFundingAmount(currentBlockHeight int64) sdk.Coin { | |||
| // based on the number of relays it needs to send. Theoretically, `+1` should | |||
| // be enough, but probabilistic and time based mechanisms make it hard | |||
| // to predict exactly. | |||
| appFundingAmount := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration * 2 | |||
| amountToSpend := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration | |||
There was a problem hiding this comment.
Suggested change
| amountToSpend := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration | |
| relayCostDuringTestPOKT := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration |
| @@ -631,7 +651,8 @@ func (s *relaysSuite) getAppFundingAmount(currentBlockHeight int64) sdk.Coin { | |||
| // based on the number of relays it needs to send. Theoretically, `+1` should | |||
| // be enough, but probabilistic and time based mechanisms make it hard | |||
| // to predict exactly. | |||
| appFundingAmount := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration * 2 | |||
| amountToSpend := s.relayRatePerApp * s.relayCoinAmountCost * currentTestDuration * blockDuration | |||
| appFundingAmount := math.Max(amountToSpend, s.appParams.MinStake.Amount.Int64()*2) | |||
There was a problem hiding this comment.
Move the comment starting with Multiply by 2 above so it's above this line instead.
| strings.NewReader(payload), | ||
| ) | ||
| go func(gwURL, appAddr, payload string) { | ||
| gwURL = gwURL + "/" |
There was a problem hiding this comment.
Can we just call it gatewayURL without the shortening?
| gwURL = gwURL + "/" | ||
| req, err := http.NewRequest("POST", gwURL, strings.NewReader(payload)) | ||
|
|
||
| req.Header.Add("X-App-Address", appAddr) |
There was a problem hiding this comment.
- TODO_TECHDEBT to remove
x- - #PUC why we need this
| if res.StatusCode != http.StatusOK { | ||
| s.failedRelays.Add(1) | ||
| } | ||
| }(s.gatewayUrls[gateway.address], application.address, relayPayload) |
There was a problem hiding this comment.
What if you do something like:
inlineFunc := func(...) {..}
gatewayURL := s.gatewayUrls[...]
go inlineFunc(gatewayURL, ...)You'll need better names, but it'll be easier to maintain.,
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This pull request includes several updates to the load-testing configuration and test files, as well as improvements to the relay stress test suite. The most important changes include updating funding account addresses, modifying gateway configurations, and refactoring various test functions for better accuracy.
Type of change
Select one or more from the following:
consensus-breakinglabel if so. See [Infra] Automatically add theconsensus-breakinglabel #791 for detailsTesting
make go_develop_and_testmake test_e2edevnet-test-e2elabel to the PR.Sanity Checklist