-
-
Notifications
You must be signed in to change notification settings - Fork 457
Fix persistence extensions test reliability #4964
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes reliability issues in persistence extensions tests by reducing the impact of timing dependencies on test outcomes. The main change is eliminating tests that rely on the current time (now()) for OnOff type average calculations, specifically removing AverageSince and AverageUntil tests in favor of only testing AverageBetween.
Key changes:
- Replaces hardcoded year values with dynamic calculations based on current year to reduce time dependencies
- Removes time-sensitive average tests for OnOff types (
AverageSinceandAverageUntil) - Consolidates Riemann sum tests to use only
RiemannSumBetweenmethod - Includes commented-out debugging code for adding delays to test queries
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| TestPersistenceService.java | Updates hardcoded year constants to be relative to current year and adds a now() helper method |
| TestCachedValuesPersistenceService.java | Adds commented debugging code for introducing delays in persistence queries |
| PersistenceExtensionsTest.java | Removes AverageSince/AverageUntil tests and consolidates to AverageBetween tests only |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...ersistence/src/test/java/org/openhab/core/persistence/extensions/TestPersistenceService.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/openhab/core/persistence/extensions/TestCachedValuesPersistenceService.java
Outdated
Show resolved
Hide resolved
...istence/src/test/java/org/openhab/core/persistence/extensions/PersistenceExtensionsTest.java
Show resolved
Hide resolved
|
I have removed 2 more tests to reduce potential issues. All remaining tests now work even if there would be a 1 minute delay in the persistence query (tested with 1 minute, but I expect it can even be more). I think that should cover stability issues. |
|
This is now ready for review. |
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
f35ba39 to
746f569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
See discussion: #4818 (comment)
This reduces the impact of
now()on the persistence extensions test reliability. There only was a major impact for average on OnOff types. This impact has been removed by not testing theAverageSinceandAverageUntilmethods, only theAverageBetweenmethod.@jimtng @Nadahar FYI
EDIT: I am running the tests with a sleep of 1 minute on every query to the persistence service. If this succeeds, I consider the persistence extension tests to be independent from the current time for all practical purposes.