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

Update READ.me to make part of mockery more clear #72

Closed
wants to merge 2 commits into from

Conversation

timnew
Copy link
Contributor

@timnew timnew commented Mar 17, 2016

Kind of related to issue #70

  • Register mock node-fetch instead of fetch, since node-fetch is used in the code, which is more straightforward.
  • Move deregisterMock call to afterEach hook, since current implementation might cause mock leak if test code failed
  • Add the instruction to use require rather than import

@wheresrhys
Copy link
Owner

I've just merged some biggish refactors and changes to documentation, including a lot more on use with mockery. I think all your points are addressed, but feel free to open another PR if you can think of any improvements

@wheresrhys wheresrhys closed this Mar 17, 2016
@timnew
Copy link
Contributor Author

timnew commented Mar 17, 2016

OK

@timnew
Copy link
Contributor Author

timnew commented Mar 17, 2016

I still think current implementation might cause mock leak when test failed.

Since when assertion failed, the mockery.deregisterMock('node-fetch') will never get executed.
An easy and safe way to solve the issue is to move this call into afterEach hook, which is guaranteed to be executed after every test, whether it is passed or not.

And there is a typo: mockery.deregisterMock('fetch') should be mockery.deregisterMock('node-fetch')

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