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

[MQTT] Fix tests after core change #16774

Merged
merged 2 commits into from
May 19, 2024
Merged

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented May 19, 2024

See openhab/openhab-core#4169 (comment)

Some MQTT tests with QuantityTypes fail after openhab/openhab-core#4169 because the MQTT binding assumes a format pattern of %s to drop the unit of the Number with Dimension. As this was changed in core, this PR introduces some logic in the MQTT binding to account for this.
All failing tests have run again and pass.

Also, default formatting for Number types was not in line with the documentation. It used %s, while the documentation states it should be %f, dropping all decimals (and the unit). It now is for Number types.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from ccutrer as a code owner May 19, 2024 09:23
@mherwege
Copy link
Contributor Author

All test run through fine locally.

The change is in the org.openhab.binding.mqtt.generic package and that should solve the issue for the tests in both the org.openhab.binding.mqtt.generic and org.openhab.binding.mqtt.homeassistant package. Can it be that when running the tests for the org.openhab.binding.mqtt.homeassistant package, it still works with the unchanged compiled code of org.openhab.binding.mqtt.generic? When I run this in my IDE with both packages open and the changes I made, all tests pass.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo
Copy link
Contributor

I am annoyed by the failing builds...

@mherwege mherwege marked this pull request as draft May 19, 2024 10:41
@mherwege
Copy link
Contributor Author

I am annoyed by the failing builds...

Agree. It was working for me in Eclipse. But it looks like Eclipse might not have had the last QuantityType changes. Looking into fixing this.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege marked this pull request as ready for review May 19, 2024 11:47
@mherwege
Copy link
Contributor Author

@lolodomo It should be much better now.

The key challenge remains though that the format method in the core types is used both for formatting for display and when doing these transformations. It leads to all kinds of funny effects.
E.g. if you use %.1f as a format pattern, depending on your locale, you will get a . or ,. That is what you want in the UI, but not in a transformation. And that basically means one has to cope with exceptions in many places. By default transformations will use %s. That should keep everything... except it used to drop the unit from a QuantityType before openhab/openhab-core#4169, which was not nice from a UI perspective, and it caused errors when updating from the UI when the unit was not the default unit.
Simply using a %.0f format on QuantityType works to get you the numeric value, but without digits. %.1f would be locale dependent.
I believe it is solved for all cases in the test set, but I cannot guarantee there are no configurations out there that may have a slightly different behaviour depending on the pattern used. I hope I have been as little intrusive as possible.

@kaikreuzer kaikreuzer merged commit 0ff8564 into openhab:main May 19, 2024
5 checks passed
@kaikreuzer kaikreuzer added this to the 4.2 milestone May 19, 2024
@mherwege mherwege deleted the mqtt_test branch May 19, 2024 12:38
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
PRosenb pushed a commit to PRosenb/openhab-addons that referenced this pull request May 22, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Jun 15, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Paul Smedley <paul@smedley.id.au>
pgfeller pushed a commit to pgfeller/openhab-addons that referenced this pull request Sep 29, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Patrik Gfeller <patrik.gfeller@proton.me>
joni1993 pushed a commit to joni1993/openhab-addons that referenced this pull request Oct 15, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Oct 18, 2024
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
cipianpascu pushed a commit to cipianpascu/openhab-addons that referenced this pull request Jan 2, 2025
* fix mqtt tests

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>
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.

3 participants