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

Added unit tests for network.js #1129

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

Conversation

haxwell
Copy link

@haxwell haxwell commented Jan 14, 2023

No description provided.

@theanmolsharma
Copy link
Collaborator

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
Copy link
Author

haxwell commented Feb 7, 2023

@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.

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

Successfully merging this pull request may close these issues.

2 participants