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.

Sign up for GitHub

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

Jump to bottom

voting-contract: tests #8

Merged
merged 30 commits into from
Jan 13, 2022
Merged

voting-contract: tests #8

merged 30 commits into from
Jan 13, 2022

Conversation

uint
Copy link
Contributor

@uint uint commented Jan 5, 2022 •
edited
Loading

Closes #6

@uint uint force-pushed the 6-voting-contract-tests branch 2 times, most recently from 78d7bf3 to b2408ae Compare January 6, 2022 23:26
@uint uint changed the base branch from main to move-rules-builder January 7, 2022 09:28
@uint
Copy link
Contributor Author

uint commented Jan 7, 2022

Open question: if some of these tests are copied from oc-proposals and similar, should we remove them from oc-proposals once this is merged? No reason to test the same logic twice and maintain those tests there.

hashedone
hashedone reviewed Jan 7, 2022
View reviewed changes
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Just commenting in-between to not have too much at the end. Some comments, but not strongly relevant, just my recent thoughts.

Also tests for voting and executing are missing and they are very important, but I assume it is a reason why it is still in draft.

packages/voting-contract/src/multitest/suite.rs Outdated Show resolved Hide resolved
packages/voting-contract/src/multitest/contracts.rs Outdated Show resolved Hide resolved
Base automatically changed from move-rules-builder to main January 10, 2022 09:13
@uint
Copy link
Contributor Author

uint commented Jan 10, 2022

Rebased.

@uint
Copy link
Contributor Author

uint commented Jan 11, 2022

Only missing tests for the various queries now.

@uint uint marked this pull request as ready for review January 12, 2022 10:16
@uint
Copy link
Contributor Author

uint commented Jan 12, 2022

Also tests for voting and executing are missing and they are very important, but I assume it is a reason why it is still in draft.

No execution tests because there is no execution logic to test in voting-contract. I think we should move some of it to voting-contract and then test it here, but that's definitely out of scope for this PR. See discussion in #14.

@maurolacy maurolacy mentioned this pull request Jan 12, 2022
Merged
@maurolacy
Copy link
Contributor

maurolacy commented Jan 12, 2022 •
edited
Loading

No execution tests because there is no execution logic to test in voting-contract. I think we should move some of it to voting-contract and then test it here, but that's definitely out of scope for this PR. See discussion in #14.

Done in #18.

@uint
Copy link
Contributor Author

uint commented Jan 12, 2022

Done in #18.

Did you intend to force-push to this branch?

@uint
Copy link
Contributor Author

uint commented Jan 12, 2022

Did you intend to force-push to this branch?

Apparently this was just a rebase. Review away!

@maurolacy
Copy link
Contributor

maurolacy commented Jan 12, 2022 •
edited
Loading

Ups, sorry about that. I'll review this tomorrow, first thing in the morning. I was taking a look while developing my changes, and this looks pretty good.

@maurolacy
Copy link
Contributor

maurolacy commented Jan 12, 2022 •
edited
Loading

Just merged #18 in here. If you wanted to add a couple more execute tests, now is when.

maurolacy
maurolacy approved these changes Jan 13, 2022
View reviewed changes
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Pretty good.

packages/voting-contract/src/multitest.rs
mod voting;

#[test]
fn simple_instantiate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this test? In any case, it shouldn't assert something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will fail if the contract fails to instantiate, which includes the library instantiate fn. Sure, it's tested by literally every other test we have, but I figured it can be helpful when trying to isolate a bug.

packages/voting-contract/src/multitest/suite.rs Outdated Show resolved Hide resolved
packages/voting-contract/src/multitest/voting.rs Show resolved Hide resolved
@maurolacy maurolacy merged commit cad4bf4 into main Jan 13, 2022
@maurolacy maurolacy deleted the 6-voting-contract-tests branch January 13, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maurolacy maurolacy maurolacy approved these changes

@hashedone hashedone Awaiting requested review from hashedone

@ethanfrey ethanfrey Awaiting requested review from ethanfrey

@ueco-jb ueco-jb Awaiting requested review from ueco-jb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Multitests for tgrade-voting-contract
3 participants
@uint @maurolacy @hashedone