-
Notifications
You must be signed in to change notification settings - Fork 37
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
feat(jellyfish-testing): TestingWrapper as an abstraction layer #1093
base: main
Are you sure you want to change the base?
Conversation
Code Climate has analyzed commit 26981fb and detected 8 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
✔️ Deploy Preview for jellyfish-defi ready! 🔨 Explore the source changes: 26981fb 🔍 Inspect the deploy log: https://app.netlify.com/sites/jellyfish-defi/deploys/62170bb70d071400081bd332 😎 Browse the preview: https://deploy-preview-1093--jellyfish-defi.netlify.app |
Codecov Report
@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
- Coverage 95.69% 95.69% -0.01%
==========================================
Files 169 170 +1
Lines 5045 5113 +68
Branches 657 665 +8
==========================================
+ Hits 4828 4893 +65
- Misses 208 211 +3
Partials 9 9
Continue to review full report at Codecov.
|
return TestingGroup.create(n, init) | ||
} | ||
|
||
createNonMN (): NonMNTesting |
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.
Wanted to start a quick discussion on naming since it will be harder to change later once this starts spreading into our testing structures.
From initial reading NonMN
twists the tongue a little. Wondering if perhaps we want to create a proper name for a reg container that is not a MasterNode. I was thinking perhaps PartialNode?
And then since we are creating individual methods with expected outcomes perhaps the wrapper exposes.
createMasterNode
and createPartialNode
It creates a little more uniformity in naming and I think more clear in their meaning, thoughts?
What this PR does / why we need it:
Tries a different approach to address #637 and #965 instead of #1035
Here we introduce
TestingWrapper
class to create a new layer of abstraction with the goal of being able to create a test /testGroup instance in a standardized manner.creating a new layer seems to introduce the least amount of changes to existing use cases
Also adds the capability to add NonMasternode RegTestContainers to the tests. For this,
NonMNTesting
class has been introduced.Again this is not the most perfect way to do this. But given the restrictions discussed here -> #1035 (comment), this way seems not ugly and at the same time serves the purpose.
Which issue(s) does this PR fixes?:
Fixes #637
Fixes #965
Additional comments?: