Skip to content

Add tests for the config#495

Merged
itaisteinherz merged 5 commits into
sindresorhus:masterfrom
bunysae:config_test
Mar 3, 2020
Merged

Add tests for the config#495
itaisteinherz merged 5 commits into
sindresorhus:masterfrom
bunysae:config_test

Conversation

@bunysae

@bunysae bunysae commented Feb 9, 2020

Copy link
Copy Markdown
Contributor

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

Comment thread test/config.js Outdated
@itaisteinherz

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

bunysae commented Feb 18, 2020

Copy link
Copy Markdown
Contributor Author

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
Comment thread test/fixtures/local_config/.np-config.json Outdated
@sindresorhus sindresorhus changed the title Added tests for config Add tests for config Feb 22, 2020
Comment thread test/config.js Outdated
Comment thread test/config.js Outdated
Comment thread test/config.js Outdated
Comment thread test/config.js Outdated
Comment thread test/config.js Outdated
@itaisteinherz

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

bunysae commented Feb 27, 2020

Copy link
Copy Markdown
Contributor Author

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.

Comment thread test/fixtures/config/homedir1/.np-config.json
Comment thread test/config.js Outdated
Comment thread test/config.js Outdated

@itaisteinherz itaisteinherz left a comment

Copy link
Copy Markdown
Collaborator

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