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

feature: test for state version in integration test #82

Closed

Conversation

Tguntenaar
Copy link
Collaborator

@Tguntenaar Tguntenaar commented Dec 19, 2023

Resolves 71

I wasn't sure how to test the correct version using rust, so I suggest this approach. However, if there's a more effective way to test it, please let me know.

Also this is not perfect since it has the flaw that it depends the migrations.rs to be chronically correct.

@petersalomonsen
Copy link
Collaborator

Wouldn't it be better to solve this with a test in Rust, that first calls new and then runs migration?

If the developer forgets to bump the version number in new, then data will be written in the new format , but a following upgrade will fail, because it expects to upgrade from an old format. Wouldn't that work?

@Tguntenaar
Copy link
Collaborator Author

Tguntenaar commented Dec 19, 2023

@petersalomonsen Thanks for the feedback, I appreciate it. Indeed. However, if new initiates the contract in version 7 than upgrading to v8 is still possible. The unsafe migrate function will match the contract state version on v7 and upgrade it to v8.

The problem is how do we test what the latest state version is. I don't think it is possible to get the last or highest value from an enum in Rust. My guess is we can read the state version and call new, than check if it is upgradable.

If it is it means new didn't initiate in the latest version. However this approach would mean we have to upgrade the test on every migration as well which doesn't solve any problem.

I will share the code now

@Tguntenaar Tguntenaar changed the title feature: test for state version in fn new using awk feature: test for state version in integration test Dec 20, 2023
@petersalomonsen
Copy link
Collaborator

@petersalomonsen Thanks for the feedback, I appreciate it. Indeed. However, if new initiates the contract in version 7 than upgrading to v8 is still possible. The unsafe migrate function will match the contract state version on v7 and upgrade it to v8.

The problem is how do we test what the latest state version is. I don't think it is possible to get the last or highest value from an enum in Rust. My guess is we can read the state version and call new, than check if it is upgradable.

If it is it means new didn't initiate in the latest version. However this approach would mean we have to upgrade the test on every migration as well which doesn't solve any problem.

I will share the code now

I see that you have added a unit test, but I believe we should rather have an integration test in tests/migration.rs.

Here we have the possibility to fetch the current contract deployed to mainnet, for running in the near-workspaces environment.

 let contract = worker
        .import_contract(&contract_id, &mainnet)
        .initial_balance(NearToken::from_near(1000))
        .transact()
        .await?;

and try upgrading it to the one we have in the PR.

let wasm = near_workspaces::compile_project("./").await?;

let contract_deploy_result = contract.as_account().deploy(wasm.as_slice()).await?;
assert!(contract_deploy_result.is_success());

And run the migration and then in our integration test, we should run some transaction that writes some data.

To reproduce the scenario in the issue, we can upgrade it again, and if the developer has forgot to bump the version in new then the migration function will try upgrading on a state that is already the newest version, and the test will fail.

Does that make sense? So in short, create an integration test for this, to simulate exactly what happened when we experienced the issue, rather than a unit test.

@Tguntenaar Tguntenaar closed this Jul 19, 2024
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