moll / node-mitm

Intercept and mock outgoing Node.js network TCP connections and HTTP requests for testing. Intercepts and gives you a Net.Socket, Http.IncomingMessage and Http.ServerResponse to test and respond with. Super useful when testing code that hits remote servers.
Other
636 stars 48 forks source link

Simulating error conditions? #15

Closed papandreou closed 8 years ago

papandreou commented 9 years ago

I think one of the major advantages of mocking out the network layer is the ability to gain test coverage of all the weird error handling code that you end up having in your programs. However, I cannot figure out how to provoke errors from within my connection/request handlers that actually propagate to the corresponding mocked out ServerResponse/ClientRequest instances that the tested program code gets.

Here is one of the things I tried:

var Mitm = require("mitm"),
    mitm = Mitm();

mitm.on("request", function (req, res) {
    setImmediate(function () {
        req.emit("error", new Error("foo"));
    });
});

var req = require("http")
    .get("http://www.cqwiejcqwoiejcqoiwjceoiqwjceoiqjce.com/")
    .on("error", function (err) { console.log("req got err", err); })
    .on("response", function (response) {
        console.log("got response");
        response.on("data", function (chunk) {
            console.log("response chunk", chunk.length);
        }).on("end", function () {
            console.log("response end");
        }).on("error", function (err) {
            console.log("response error", err);
        });
    }).end();

which produces:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: foo
    at Object._onImmediate (/home/andreas/work/node-mitm/error.js:6:27)
    at processImmediate [as _immediateCallback] (timers.js:336:15)

I expected the error to be handled by the req error handler.

I also tried emitting error events on res and res.connection, but none of my attempts caused the error to propagate to the application code.

Any suggestions?

moll commented 9 years ago

Hey,

I think one of the major advantages of mocking out the network layer is the ability to gain test coverage of all the weird error handling code that you end up having in your programs.

Definitely.

The req and res given to the request events are actually separate instances from the ones your client code gets, hence emitting error on them doesn't reach those error handlers. They're the server-side objects whose data is piped to and from the client side request objects.

It haven't made up my mind if or how it could be made more elegant, but one way you could get a reference to the client-side Socket instance is through the connectevent (https://github.com/moll/node-mitm/blob/8a843b3ca00c95febea4ddc3c2474933b22816cd/index.js#L77). You could try throwing from in either synchronously (if there are any synchronous errors Node.js throws) or emitting error on it later on. Will that work for you?

If you've got better ideas, I'm all ears.

papandreou commented 9 years ago

one way you could get a reference to the client-side Socket instance is through the connectevent

Thanks for the hint. I played around with it, and it seems to work fine! I am a bit worried about having to keep state between my event handlers for the reasons stated in #14, so in general I would prefer to be able to provoke an error with an HTTP request from the args passed to the request event alone, and likewise for the connection event wrt. non-HTTP sockets.

How about relaying error events emitted on req/res/req.connection to the corresponding object exposed to the program?

moll commented 9 years ago

Something to think about indeed.

Would you mind reminding me what errors are likely to happen (i.e emitted) on the client side? DNS and connecting issues, right, but those happen before any connection or request events. Which particulars are you testing right now?

Relaying error is probably not going to work as that would interfere with regular error handling on the server side objects. One option would be to add a reference to the client side request object to the server side object...

papandreou commented 9 years ago

Would you mind reminding me what errors are likely to happen (i.e emitted) on the client side? DNS and connecting issues

Yes, ENOTFOUND/EADDRINFO when the host name cannot be looked up via DNS, ECONNREFUSED and probably others when a connection cannot be established. And perhaps most importantly (as it is hard to test without mocking out the network or setting up a special purpose server), ECONNRESET and ETIMEDOUT while a request or a response is being streamed.

moll commented 9 years ago

Okay, ENOTFOUND, EADDRINFO and ECONNREFUSED won't result in reqs and ress, so you'd have to handle those in connect anyhow, am I right?

I've used some date and time mocking libraries (e.g. Sinon) to custom request timeout code. As to ETIMEDOUT, that's similar to ECONNREFUSED, but delayed, right, in that it happens before an actual connection is made?

When you call socket.destroy() (and not socket.end()) on the server side, does that result in a ECONNRESET thrown on the client side?

papandreou commented 9 years ago

Okay, ENOTFOUND, EADDRINFO and ECONNREFUSED won't result in reqs and ress, so you'd have to handle those in connect anyhow, am I right?

Hmm, you're right about that. I guess that means that a full error simulation will have to involve all the events emitted by mitm.

I've used some date and time mocking libraries (e.g. Sinon) to custom request timeout code. As to ETIMEDOUT, that's similar to ECONNREFUSED, but delayed, right, in that it happens before an actual connection is made?

AFAIR you can also get a timeout event after the connection has been established. If you use socket.setTimeout to set a "between bytes" timeout.

When you call socket.destroy() (and not socket.end()) on the server side, does that result in a ECONNRESET thrown on the client side?

Yes, the below script outputs err { [Error: socket hang up] code: 'ECONNRESET' }:

var request = require('request'),
    server = require('http').createServer(function (req, res) {
        req.socket.destroy();
    }).listen(1337),
    serverAddress = server.address();

request('http://' + serverAddress.address + ':' + serverAddress.port + '/', function (err) {
    console.log('err', err);
});
moll commented 9 years ago

Yes, the below script outputs err { [Error: socket hang up] code: 'ECONNRESET' }:

What if you call destroy on the "server side" socket Mitm gives you? It'd be nice if it also resulted in ECONNRESET on the client side socket. Then you could almost emulate all of those error cases you mentioned.

Then there's only the ETIMEDOUT after connecting case left to trigger... Do you remember from reading Node's source, is that handled by a plain old setTimeout somewhere? Then one could speed up timeouts with a time mocking library.

papandreou commented 9 years ago

What if you call destroy on the "server side" socket Mitm gives you? It'd be nice if it also resulted in ECONNRESET on the client side socket. Then you could almost emulate all of those error cases you mentioned.

Yup, that works:

var Mitm = require("./"),
    mitm = Mitm();

mitm.on('connect', function (socket) {
    setImmediate(function () {
        socket.destroy();
    });
});

var req = require("http")
    .get("http://www.cqwiejcqwoiejcqoiwjceoiqwjceoiqjce.com/")
    .on("error", function (err) { console.log("req got err", err); })
    .end();

Output:

req got err { [Error: socket hang up] code: 'ECONNRESET' }

That's great -- it's certainly the most intuitive way of triggering that particular error.

moll commented 9 years ago

Not the most intuitive event naming it seems, though, as connect gives you the client side socket. :-) The connection event is the server side socket I had in mind. Does destroying that also trigger ECONNRESET?

papandreou commented 9 years ago

Then there's only the ETIMEDOUT after connecting case left to trigger... Do you remember from reading Node's source, is that handled by a plain old setTimeout somewhere? Then one could speed up timeouts with a time mocking library.

Unfortunately I don't remember. I think the socket timeouts are somehow implemented by libuv using system calls, but I'm not sure.

papandreou commented 9 years ago

Not the most intuitive event naming it seems, though, as connect gives you the client side socket. :-) The connection event is the server side socket I had in mind. Does destroying that also trigger ECONNRESET?

Yes, it does :)

vvo commented 9 years ago

Being able to call .emit('error') on the created ClientRequest object would be cool indeed. So from what I read the current solution is to use .on('connect') right?

vvo commented 9 years ago

req.setTimeout works but then it will only emit a timeout event while it should emit an error event if no timeout listener set as per node documentation.

moll commented 9 years ago

Ah, okay. That reminds me, I believe I've tested timeouts with Sinon.js's Date mocking. You then not bypass the request, but don't respond to it either. You just jump time ahead by your timeout-amount. Have you tried that?

vvo commented 9 years ago

Could you provide an example where you did this? Right now I have tried to simulate this:

var http = require('http');
var Mitm = require('mitm');
var mitm = Mitm();

mitm.on('request', function(req, res) {
  // cannot listen for 'timeout' on req
  setTimeout(function() {}, 1000);
});

var req = http.request('http://www.google.com', function() {});
req.setTimeout(300);
req.end();

// This should trigger an uncaught exception

// I can listen to 'timeout' from here but I would either like node to emit an error right away
// or be able to listen for timeouts in mitm.on('request')
moll commented 9 years ago

I wonder if Node's stuff works too with the following approach. I was using the Request library.

Relevant lines copied from my tests:

beforeEach(function() {
  this.time = Sinon.useFakeTimers("Date", "setTimeout", "clearTimeout")
})
afterEach(function() { this.time.restore() })

it("must reject if timeout", function*() {
  var res = new Api().get("/")
  yield wait(this.mitm, "request")
  this.time.tick((2 * 60 + 1) * 1000)

  var err
  try { yield res } catch (ex) { err = ex }
  err.must.be.an.instanceof(Error)
  err.message.must.equal("ETIMEDOUT")
})
vvo commented 9 years ago

I wonder if Node's stuff works too with the following approach. I was using the Request library.

Yes by default you get an error when the request times out using req.setTimeout

moll commented 9 years ago

I meant perhaps the Request lib was using some other timeout mechanism in addition to Node's core. But if not, I wonder if my example solves your testing goal?

vvo commented 9 years ago

Maybe request uses a different approach yes, I will have to test this. Thanks for your help.

Care to have a small look at: #10 ? Thx

moll commented 9 years ago

Done, @vvo. :tada:

moll commented 8 years ago

I'll close this for now as it seems we came up with a few ideas on how to introduce errors. If I missed something and we can improve Mitm somehow, please point it out! Thanks!

vislamov commented 8 years ago

@moll could you explain what this line yield wait(this.mitm, "request") in your example does? May be post expanded example? Thanks!

moll commented 8 years ago

Good point, @vislamov. I didn't seem to include that helper. It's basically just a obj.once that returns a promise:

function wait(obj, event) { return new Promise(obj.once.bind(obj, event)) }
vislamov commented 8 years ago

@moll I can't make it work. My test lib just times out. I use lab and code. I wonder how your generator function callback works? At what point and who's calling next() on it?