-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add more tests for create multisig #466
Conversation
bbb8aa2
to
fc24c23
Compare
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.
Left some minor suggestions but mainly on naming convention or typos.
I think in this test context it makes sense to have the several describe blocks to logically separate each screen in the flow 👍
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.
Thanks for this.
I'd like to do many more checks such as the ones in the issue, once we passed the creation. Verify the pending Tx (a batch or a remark depending on whether it's been creating with or without proxy).
that there's a remark created
verify the behavior if we already have watched accounts with multisigs, e.g there's no big page with "Multisig creation in progress" but rather a banner "your new multisig is being created..."
check that for a pure creation, there's a pure creation TX pending.
But also things like some error happening when for instance we type in twice the same signatory.
Also we should actually create the pure, and validate the waiting TX for the pure creation.
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.
Please correct the account and remove the chopsticks config. There are an infinity of empty accounts, no need to fake one.
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.
Please re-read my comments. I feel like half of what I suggested was ignored, without comment.
f7358bb
to
a787fb1
Compare
…test_create_multisig
I added some checks code to:
|
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.
lmk @asnaith if there's anything important that you think is missing. Anton fixed the main issue, and I've added all the test I mentioned in my comments or that where mentioned in the issue. I think this is a pretty deep coverage.
Ideally, we should now add not only the creation of the multisig, but also the pure, and verify that the UI updates showing the pure in the header once it's available. This is outside of the scope of this issue, but I think it could be added by just extending the test with the pure creation.
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.
@Tbaut Took some time to look through it again, I think we're all good. This is very thorough now :)
Pushed a minor wording change on a couple of test names
Amazing thanks |
closes #445