-
Notifications
You must be signed in to change notification settings - Fork 12
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
Conversation
This dependancy has not been mantained for over a year and is only used to calculate a mean
panic!("Transaction {transaction:#064x} has been rejected/reverted: {reason}"); | ||
} | ||
}, | ||
MaybePendingTransactionReceipt::PendingReceipt(_) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 }) |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 :(
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 |
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:
Closes #27 and closes #17