Skip to content
This repository has been archived by the owner on Dec 10, 2020. It is now read-only.

Unit tests for all components #70

Merged
merged 1 commit into from
Dec 1, 2018
Merged

Unit tests for all components #70

merged 1 commit into from
Dec 1, 2018

Conversation

vpulim
Copy link
Contributor

@vpulim vpulim commented Nov 17, 2018

This PR adds unit tests for all components.

  • logging
  • blockchain
  • handler
  • net/peer
  • net/peerpool
  • net/protocol
  • net/server
  • rpc
  • service
  • sync
  • util
  • node

@coveralls
Copy link

coveralls commented Nov 17, 2018

Coverage Status

Coverage decreased (-3.2%) to 92.703% when pulling 231c166 on tests into a45c129 on master.

@vpulim vpulim force-pushed the tests branch 3 times, most recently from 3e128f9 to 261d3bb Compare November 17, 2018 20:35
@holgerd77
Copy link
Member

Whew, list completion makes some good progress. 👍 Out of interest: did you find (significant) bugs along the way? Or is everything just working as intended?

@vpulim
Copy link
Contributor Author

vpulim commented Nov 27, 2018

@holgerd77
Copy link
Member

Just out of interest, no need to change here on the last stages: why are you doing this in such a monster 🐲 PR and not adding test types PR by PR? Does this have advantages regarding working out utility functions or something?

@vpulim
Copy link
Contributor Author

vpulim commented Nov 28, 2018

No real advantages. Just wanted to avoid polluting the commit history. Also, I underestimated the amount of work 😂

In hindsight, if I had known how much work it would be, I would have created a tracking issue and done this PR by PR so that it would have been easier for others to contribute tests.

@holgerd77
Copy link
Member

Hehe. 😛

@holgerd77
Copy link
Member

Whew. A whopping > 2.000 lines of code added. Not in all cases of software dev a sign for quality, but in this case really impressive! 👍

@vpulim
Copy link
Contributor Author

vpulim commented Dec 2, 2018

😅Thanks @holgerd77. Not sure if it was worth it, but I did catch a few bugs and hopefully will identify any future bugs/typos. Now, on to integration tests! These should be more useful I think.

@holgerd77
Copy link
Member

Think so too, this gives a very solid foundation and will likely pay off in the future! 😄

@vpulim vpulim mentioned this pull request Dec 6, 2018
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants