Skip to content

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 👍

@wesleytodd
Copy link
Member

I am a bit confused as to why a fix for bad behavior on Jest's part is something we would want to land? Is this still a problem in Jest today? If so, can we see a report to them about this?

@UlisesGascon
Copy link
Member

@ardiadrianadri is this still an issue? did you find an alternative way to solve this?

@UlisesGascon UlisesGascon self-assigned this Jul 31, 2024
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.

4 participants