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 for #158 #170

Merged
merged 6 commits into from
Nov 14, 2017
Merged

test for #158 #170

merged 6 commits into from
Nov 14, 2017

Conversation

cordovapolymer
Copy link
Contributor

@mcollina
Please see my test for #158
It doesn't fail without the patch from #158 ..

Am I doing it right?

@mcollina
Copy link
Collaborator

Subscriptions are cleared on connect https://github.com/cordovapolymer/aedes/blob/5a4210ad477bd2477be2748a19485a4e5f7a173b/lib/handlers/connect.js#L96-L99. You should put some assertions on the persistence itself if you want to check that they should not be stored.

@cordovapolymer
Copy link
Contributor Author

@mcollina
I've added the assertion on persistence subscriptions, can you review?

var subscriber = connect(setup(broker), { clean: true, clientId: 'abcde' })

subscribe(t, subscriber, 'hello', 0, function () {
subscriber.inStream.end()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should check after this, not after reconnection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! corrected that, now it fails on a master without a patch from @sundryp

@mcollina
Copy link
Collaborator

Can you remove the yarn-lock file?

@cordovapolymer cordovapolymer changed the title test for #158 (not to merge) test for #158 Nov 14, 2017
@mcollina mcollina merged commit c0da871 into moscajs:master Nov 14, 2017
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.

2 participants