search

mqttjs / mqtt-connection

Barebone Connection object for MQTT in node.js
Other
114 stars 25 forks source link

fix tests to work with changes to mqtt-packet #4

Closed jdiamond closed 8 years ago

jdiamond commented 8 years ago

This fixes the tests that were failing due to various changes in mqtt-packet since the last time mqtt-connection was worked on.

That 2MB payload test that I had to increase the timeout for is taking 6-8 seconds to run on my Mac. Seems strange.

Maybe it might also be worth updating the Travis file to include Node 4 and 6? I've never used Travis so I'll leave that to you.

jdiamond commented 8 years ago

The .travis.yml file is trying to install a version of npm that's no longer available?

jdiamond commented 8 years ago

Well, now I can no longer say I've never used Travis before. =)

So it looks like the version of npm that came with 0.8 doesn't work with this package.json. That must be why you were installing that specific version of npm. Do you still want to support 0.8?

The build for 0.10 failed because a Buffer is being passed to Buffer.byteLength() but that's not allowed in 0.10.

While looking into that, I noticed the stack traces for mqtt-packet files don't correspond to the latest. mqtt-connection is still depending on mqtt-packet@^3.0.0.

Using latest mqtt-packet still fails on 0.10, but everything is working on 4 and 6. Do you still want to support 0.10?

mcollina commented 8 years ago

So it looks like the version of npm that came with 0.8 doesn't work with this package.json. That must be why you were installing that specific version of npm. Do you still want to support 0.8?

As you guessed, no.

While looking into that, I noticed the stack traces for mqtt-packet files don't correspond to the latest. mqtt-connection is still depending on mqtt-packet@^3.0.0.

Yes. mqtt-packet v4 does not work on v0.8. Thanks for updating!

Using latest mqtt-packet still fails on 0.10, but everything is working on 4 and 6. Do you still want to support 0.10?

No, we'll bump the major

mcollina commented 8 years ago

@jdiamond do you want to help maintaining this module?

That 2MB payload test that I had to increase the timeout for is taking 6-8 seconds to run on my Mac. Seems strange.

The problem relies on the difference between mqtt-packet v3 and v4. You should update this library to use https://github.com/mqttjs/mqtt-packet#writeToStream and not generate. Here is an example: https://github.com/mqttjs/MQTT.js/blob/v2-dev/lib/client.js#L32-L51.

jdiamond commented 8 years ago

I updated mqtt-connection to use writeToStream. That didn't end up helping make the 2MB tests faster. It seems like it was should.eql that was being slow comparing the 2MB buffers so I changed those two tests to just check the length.

The way writeToStream does multiple writes to its output stream means multiple readable events were being triggered, breaking many tests that assumed they would only get one. I made a helper function (readFromStream) in the tests to deal with that.

writeToStream doesn't really have an API that's usable with through2 so I had to make a custom duplex stream. reduplexer was giving me an error I didn't understand, but it worked with duplexify. I tried to replace the other use of reduplexer with duplexify and got a different error that I'll look into a bit later.

I also tried to update through2 to latest, but the tests failed. I'll look into that later, too.

I think swapping out reduplexer for duplexify (I saw your comments in the reduplexer issues about possibly switching) and updating through2 are the last things needed to modernize mqtt-connection.

mcollina commented 8 years ago

Do you want to help maintaining this and mqtt-packet? I will add you to the organization.

jdiamond commented 8 years ago

I don't mind helping. I think I'm already in the org from last time I helped with MQTT.js.

jdiamond commented 8 years ago

I finished upgrading all of the dependencies and all the existing tests pass quickly now. Let me know if you think anything else needs to be done.