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

Rewrite of shoot to use the Goose load testing framework #40

Merged
merged 13 commits into from
Feb 12, 2024

Conversation

Angel-Petrov
Copy link
Contributor

@Angel-Petrov Angel-Petrov commented Feb 2, 2024

This is a major rewrite of the gatling shooter use goose for load testing instead of the previous single threaded implementation.

The main benefits of this are that the tests are now multithreaded, allowing for the load on the server to be higher. The code also doesn't need to measure metrics manually as Goose will do it for us.

This attempts to preserve the previous behaviour of shooter, apart from:

  • Verifying/checking transactions is now done seperately at the end of erc20/erc721 tests and not at the end of all tests
  • erc20 transaction senders will not be randomized but instead each thread will be assigned a sender
  • erc721 transaction recipients will not be randomized but instead each thread will be assigned a recipient

Closes #27 and closes #17

src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
panic!("Transaction {transaction:#064x} has been rejected/reverted: {reason}");
}
},
MaybePendingTransactionReceipt::PendingReceipt(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can this condition could be true later on. Like you've multiple accounts and we confirmed the last txn on an account has been accepted but some other accounts last txn might not have been yet (which is possible because we don't know how the sequencer is ordering txs). So instead of panicing, we should poll and have a timeout. Not necessary for now, you can create a TODO and create an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think for this we can make it so the waiting for transactions is a part of the previous GooseAttack as well as having this be a part of it as well, that way we can use a Vec to store all the transactions to the state of the goose user and not combine them all into one ArrayQueue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still a issue in the current code? It should handle the multiple accounts properly now.

src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/mod.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/shoot.rs Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
let transfer: TransactionFunction = Arc::new(move |user| {
let queue = queue_trans.clone();
let last_mint = last_transaction_clone.clone();
Box::pin(async move { transfer(user, &queue, _erc20_address, &last_mint).await })
Copy link

Choose a reason for hiding this comment

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

https://docs.rs/goose/latest/src/goose/goose.rs.html#317-323

That is what the transaction! macro does. You don't have to do it manually every time.

src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Show resolved Hide resolved
src/actions/goose.rs Show resolved Hide resolved
src/actions/goose.rs Outdated Show resolved Hide resolved
src/actions/goose.rs Show resolved Hide resolved
Copy link

@tdelabro tdelabro left a comment

Choose a reason for hiding this comment

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

@Angel-Petrov I find all this arc box pin boilerplate rather repelling but I guess it is a limitation of the framework. As is the absence of error handling 🥲
I will stop bothering you with those :(

@Angel-Petrov
Copy link
Contributor Author

The error handling also irks me quite a lot as it would be possible to add as a custom error variant to the goose error :/

It also seems like others are experiencing the same issues, so hopefully it gets addressed sooner or later

@tdelabro tdelabro merged commit 93cb64e into keep-starknet-strange:main Feb 12, 2024
3 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dev: add multi-threading to send txs RUSTSEC-2021-0141: dotenv is Unmaintained
3 participants