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

[astro] Fix returning wrong sun phase name #14078

Merged
merged 9 commits into from
Dec 31, 2022
Merged

[astro] Fix returning wrong sun phase name #14078

merged 9 commits into from
Dec 31, 2022

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Dec 27, 2022 •
edited
Loading

While troubleshooting issue 7642 first a minor bug was found in the calculation of sun phases.

In Sun.getSunInfo method for each phase (sunrise, sunset etc.) a range (start/end datetime) where that phase is valid for is calculated. At the end of all the calculations the current datetime instead of the method parameter was checked against these ranges to determine the current phase.
This behavior is for almost all use-cases perfectly fine as the binding usually only shows the current sun phase. But when performing unit tests, this limits you to the current date and prevents you from checking historical phases etc. So, i changed it to the method parameter.

Also added three tests related to the issue. This revealed that the Map holding the ranges for all subphases is not sorted. giving strange results when null values get involved.

So, two fixes:

  1. Fixed sorting of sun phases /ranges
  2. Using SunPhase Calendar parameter instead of Calendar.Instance (now)

Closes #7642
Closes #12746
Closes #12767

@lsiepel
Add tests and fix very minor bug
bb7085b 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
spotless
3aaf598 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
Correct wrong test
d96994b 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel lsiepel added bug An unexpected problem or unintended behavior of an add-on test labels Dec 27, 2022
@lsiepel lsiepel requested a review from gerrieg as a code owner December 27, 2022 22:08
@lsiepel lsiepel added the work in progress A PR that is not yet ready to be merged label Dec 29, 2022
@lsiepel lsiepel marked this pull request as draft December 29, 2022 12:55
This was referenced Dec 29, 2022
Closed
Closed
@lsiepel
Update tests and fix sorting
6537b09 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
Some checkstyle improvements
b3ac412 
Signed-off-by: lsiepel <leosiepel@gmail.com>
This was linked to issues Dec 30, 2022
[astro] EVENING_NIGHT and MORNING_NIGHT are undef #12746
Closed
[astro] LocalSun_SunPhaseName giving wrong results #12767
Closed
@lsiepel lsiepel changed the title [astro] Add tests and fix historical calculations [astro] Fix returning wrong sun phase name Dec 30, 2022
@lsiepel lsiepel removed the work in progress A PR that is not yet ready to be merged label Dec 30, 2022
@lsiepel lsiepel marked this pull request as ready for review December 30, 2022 23:37
@jlaur jlaur removed the test label Dec 31, 2022
@lsiepel
Merge remote-tracking branch 'upstream/main' into astro
03fbad4 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
conflict fix
2ac11fe 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
Strange merge fix :-/
d76b62f 
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
review comment
bc16701 
Signed-off-by: lsiepel <leosiepel@gmail.com>
lolodomo
lolodomo approved these changes Dec 31, 2022
View reviewed changes
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 lolodomo merged commit 71ef3d9 into openhab:main Dec 31, 2022
@lolodomo lolodomo added this to the 4.0 milestone Dec 31, 2022
@lolodomo
Copy link
Contributor

Is it something we should cherry-pick in 3.4.x ?
@jlaur

@lsiepel lsiepel deleted the astro branch December 31, 2022 14:16
@jlaur
Copy link
Contributor

jlaur commented Dec 31, 2022

@lsiepel - good job hunting and nailing this long-time bug!

@lolodomo - I'm not sure. It's an equation of risk vs. impact. It seems this bug happens rarely (next time May?), but how sure are we that this fix will not cause any regressions? Perhaps we should leave it for 4.0 so we would have a full release cycle for detecting any regressions. I didn't look closely into the details, but please correct me if you believe the risk is negligible.

@lolodomo
Copy link
Contributor

It seems this bug happens rarely (next time May?), but how sure are we that this fix will not cause any regressions? Perhaps we should leave it for 4.0 so we would have a full release cycle for detecting any regressions.

Ok

@lsiepel
Copy link
Contributor Author

lsiepel commented Dec 31, 2022

@lsiepel - good job hunting and nailing this long-time bug!

@lolodomo - I'm not sure. It's an equation of risk vs. impact. It seems this bug happens rarely (next time May?), but how sure are we that this fix will not cause any regressions? Perhaps we should leave it for 4.0 so we would have a full release cycle for detecting any regressions. I didn't look closely into the details, but please correct me if you believe the risk is negligible.

Agree, hope the milestones will get some feedback.

borazslo pushed a commit to borazslo/openhab-mideaac-addon that referenced this pull request Jan 8, 2023
@lsiepel @borazslo
[astro] Fix returning wrong sun phase name (openhab#14078)
4ca7cad 
* Add tests and fix very minor bug
* Correct wrong test
* Update tests and fix sorting
* Some checkstyle improvements

Signed-off-by: lsiepel <leosiepel@gmail.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
@lsiepel @nemerdaud
[astro] Fix returning wrong sun phase name (openhab#14078)
9c48fc0 
* Add tests and fix very minor bug
* Correct wrong test
* Update tests and fix sorting
* Some checkstyle improvements

Signed-off-by: lsiepel <leosiepel@gmail.com>
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@lsiepel @renescherer
[astro] Fix returning wrong sun phase name (openhab#14078)
cf17b34 
* Add tests and fix very minor bug
* Correct wrong test
* Update tests and fix sorting
* Some checkstyle improvements

Signed-off-by: lsiepel <leosiepel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lolodomo lolodomo lolodomo approved these changes

@clinique clinique clinique approved these changes

@gerrieg gerrieg Awaiting requested review from gerrieg

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
4.0
4 participants
@lsiepel @lolodomo @jlaur @clinique