search

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
637 stars 48 forks source link

Travis: Also build with node.js 11 to showcase failure #55

Closed papandreou closed 5 years ago

papandreou commented 5 years ago

Looks like there's also breakage with node.js 11. They should just stop changing those internals already :)

moll commented 5 years ago

Thanks for the heads up! If only people gave up novelty for novelty's sake we could all take some time off. :P

Do you by any chance know whether Node v11.0 works with Mitm.js and it's only the latest, v11.2, that breaks? You haven't looked into why it fails yet, right?

papandreou commented 5 years ago

Didn't drill any further into it, no.

Playing around with nvm just now it appears that it actually does work with node 11.0.0, but that it broke with 11.1.0. Unfortunately nothing seems to stand out in the changelog for 11.1.0. I guess that leaves git bisect.

moll commented 5 years ago

I did a quick diff between Node.js's v11.0 and v11.1 tags and have a feeling I know where the problem is. stream_base_commons.js expects the byte count previously given to onStreamRead to be now given in a mutable property. I'll test if that feeling is correct.

Without knowing much about the reasons, it does sure seem like poor technical design to pass arguments to functions through mutable properties on contexts (this).

moll commented 5 years ago

I got Node v11.1 working in https://github.com/moll/node-mitm/tree/fixes/node-v11.1. v11.2 seems to have made even further changes. I'll get to that soon.

papandreou commented 5 years ago

Haha, awesome, great work! I wasn't even done bisecting yet :)

moll commented 5 years ago

And now pushed what I believe to be the only breaking change between v11.1 and v11.2. Would you mind giving it a spin with any real codebase of yours? I'm unsure if my interpretation of Node's internal socket/handle's shutdown method is exactly right, but if it works for you and me, I'm willing to ship it. ^_^

papandreou commented 5 years ago

@moll, those changes make the unexpected-mitm, assetgraph, httpception, and subfont test suites work with node.js 11.2.0. So all good from here! :tada:

moll commented 5 years ago

Great, thanks! Btw, have any so-called end-user facing webapps you've tested with Mitm.js, too?

papandreou commented 5 years ago

I don't have any such things at the moment, no.

@alexjeffburke, could you help testing in a real app? :)

alexjeffburke commented 5 years ago

@papandreou yeah I think I've got enough stuff at work to hand I can exercise it pretty well :) @moll examples include small clients used as modules larger apps as well the handlers that compose those bits. While we're chatting thanks very much for the support and if you can give me a day or so I'll gladly check things on node 11.2+.

moll commented 5 years ago

Thanks, @alexjeffburke. I'll await for your signal!

alexjeffburke commented 5 years ago

I'm sorry this has taken a little longer than I'd hoped (setting up the testing rig took a small amount of doing) but I can now say that I've seen modules that face the real world run against mitm on node 11.3. Examples include HTTP handlers, clients and in one case a streaming XML parser. I think that is enough to conclude that this is working correctly and is great news 🎉

moll commented 5 years ago

Thank you both for your help! I threw v1.5 out.