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.

Sign up for GitHub

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

Jump to bottom

[mqtt][homie] fix integration tests #5915

Merged
merged 4 commits into from
Sep 6, 2019
Merged

[mqtt][homie] fix integration tests #5915

merged 4 commits into from
Sep 6, 2019

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Aug 11, 2019 •
edited
Loading

Fixes #5905

I reworked the tests a little bit and now they succeed when using an external broker (just change the address of the publishConnection from embeddedConnection.getHost() to the IP/hostname of an external broker).

What I can tell is that the subscription to 'homie/#' is obviously received by moquette but NO topics are send back to the client. When using my loccal mosquitto, this works. I'm out of ideas why this happens. Either paho is really sending the messages with QoS 0 (specs allow the broker to discard those, even if retained and moquette is known to do so), moquette is ignoring teh retained flag or not properly expanding the subscription wildcard.

Signed-off-by: Jan N. Klug [email protected]

@J-N-K J-N-K requested a review from davidgraeff as a code owner August 11, 2019 13:29
@J-N-K J-N-K changed the title fix some test [homie] fix some test Aug 11, 2019
@TravisBuddy
Copy link

TravisBuddy commented Aug 11, 2019 •
edited
Loading

Travis tests were successful

Hey @J-N-K,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@davidgraeff
Copy link
Member

davidgraeff commented Aug 11, 2019 •
edited
Loading

When I initially wrote the tests, it worked with moquette. Since then we have updated moquette from 0.10 to 0.11 and also updated paho. I guess a simple retained topic publish subscribe integration test could be part of the moquette mqtt broker bundle to make sure that this is not the culprit?

@J-N-K J-N-K mentioned this pull request Aug 12, 2019
Closed
@J-N-K J-N-K added external bug A problem or unintended behavior of an external library work in progress A PR that is not yet ready to be merged labels Aug 12, 2019
@J-N-K
Copy link
Member Author

J-N-K commented Aug 16, 2019

@openhab/2-x-add-ons-maintainers How should we proceed here? Obviously the bug is external and will affect everyone using the internal broker.

@J-N-K
Copy link
Member Author

J-N-K commented Aug 20, 2019

Found the bug in moquette for that: moquette-io/moquette#489

@davidgraeff
Copy link
Member

Awesome. The h2 persistence was not fixed by that PR though :/

davidgraeff
davidgraeff approved these changes Aug 26, 2019
View reviewed changes
Copy link
Member

@davidgraeff davidgraeff left a comment

Choose a reason for hiding this comment

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

LGTM. A new moquette release, yeah.

davidgraeff
davidgraeff reviewed Aug 26, 2019
View reviewed changes
...binding.mqtt.homie.tests/src/main/java/org/openhab/binding/mqtt/HomieImplementationTest.java
assertTrue("Connection " + connection.getClientId() + " not retrieving all topics",
c.await(1000, TimeUnit.MILLISECONDS));
// four topics are not under /testnode !
CountDownLatch c = new CountDownLatch(registeredTopics - 4);
Copy link
Member

@davidgraeff davidgraeff Aug 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, with the constant "4" this test class is quite fragile towards extendability. Maybe repurpose the variable to only count "/testnode" topics?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to split off the moquette-thing anyway, it's not directly related. I merely pushed it to show that it's working. I'll check how this can be changed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the constant is still there. Is it easy to fix or should we merge it as-is?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not so easy to fix. I think that the comment will help if someone changes the test and it fails again.

Copy link
Member

@wborn wborn Sep 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

OK let's then merge it! The comment is certainly helpful.

@J-N-K J-N-K removed external bug A problem or unintended behavior of an external library work in progress A PR that is not yet ready to be merged labels Sep 5, 2019
@J-N-K
Copy link
Member Author

J-N-K commented Sep 5, 2019

They all succed in my local build now

@J-N-K
fix some test
10422f2 
Signed-off-by: Jan N. Klug <[email protected]>
J-N-K added 3 commits September 5, 2019 16:15
@J-N-K
remove unnecessary characterset
7c288f4 
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
formatting & comments
00d26af 
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
more formatting
bbf9946 
Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K J-N-K changed the title [homie] fix some test [mqtt][homie] fix integration tests Sep 6, 2019
@J-N-K J-N-K mentioned this pull request Sep 6, 2019
Closed
9 tasks
wborn
wborn approved these changes Sep 6, 2019
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks. These tests now also work on my local build.

@wborn wborn merged commit d1193fe into openhab:master Sep 6, 2019
@J-N-K J-N-K deleted the fix-homie-tests branch September 6, 2019 11:07
baggior pushed a commit to baggior/openhab2-addons that referenced this pull request Sep 6, 2019
@J-N-K @baggior
[mqtt][homie] fix integration tests (openhab#5915)
32867db 
Signed-off-by: Jan N. Klug <[email protected]>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
@J-N-K @ne0h
[mqtt][homie] fix integration tests (openhab#5915)
d595b21 
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Maximilian Hess <[email protected]>
@wborn wborn added this to the 2.5 milestone Sep 29, 2019
@wborn wborn added the test label Oct 1, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Dec 8, 2019
@J-N-K @Pshatsillo
[mqtt][homie] fix integration tests (openhab#5915)
c772698 
Signed-off-by: Jan N. Klug <[email protected]>
tmrobert8 pushed a commit to tmrobert8/openhab-addons that referenced this pull request Jan 21, 2020
@J-N-K @tmrobert8
[mqtt][homie] fix integration tests (openhab#5915)
da5472d 
Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Tim Roberts <[email protected]>
hww3 pushed a commit to hww3/openhab2-addons that referenced this pull request Feb 22, 2020
@J-N-K @hww3
[mqtt][homie] fix integration tests (openhab#5915)
c63e4f2 
Signed-off-by: Jan N. Klug <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidgraeff davidgraeff davidgraeff approved these changes

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
test
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

[mqtt.homie] Integration tests fail
4 participants
@J-N-K @TravisBuddy @davidgraeff @wborn