test case for retained message on negated subsciption bug #227
Conversation
test/client-pub-sub.js
Outdated
| } else { | ||
| t.pass('Message not received') | ||
| } | ||
| }, 1000) |
There was a problem hiding this comment.
Can you reduce this timer? Is there another test without a timer?
| var s = connect(setup(broker)) | ||
| var receivedPacket = null | ||
| s.outStream.once('data', function (packet) { | ||
| receivedPacket = packet |
There was a problem hiding this comment.
I think you can just call t.fail() here.
There was a problem hiding this comment.
The test process would not exit using this approach as t.fail() will never be called(once we fix the issue) and the plan takes 3 assertions.
|
@mcollina I've reduced the timer for now. Let me know if you have a better way. |
|
I noticed that we were calling addSubscription as well, which should not happen. Right now the fix has redundant checks in every series function which does not look good. Maybe we could break out of the series in this case ? |
I don't understand. However, +1 to the change. |
|
There are redundant null checks in every series function with my approach. For eg https://github.com/mcollina/aedes/blob/0ea4a0c324f027165c2c2f834f943e08b018d89f/lib/handlers/subscribe.js#L68-L71 and https://github.com/mcollina/aedes/blob/0ea4a0c324f027165c2c2f834f943e08b018d89f/lib/handlers/subscribe.js#L97-L99. Ideally, I would have a check in the first series function and then complete the series somehow. Anyway, we could merge this now and optimize later if required. 👍 |
Hey @mcollina
This PR includes a failing test case for the bug mentioned in #200