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

Fix error when http module is mock in tests #20

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

Conversation

ardiadrianadri
Copy link

Hello:

Let me explain to you the problem that I am trying to solve with this PR. The problem I found is that, when I create a mock with the Jest library of Http for my unit tests, the tests fails. The reason is because the module application.js from the express library, which has a dependency on "methods", obtaining an empty array of methods ... and that ends with an exception. The problem comes because, when I make the mock of the http library with Jest, the value of * http.METHODS * is [] which it is evaluated as true and makes the function * getBasicNodeMethods * never run.

My changes are, basically, to check that he http.METHODS array exist and its length is different of 0 so, it that way, the library http can be mock for unit testing.

Regards

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thanks. Please add a test to prevent this change from regressing.

@jshttp jshttp deleted a comment from coveralls Jan 17, 2019
@jshttp jshttp deleted a comment from coveralls Jan 17, 2019
@ardiadrianadri
Copy link
Author

Test added

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thank you for adding a test, but I apologize, as I don't think I really articulated what my thoughts were on that point when I left the original comment from my phone.

Just like we suppose the Browserify stuff and the tests actually use Browserify, I think that it's find to support this Jest thing you're talking about, but the test should use Jest to assert that we remain compatible with the Jest issue, especially so we know when a future version of Jest changes how this works.

In addition, I forgot to mention the first time, this behavior / Jest thing needs to be documented in the README along with the other support 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants