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

Derive Clone for App #55

Merged
merged 1 commit into from
Oct 2, 2023
Merged

Derive Clone for App #55

merged 1 commit into from
Oct 2, 2023

Conversation

0xForerunner
Copy link
Contributor

This is useful in testing. It's common to run a complicated setup procedure to build an app state, then to run different tests on that app. Rather that constantly rebuilding the state this allows you to clone it around.

It seems that perhaps other useful derives are missing as well (Debug being an obvious one). Though I don't immediately have a use case for that.

@DariuszDepta
Copy link
Member

DariuszDepta commented Sep 7, 2023

Hi @ewoolsey!, just to make things clearer, could you provide an example of such test suite where the app is cloned and then used in multiple tests?

@DariuszDepta DariuszDepta added the question Further information is requested label Sep 14, 2023
@DariuszDepta DariuszDepta removed the question Further information is requested label Sep 27, 2023
@DariuszDepta
Copy link
Member

@ewoolsey I am closing this PR as no answer received. This PR can be reopened in the future if there will be a reason to add requested derives to App.

Remark/workaround:
In tests we do, when there is a need to do complicated App setup to be used in multiple tests, we implement utility function that does this setup and returns the App. This function is called in each test, so each test has fresh copy of the App. This is an equivalent of cloning the App without a risk of interference between tests, like setup-ing App, running one test that possibly changes the App state, then cloning the App and using it in another test with the state that is not initial anymore.

I hope this helps.

@DariuszDepta DariuszDepta self-assigned this Sep 29, 2023
@DariuszDepta DariuszDepta added wontfix This will not be worked on and removed wontfix This will not be worked on labels Sep 29, 2023
@0xForerunner
Copy link
Contributor Author

@ewoolsey I am closing this PR as no answer received. This PR can be reopened in the future if there will be a reason to add requested derives to App.

Remark/workaround: In tests we do, when there is a need to do complicated App setup to be used in multiple tests, we implement utility function that does this setup and returns the App. This function is called in each test, so each test has fresh copy of the App. This is an equivalent of cloning the App without a risk of interference between tests, like setup-ing App, running one test that possibly changes the App state, then cloning the App and using it in another test with the state that is not initial anymore.

I hope this helps.

Hey sorry for missing the reply my apologies. Yes your workaround is what I'm currently doing. This is very computationally expensive when performing 10 thousand randomized tests. Being able to clone this structure would be ideal. As far as I'm aware there are exactly no down sides to deriving Clone so my question to you would be, why not?

@0xForerunner
Copy link
Contributor Author

For more context, I have very complicated setup procedure for my app, around 500 lines or so. This base state is then modified using an rng to test edge cases. Running my 500 line setup procedure 10000 times in incredible slow.

@DariuszDepta DariuszDepta reopened this Oct 2, 2023
@DariuszDepta DariuszDepta merged commit 7a427b6 into CosmWasm:main Oct 2, 2023
2 checks passed
@DariuszDepta
Copy link
Member

@ewoolsey Just let me know if using Clone is the performance better in your case. Thanks.

@DariuszDepta DariuszDepta added this to the 0.18.0 milestone Oct 2, 2023
@0xForerunner
Copy link
Contributor Author

@ewoolsey Just let me know if using Clone is the performance better in your case. Thanks.

Just did a quick benchmark and one of my fuzz tests had a 5x perf improvement :) Thanks for approving the PR. Really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants