fix: tests that relied on order of events #1534
Conversation
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1534 +/- ##
=======================================
Coverage 86.22% 86.22%
=======================================
Files 13 13
Lines 1321 1321
=======================================
Hits 1139 1139
Misses 182 182 ☔ View full report in Codecov by Sentry. |
|
@robertsLando - yes, this is ready for review. |
robertsLando
changed the title
test/abstract_client.js
Outdated
| }) | ||
|
|
||
| // TODO: all of these test changes have to do with calling end() before the callback is complete. Maybe that should be fixed first |
There was a problem hiding this comment.
Would you mind to do this?
There was a problem hiding this comment.
Depends on what you mean by "this" :) Do you mean "make these tests better" or do you mean "fix the problems that happen when you call client.end from inside a callback?"
IMHO, one of the ugliest problems in this code (MQTT.js), is the spaghetti flow that happens when you call client.end. I've tried to make sense of the order that things get shut down, and there are too many branches and patches and callbacks inside that code to make me confident in any changes to the shutdown code.
If the flow of client.end does get fixed, it needs to be done very carefully. I assume that customers have worked around its oddities (whether they know it or not), and it just really feels like changing how client.end works is opening a large can of worms.
There was a problem hiding this comment.
And I forgot to say that I'm assuming that you want to make these tests better. Sure, I can do that.
There was a problem hiding this comment.
Depends on what you mean by "this" :) Do you mean "make these tests better" or do you mean "fix the problems that happen when you call client.end from inside a callback?"
Sorry for not being clear. I assumed both, anyway I already checked the end() code and it's a real mess so we can keep that for another PR 🙏🏼
test/abstract_client.js
Outdated
| @@ -2983,7 +3019,12 @@ module.exports = function (server, config) { | |||
|
|
|||
| client.on('connect', function () { | |||
| if (!reconnect) { | |||
| client.publish('topic', 'payload', { qos: 2 }) | |||
| client.publish('topic', 'payload', { qos: 2 }, function (err) { | |||
| server2.close() | |||
There was a problem hiding this comment.
Does server.close expects a callback too?
There was a problem hiding this comment.
It looks like it does. Do you think I should update the tests to use it? I assume it will help, but I can't guarantee that it won't cause other problems.
|
@BertKleewein Should this fix flaky tests problem? I noticed that sometime tests fail with no reason |
|
@BertKleewein kindly ping |
|
@robertsLando - it should fix some flaky tests. I don't know if it will fix the flaky tests that you were seeing though. :) |
…ket (to a stream that's already closed)
|
@robertsLando - ready again. I think this second set of changes really improves these tests. I found a few things that might explain general flakiness. |
| /** | ||
| * These tests try to be consistent with names for servers (brokers) and clients, | ||
| * but it can be confusing. To make it easier, here is a handy translation | ||
| * chart: | ||
| * | ||
| * name | meaning | ||
| * ---------------|-------- | ||
| * client | The MQTT.js client object being tested. A new instance is created for each test (by calling the `connect` function.) | ||
| * server | A mock broker that you can control. The same server instance is used for all tests, so only use this if you plan to clean up when you're done. | ||
| * serverBuilder | A factory that can make mock test servers (MQTT brokers). Useful if you need to do things that you can't (or don't want to) clean up after your test is done. | ||
| * server2 | The name used for mock brokers that are created for an individual test and then destroyed. | ||
| * serverClient | An socket on the mock broker. This gets created when your client connects and gets collected when you're done with it. | ||
| * | ||
| * Also worth noting: | ||
| * | ||
| * `serverClient.disconnect()` does not disconnect that socket. Instead, it sends an MQTT disconnect packet. | ||
| * If you want to disconnect the socket from the broker side, you probably want to use `serverClient.destroy()` | ||
| * or `serverClient.stream.destroy()`. | ||
| * | ||
| */ |
There was a problem hiding this comment.
Thanks for this! Will be super useful 👍🏼
No description provided.