-
Notifications
You must be signed in to change notification settings - Fork 31
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
Stake and mint integration test #634
Stake and mint integration test #634
Conversation
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.
Just some minor comments about documentation.
LGTM 💯
test_integration/02_stake_and_mint/utils/assert_confirm_stake_intent.js
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.
Very nice!! 🙌 Very big PR!
I am super excited about these integration tests 😬
We finally do the actual processes on a real geth node 😍
The concept of an "owner" is an implementation detail of one specific organization implementation. It is not reflected in the interface. It is more about onlyOrganization
and isOrganization
. I would opt to call the addresses organization
addresses instead of owner
addresses. What do you think?
Instead of putting all the stake and mint into a single test file and then creating a bunch of "assert" classes (helpers), maybe it could also work to split the test files and make them contained, including the util functions? 🤔
For example: imagine there was 01_stake
, 02_confirm_stake
, 03_progress_stake
, and so on. Then you could have the assert functions as extracted functions in these files. Tests would be contained and it would also be nicely separated to a reader.
Generally, class names are usually nouns and function names are usually verbs. Giving a class a verb as a name is a smell (assert...
).
I added more comments directly to the code.
test_integration/02_stake_and_mint/utils/assert_progress_mint.js
Outdated
Show resolved
Hide resolved
test_integration/02_stake_and_mint/utils/assert_progress_mint.js
Outdated
Show resolved
Hide resolved
test_integration/02_stake_and_mint/utils/assert_progress_mint.js
Outdated
Show resolved
Hide resolved
…sarvesh-ost/mosaic-contracts into stake_and_mint_integration_test
test_integration/02_stake_and_mint/utils/confirm_stake_intent_assertion.js
Outdated
Show resolved
Hide resolved
Really nice progress The remaining open questions are:
|
…deployment script. Fixed some documention issue based on PR feedbacks
Thank you @schemar and @hobofan for the feedbacks. Need your opinion if the effort of breaking tests into a separate file is worth the efforts. |
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.
Good documentation 👍
Some change requests:
- Check your JSDoc in
progress_stake_assertion.js
andprogress_redeem_assertion.js
. The indentation is off in some places. stake_assertion
line 60: missing space.- Generally:
{string}
is lowercase. It is a primitive, not a class.
Thank you @schemar and @hobofan for the feedbacks.
I have addressed all of them except breakingstake_and_mint.js
into01_stake
,02_confirm_stake
,03_progress_stake
. If we try to break tests into separate files then there will be some added complexity.
All these tests share state for examplestakeRequest
object. There would be added complexity to share these states among tests from different files.Need your opinion if the effort of breaking tests into a separate file is worth the efforts.
@schemar @hobofan @deepesh-kn
It is probably not worth the effort at this time. As long as we understand what we are doing :)
Maybe for redeem you can find a better architecture with classes 👍 |
Thanks 🙌 |
In tests, a general structure that I usually follow: This provides good readability to the reader. Splitting test code into classes reduces readability by breaking above natural structure of the test. That's the reason, I don't want to design tests into several classes. 🙏 Definitely, I am open for more discussion on this. 👍 |
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.
Maybe for redeem you can find a better architecture with classes 👍
In tests, a general structure that I usually follow:
1️⃣ Test data/precondition.
2️⃣ Actual method call.
3️⃣ Assertions.This provides good readability to the reader. Splitting test code into classes reduces readability by breaking above natural structure of the test. That's the reason, I don't want to design tests into several classes. 🙏
Definitely, I am open for more discussion on this. 👍
But you did break the tests into multiple classes (the assertion classes). All I am saying is: if you want to do that, then the current way of helper classes is not ideal.
An alternative would be separate files for stake, etc. as proposed above. Then you would actually have tests contained in a single file. And you could improve readability through functions that are relevant to those tests.
Following the 1, 2, 3 steps above is of course correct. That doesn't mean you cannot improve your code by making it modular.
Fixes #612
Stake and mint integration test.