bcoin-org / bcoin

Javascript bitcoin library for node.js and browsers
https://bcoin.io
Other
3.01k stars 811 forks source link

Added unit tests for network.js #1129

Open haxwell opened 1 year ago

theanmolsharma commented 1 year ago

General thoughts:

  1. Some of the it() blocks can be combined into just one block.
  2. The commits should be squashed into a single commit unless adding a commit is relevant. Commits like lint fixes and nits must be squashed.
  3. We shouldn't add additional dependencies unless absolutely required.
haxwell commented 1 year ago

@theanmolsharma Thank you for your review.

  1. Each of the it() blocks is to cover a specific condition in the code. The goal is to get 100% coverage. I could merge some of the tests, but there would be paths which were untested.

  2. I will keep this in mind. Please tell me your opinion, though.. When the reviewer-with-write-access merges the PR, they have the option to squash all the PR commits at that time. The same end goal is accomplished. Would you consider that the same either way, whether I do it, or the reviewer does it? Or no?

  3. I am of the opinion that 'sinon', is necessary. If not it, then some other mocking framework. I may be able to test without its functionality, but the test will not be nearly as clear. It is indeed a new library, but I say it is far from clutter.