-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: TokenVoting and TokenVotingSetup tests #16
Conversation
422c631
to
fa8a8f0
Compare
fa8a8f0
to
e9b8ec7
Compare
2286b1a
to
d20c0b4
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.
I haven't gone through the whole PR. Only reviewed this unit test of token-voting. I want to first get opinions and whether it makes sense for me to continue reviewing.
- The token-voting unit test file is super big.
- It's really not unit testing because it uses dao and tokens and doesn't mock calls. So not sure why we call it unit test at all.
- I noticed the tendency that most of the tests retests things again and again, which clearly makes the file increase. The 3500 line for token-voting in my opinion is still huge and unmanageable. See my half-review on the PR.
I added my reviews, but stopped in the middle because none of you might agree with me, so I might be reviewing such details for no reason. As for the security thing, at first glance, it's testing all flows, but it took me quite some time to understand the token voting and not sure whether it's worth it to re-learn it at this point.
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Giorgi Lagidze <[email protected]>
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.
This looked like a lot of work. Thank you for getting it done.
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/12_token-voting-setup.ts
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/12_token-voting-setup.ts
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/12_token-voting-setup.ts
Outdated
Show resolved
Hide resolved
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.
let some minor comments
packages/contracts/test/10_unit-testing/base/11_majority-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/base/11_majority-voting.ts
Outdated
Show resolved
Hide resolved
packages/contracts/test/10_unit-testing/token-voting/11_token-voting.ts
Outdated
Show resolved
Hide resolved
5c67555
to
98b4604
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.
LGTM!
This PR refactors the
TokenVoting
andTokenVotingSetup
Task ID: OS-1121