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

Use configured locale for SSE displayState #3069

Merged
merged 1 commit into from
Aug 27, 2022
Merged

Use configured locale for SSE displayState #3069

merged 1 commit into from
Aug 27, 2022

Conversation

pacive
Copy link
Member

@pacive pacive commented Aug 22, 2022

Fixes #3067

Signed-off-by: Anders Alfredsson andersb86@gmail.com

Signed-off-by: Anders Alfredsson <andersb86@gmail.com>
@pacive pacive requested a review from a team as a code owner August 22, 2022 07:09
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM, can you add a test that ensures this will not break again? It should ensure that the locale returned by the LocaleService is different from the system-locale and then check that the display state is in the correct format.

@pacive
Copy link
Member Author

pacive commented Aug 22, 2022

I've only ever done simple unit tests unfortunately, not sure how I would go about testing something like this, with so many different parts I'm afraid. If I understand it correctly, it would require a mocked Thing with stateOptions defined for a channel, (in two different languages), an Item connected to that channel, plus configuring the I18nProviderImpl (the default LocaleProvider), and finally calling SseItemStatesEventBuilder.buildEvent(), to check that the correct language is used? I honestly have no idea how to even begin, but if I could get some guidance I would be happy to learn more about it.

@J-N-K
Copy link
Member

J-N-K commented Aug 24, 2022

I think you can mock everything. If you

  • mock the bundle context
  • mock the item registry and let it return return a mocked item when an item is requested
  • let the mocked item return a state and two different state descriptions, one for the system locale and one for another locale
  • mock the locale service and let it return the other locale
  • build the item state and check that it is formatted with the other locale, not the system locale

@pacive
Copy link
Member Author

pacive commented Aug 25, 2022

Ok, please bear with me trying to get my head around this.

  • If I mock the LocaleService, I implicitly make an assumption that this is working as intended.

  • The SseItemStatesEventBuilder passes the locale directly to the getDisplayState() method, which passes it on to the Items getStateDescription() method, which in turn passes it to its configured StateDescriptionService, and that is responsible for getting the StateDescription with the correct locale.

But if I mock this in the Item I thereby make the assumption that the StateDescriptionService part also works, which means the only thing the test will prove is that this method actually gets called. Maybe I'm misunderstanding the purpose of testing, but I can't really see that this proves anything useful?

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Alright, we'll see if it breaks again.

@J-N-K J-N-K merged commit 80794bf into openhab:main Aug 27, 2022
@J-N-K J-N-K added the bug An unexpected problem or unintended behavior of the Core label Aug 27, 2022
@J-N-K J-N-K added this to the 3.4 milestone Aug 27, 2022
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
Signed-off-by: Anders Alfredsson <andersb86@gmail.com>

Signed-off-by: Anders Alfredsson <andersb86@gmail.com>
GitOrigin-RevId: 80794bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REST] /rest/events displayState not localized
2 participants