obj instanceof http.IncomingMessage hard to create tests for

[twk-b]

forms/lib/forms.js

Line 71 in fbb4e5d

} else if (obj instanceof http.IncomingMessage) {

} 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

forms/lib/forms.js

Line 82 in fbb4e5d

form.parse(obj, function (err, fields, files) {

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.