-
Notifications
You must be signed in to change notification settings - Fork 1k
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
UT for Version/Verack messages #853
Conversation
Could you review your indentation? |
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.
Nice job, Rickk Hearth @lock9.
It will be crucial to have this part with better UTs. Great initiative.
I will check the code again asap.
@shargon merged! Thanks! |
Removing shutdown; Initializing testblockchain during class setup (only once)
…into ut-version-verack # Conflicts: # neo.UnitTests/UT_RemoteNode.cs
Codecov Report
@@ Coverage Diff @@
## master #853 +/- ##
==========================================
+ Coverage 43.56% 44.72% +1.16%
==========================================
Files 177 177
Lines 12577 12577
==========================================
+ Hits 5479 5625 +146
+ Misses 7098 6952 -146
Continue to review full report at Codecov.
|
Great job, Ricardo! +1,13% that are hard to achieve :) |
Ricardo, congratulations! Fine job! Can we remove your last commit? So we merge first #857 Then we format it again here after merge. It will create a much simpler merge. [EDIT:] don't worry about the other PR... this one will probably go first. |
Ricardo, I'll try to pass basic formatting at once, to simplify your PR changes here. Easier to evaluate. [EDIT:] Ok! Dropped from 20 files to just 5 ;) Time to review! |
Ricardo, I think it's fine for me now, ready for approval. Please confirm if changes are good, I removed many auxiliar classes you did... but I think it's enough for testing. Otherwise we can undo my commits and try again. |
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 PR since the beginning, but even better now.
3ad56f7
@erikzhang please take a look at last commit... it tries to move Actor creation to NeoSystem class, through CreateActor method. This makes easier to check mailboxes, and also to create test actors when necessary (just need to mock NeoSystem class). I think NeoSystem should be available on Peer class, so it's inherited on LocalNode.. just need to find a good way to do that. Some actors may still be created outside NeoSystem, but the most important ones are there. |
Please revert the last commit...We should not create all actors in NeoSystem. It is not used to do this. |
This reverts commit 3ad56f7.
@erikzhang @lock9 I managed to make it work without the parameter passing, and avoiding all hard tricks... Solution was to instantiate ProtocolHandler using |
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.
Only few changes now, nothing big affected. Fine for me.
Erik master yoda. |
* UT for Version/Verack messages * Indentation * Indentation * Update ProtocolHandler.cs * Fixing ClassInit; Removing shutdown; Initializing testblockchain during class setup (only once) * Running formatter * Eriks suggestion * fix typo * removed ProtocolFactory * remove TestProtocolFactory * simplify * CreateActor * Revert "CreateActor" This reverts commit 3ad56f7. * removed parameter from RemoteNode * fix * removed unused parameters * format
Unit tests for RemoteNode and ProtocolHandler actors (Version and Verack only)