-
Notifications
You must be signed in to change notification settings - Fork 66
Test: Add createStakingPool and performStakeBurn unit tests #462
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
Test: Add createStakingPool and performStakeBurn unit tests #462
Conversation
3914e81
to
4a80f41
Compare
test/unit/Cover/helpers.js
Outdated
return buyCoverOnMultiplePools.call(this, params, 1); | ||
} | ||
|
||
async function buyCoverOnMultiplePools( |
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 is not clear enough what it does.
It both create pools and buys.
Also it's used in only 1 place.
Let's have a function that's buyCoverOnMultiplePools
but creates the pools before separately. There's only 1 place where this is used.
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'm not sure I fully understood your proposed changes. I applied the following changes 132e088 to only buy the cover on buyCoverOnMultiplePools
and creating the pools outside the function, in the test that uses it and in buyCoverOnOnePool
to not modify the behavior of that function as it is used in several unit tests.
Is something like this what you had in mind?
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 is what i mean yes, good!
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.
some changes necessary.
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.
Some changes necessary.
132e088
to
95a0e7d
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
Context
Closes #274
Changes proposed in this pull request
Adds new unit tests for
createStakingPool
andperformStakeBurn
. Implemented the list of test cases from the issue.Test plan
New tests were added in
test/unit/Cover/createStakingPool.js
andtest/unit/Cover/performStakeBurn.js
Checklist
Review
When reviewing a PR, please indicate intention in comments using the following emojis: