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

Add tests for the config #495

Merged
merged 5 commits into from
Mar 3, 2020
Merged

Conversation

bunysae
Copy link
Contributor

@bunysae bunysae commented Feb 9, 2020

Fixes #381. I used proxyquire for mocking the dependencies is-installed-globally and pkg-dir. Mocking with sinon was not possible.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

test/config.js Outdated Show resolved Hide resolved
@itaisteinherz
Copy link
Collaborator

Could you add tests for more edge cases? (e.g. global config is only used when the global np binary is used, when using the local binary only the local config files are used).

@bunysae
Copy link
Contributor Author

bunysae commented Feb 18, 2020

I added tests for the two only-edge cases.

@bunysae bunysae force-pushed the config_test branch 2 times, most recently from fe98489 to 5a43042 Compare February 21, 2020 17:54
@sindresorhus sindresorhus changed the title Added tests for config Add tests for config Feb 22, 2020
test/config.js Outdated Show resolved Hide resolved
test/config.js Outdated Show resolved Hide resolved
test/config.js Outdated Show resolved Hide resolved
test/config.js Outdated Show resolved Hide resolved
test/config.js Outdated Show resolved Hide resolved
@itaisteinherz
Copy link
Collaborator

@bunysae Thanks for working on this! I left a couple of comments for you to take a look at, and overall this looks really solid (just a few implementation details I wanted to sort out).

@bunysae
Copy link
Contributor Author

bunysae commented Feb 27, 2020

After @itaisteinherz comments i refactored the tests.
Now the should be a lot cleaner, contain every edge case without redundancy
and verify only the behavior, not the implementation.

test/config.js Outdated Show resolved Hide resolved
test/config.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@itaisteinherz itaisteinherz left a comment

Choose a reason for hiding this comment

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

@bunysae Thanks for working on this!

@itaisteinherz itaisteinherz changed the title Add tests for config Add tests for the config Mar 3, 2020
@itaisteinherz itaisteinherz merged commit d9848db into sindresorhus:master Mar 3, 2020
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.

Add tests for the config
4 participants