Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: transfer aedes tests to the project #7

Merged
merged 8 commits into from
Oct 9, 2023
Merged

Conversation

getlarge
Copy link
Member

@getlarge getlarge commented Oct 5, 2023

@getlarge
Copy link
Member Author

getlarge commented Oct 5, 2023

The current approach is not the most elegant as i duplicated aedes tests inside the codebase to have reliably running tests. Albeit by now i could move those changes back to the local aedes node_modules and trigger patch-package.
The good news is that we can run tap tests with the node:test runner, that's a really great effort!
Another good news is that i know the issue you met is (most probably) related with patchedDeliverFunc, and i'm currently trying to fix it.

@robertsLando
Copy link
Member

The good news is that we can run tap tests with the node:test runner, that's a really great effort!

That's so good! 🎉

Another good news is that i know the issue you met is (most probably) related with patchedDeliverFunc, and i'm currently trying to fix it.

Awesome, let me know when this is ready 💪🏼

@getlarge
Copy link
Member Author

getlarge commented Oct 5, 2023

Found it.
When subscribing, the delivery function is patched after being passed to Aedes.subscribe and this patched function is then stored by MQEmitter (or rather Qlobber).
When unsubscribing, the delivery function is retrieved from the Subscription - created during handleSubscribe - before being passed to Aedes.unsuscribe and then MQEmitter, to be removed. But this is the original function (not the patched one).
I let you figure out the damage that follows...

It seems like the Subscription has to be patched and then the patched Subscription.func should be passed to Aedes.subscribe.

@robertsLando
Copy link
Member

I let you figure out the damage that follows...

Yeah in fact as I said there was some really weird behaviour and this explains it. I think that adding the tests should cover us on almost all this kind of issues 💪🏼

@getlarge getlarge marked this pull request as ready for review October 8, 2023 07:03
package.json Show resolved Hide resolved
patches/aedes+0.50.0.patch Show resolved Hide resolved
@getlarge getlarge requested a review from robertsLando October 8, 2023 07:55
package.json Show resolved Hide resolved
patches/aedes+0.50.0.patch Show resolved Hide resolved
patches/aedes+0.50.0.patch Show resolved Hide resolved
src/aedes-instrumentation.ts Outdated Show resolved Hide resolved
src/aedes-instrumentation.ts Show resolved Hide resolved
@getlarge getlarge merged commit 9d0e67c into main Oct 9, 2023
4 checks passed
@getlarge getlarge deleted the test-use-aedes-tests branch October 9, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Strange behaviour with retained packets
2 participants