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

[awattar] Refactor and add test coverage #17752

Merged
merged 13 commits into from
Jan 15, 2025
Merged

[awattar] Refactor and add test coverage #17752

merged 13 commits into from
Jan 15, 2025

Conversation

tl-photography
Copy link
Contributor

PR does not change behaviour.

Only pushes code coverage and code maintainability.

@tl-photography tl-photography marked this pull request as draft November 15, 2024 23:44
@jlaur jlaur changed the title Feature/push code coverage [awattar] Feature/push code coverage Nov 15, 2024
@jlaur
Copy link
Contributor

jlaur commented Nov 16, 2024

@tl-photography - thanks. Did you intend to include the two commits from #17729? In that case, we need to add label "awaiting other PR" to mention this dependency.

@tl-photography
Copy link
Contributor Author

Ah ok, I marked it as draft only.

Can I set this label too, otherwise please do so.

@jlaur jlaur added the awaiting other PR Depends on another PR label Nov 16, 2024
Wolfgang1966
Wolfgang1966 approved these changes Nov 29, 2024
View reviewed changes
Copy link

@Wolfgang1966 Wolfgang1966 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your work and your patience!

@lolodomo
Copy link
Contributor

I just merged #17729

@tl-photography tl-photography marked this pull request as ready for review November 30, 2024 20:29
@tl-photography
Copy link
Contributor Author

tl-photography commented Nov 30, 2024 •
edited
Loading

@lolodomo

Thanks! PR should be good. 👍

Edit: Should I rebase?

@lolodomo lolodomo removed the awaiting other PR Depends on another PR label Dec 1, 2024
@jlaur
Copy link
Contributor

jlaur commented Dec 3, 2024

Edit: Should I rebase?

Yes, please.

@tl-photography tl-photography force-pushed the feature/push-code-coverage branch from 074b441 to 88312d1 Compare December 3, 2024 18:36
@tl-photography
Copy link
Contributor Author

Edit: Should I rebase?

Yes, please.

done

@tl-photography
[aWATTar] push test coverage and improve code readability
71b17c0 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] rename now to pointInTime
11f60da 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] comply to naming convertion, simplify calc, fix typo
a5a7e7c 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] change getNow to return an instant
2cc529b 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] fix more naming and remove outdated variable
70a9c79 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] make use of clock object
77d372f 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
[awattar] revert api logic, was nicer that way
c5eec4e 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography tl-photography force-pushed the feature/push-code-coverage branch from c2c8b60 to c5eec4e Compare December 22, 2024 22:20
@tl-photography
Copy link
Contributor Author

i had conflicts and resolved them via rebase

@tl-photography
[awattar] added tests for refresh logic
3b2ccad 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
Copy link
Contributor Author

I also added now tests for the refresh logic, since this was easy to add after the clock rework.

@tl-photography
Copy link
Contributor Author

Coverage is now at 57% instr respectively 51% branch coverage.

image

@tl-photography
Copy link
Contributor Author

@jlaur @lsiepel could you have a look into this PR?

@jlaur
Copy link
Contributor

jlaur commented Jan 13, 2025 •
edited
Loading

could you have a look into this PR?

In general, there is now the problem that the Clock object is created in the handler factory with the time-zone configured at the time when the factory is created. Previously TimeZoneProvider was passed to the handlers and getTimeZone() was called when the time-zone was needed. This means that when the time-zone is reconfigured, this will no longer be picked up by the binding.

There are probably multiple ways to resolve this. I think first step would be to pass TimeZoneProvider to the handlers and revert the changes where ZoneId is passed rather than TimeZoneProvider. I'll give one example in the code.

I think you can then separate the need for injecting the current time and the need for injecting the time-zone. So if you provide the Clock (as you now do) and TimeZoneProvider as well, you can use Clock for retrieving the current Instant and you can use TimeZoneProvider for retrieving the current ZoneId. EDIT: See also Clock.withZone(ZoneId)

I hope I'm not missing anything.

@tl-photography tl-photography force-pushed the feature/push-code-coverage branch from 5684ebd to 2f396fd Compare January 14, 2025 19:34
@tl-photography
Copy link
Contributor Author

@jlaur

I introduced a new class, AwattarTimeProvider to abstract the usage of TimeZoneProvider and Clock, as well having a place to mock.

This should address your concerns... i hope.

@tl-photography
[awattar] introduce AwattarTimeProvider
adde35c 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography tl-photography force-pushed the feature/push-code-coverage branch from 2f396fd to adde35c Compare January 14, 2025 19:47
jlaur
jlaur reviewed Jan 14, 2025
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks, much cleaner! I have added a few minor comments, but overall looks good.

@tl-photography
[awattar] introduce AwattarTimeProvider
08bbea7 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@tl-photography
Copy link
Contributor Author

Thanks for the suggestions, I added them with the last commit.

If its good, then I would squash the commits together before merging.

@jlaur
Copy link
Contributor

jlaur commented Jan 15, 2025

If its good, then I would squash the commits together before merging.

This is not needed, I will squash them when merging. 🙂

@tl-photography
[awattar] sort members
676030f 
Signed-off-by: Thomas Leber <thomas@tl-photography.at>
@jlaur
Remove excess line
28e578e 
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
jlaur
jlaur approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jlaur jlaur changed the title [awattar] Feature/push code coverage [awattar] Refactor and add test coverage Jan 15, 2025
@jlaur
Remove empty line
cfa4641 
Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
@jlaur jlaur merged commit baaaf7f into openhab:main Jan 15, 2025
2 checks passed
@jlaur jlaur added this to the 5.0 milestone Jan 15, 2025
@tl-photography
Copy link
Contributor Author

Merci :)

@tl-photography tl-photography deleted the feature/push-code-coverage branch January 15, 2025 22:16
chingon007 pushed a commit to chingon007/openhab-addons that referenced this pull request Jan 22, 2025
@tl-photography @chingon007
[awattar] Refactor and add test coverage (openhab#17752)
cdd9e0a 
* [aWATTar] push test coverage and improve code readability

Signed-off-by: Thomas Leber <thomas@tl-photography.at>
Signed-off-by: chingon007 <tron81@gmx.de>
chilobo pushed a commit to chilobo/openhab-addons that referenced this pull request Feb 10, 2025
@tl-photography @chilobo
[awattar] Refactor and add test coverage (openhab#17752)
31e102b 
* [aWATTar] push test coverage and improve code readability

Signed-off-by: Thomas Leber <thomas@tl-photography.at>
Signed-off-by: Christian Koch <78686276+chilobo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lsiepel lsiepel lsiepel left review comments

@jlaur jlaur jlaur approved these changes

@Wolfgang1966 Wolfgang1966 Wolfgang1966 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@tl-photography @jlaur @lolodomo @lsiepel @Wolfgang1966