obj instanceof http.IncomingMessage hard to create tests for

[twk-b]

https://github.com/caolan/forms/blob/fbb4e5dc6ff93a6e8d89480aff7e278f87403915/lib/forms.js#L71 } else if (obj instanceof http.IncomingMessage) {

that line requires that you really have an instanceof http.IncomingMessage which is not really easy to create in a mock, and then randomly the code runs differently, even though you are pass something that looks very very similar to a request ;)

maybe replace with something like: } else if (obj.body && obj.method && (obj.method === 'POST' || obj.method === 'PUT')) {

[ljharb]

That's a strictly weaker test, that converts a prototype check to a duck type check. I'm not sure how I feel about that.

Typically, mocks themselves are a code smell in tests, indicating that there's a coverage gap. However, http.IncomingMessage doesn't seem that hard to mock:

var http = require('http');
var msg = new http.IncomingMessage();
msg.method = 'POST';
msg.body = { data: true };

In addition, please note https://github.com/caolan/forms/blob/fbb4e5dc6ff93a6e8d89480aff7e278f87403915/lib/forms.js#L82 which does require an IncomingMessage object.

[twk-b]

Really appreciate the library. Just switched to using .handle() from using custom code. Figured it would just work (with previous tests in place), but I found that line while digging into the failure.

I agree it's a slower check, but it is currently checking something that is not required (although maybe intended). That path should be the typical flow, so the performance hit is not real (would typically be done later anyways).

Checking for a type where it is required makes sense. Not sure if formidable is used all over, but maybe handle() should expect a req.body() if that is what is required, simplifying the complexity of this recursive function and then remove the dependency on formidable.

Your suggestion works to fix our tests. Close this as you see fit.

Thank you.