Skip to content
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

#42 Removed Mockito from NetworkTest #64

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

carlosmiranda
Copy link
Contributor

#42: Removed Mockito from NetworkTest. Added implementations of push() and pull() in Remote.Fake that can be used for testing purposes.

@0crat
Copy link
Collaborator

0crat commented Aug 8, 2018

Job #64 is now in scope, role is REV

@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #64 into master will decrease coverage by 0.26%.
The diff coverage is 72.72%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #64      +/-   ##
============================================
- Coverage     93.03%   92.77%   -0.27%     
  Complexity       49       49              
============================================
  Files            11       11              
  Lines           158      166       +8     
  Branches         10       11       +1     
============================================
+ Hits            147      154       +7     
  Misses           11       11              
- Partials          0        1       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/io/zold/api/Remote.java 81.25% <72.72%> (+6.25%) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f250c...74bf1ed. Read the comment docs.

final Remote lowremote = Mockito.mock(Remote.class);
final Wallet wallet = Mockito.mock(Wallet.class);
final Map<Long, Wallet> highwallets = new HashMap<>();
final Remote high = new Remote.Fake(20, highwallets);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carlosmiranda don't you think Remote.Fake should expose the underlying map instead of having to keep a reference around?

@carlosmiranda
Copy link
Contributor Author

@victornoel please check new commit addressing your comments. Also added TaxesTest, which I missed due to the fact that it seems to be a recent change.

@victornoel
Copy link
Contributor

@carlosmiranda personally I like it better like that yes, also not that I am not the reviewer :)

@carlosmiranda
Copy link
Contributor Author

@victornoel doh. This is not even assigned yet, you are working for free. :P

@victornoel
Copy link
Contributor

@carlosmiranda just defending my interests by ensuring my future usage of those classes is a joy :D

@carlosmiranda
Copy link
Contributor Author

carlosmiranda commented Aug 9, 2018

@victornoel nevertheless it should be avoided, you are basically robbing both yourself and eventual assignee of the job (unless it turns out to be you). It's up to the architect to maintain joyful class usage.

@victornoel
Copy link
Contributor

@carlosmiranda it's true, thanks for the reminder

@carlosmiranda
Copy link
Contributor Author

@llorllale are there no reviewers available? This has become a long running task

@carlosmiranda
Copy link
Contributor Author

@llorllale this is more than a week old now. Can you assign a REV or do the honor yourself?

@llorllale
Copy link
Collaborator

@carlosmiranda it's a funding issue - let me try to resolve it as quickly as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants