Issue 534 fix date utils test #535
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
a +2/-2 hours offset will only fail around midnight, whereas a +12/-13 hours offset ensures that any system will fail using one of them because the current date will fall into a different day.
Every time there's a time offset change, we need to ensure we use an updated "today at start of day" epoch
Using epoch values was working but it's harder to understand by humans than actual time strings. Also, by using OffsetDateTime values and parsing strings with java.time we are ensuring as well that our test values are correctly parsed from string to date times because java.time is a battle tested Java API. Now the only thing that could be wrong in the tests are the input and output time strings (both in human readable format, easy to check) and the offset definitions.
This way, we can parameterize the test and add more cases and context
Same reasons as a couple of commits before, where we argued that using OffsetDateTime.parse is easier to understand and safer.
Same as we did with DateUtils.parseTime
It doesn't feel important now that we're improving the test suite and adding more context to DateUtils
Let's use LocalDate.parse to verify we produce valid XML date strings
Previously, different JVMs could produce different textual formats for date parts (even the same JVM vendor and version in different environments) The test now verifies what we really want to verify: that DateUtils.format produces localized outputs
The creation of the values for January and Sunday only depends on the locale
The default locale is reset after the block is run
Now the expected values are created where they're used. Less indirection
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
==========================================
- Coverage 53% 52.9% -0.1%
Complexity 3111 3111
==========================================
Files 245 245
Lines 13340 13340
Branches 2566 2566
==========================================
- Hits 7071 7058 -13
- Misses 5451 5464 +13
Partials 818 818
Continue to review full report at Codecov.
|
ggalmazor
commented
Jan 30, 2020
src/test/java/org/javarosa/core/model/utils/test/DateUtilsTests.java
Outdated
Show resolved
Hide resolved
lognaturel
approved these changes
Feb 3, 2020
src/test/java/org/javarosa/core/model/utils/test/DateUtilsParseTimeTests.java
Show resolved
Hide resolved
| @@ -77,27 +60,25 @@ public void testParseTime() { | |||
| // - We're effectively binding all times to the EPOCH date | |||
There was a problem hiding this comment.
Is this comment still accurate? Aren't all times now bound to whatever the current time of the device is?
lognaturel
reviewed
Feb 3, 2020
| import org.junit.runners.Parameterized; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class DateUtilsFormatSanityCheckTests { |
There was a problem hiding this comment.
This could probably be renamed. DateUtilsFormatTests would be better, I think.
There was a problem hiding this comment.
This one was tricky to name. "Format" tells half of the story. These tests format a date and then parse it to verify we get the same value back.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #534
This PR has become more than simply fixing the DateUtilsTest class into a review of the whole test suite to ensure we always deal with timezones and locales in a way that's consistent, safe and easier to understand.
What has been done to verify that this works as intended?
java.timevalues used to replace long timestamps actually represent the same instant.Why is this the best possible solution? Were any other approaches considered?
Using
java.timeas much as possible is the safest way to move forward even if the implementation codebase will use Jodatime while we support Android API levels that don't include it.This is the standard way to deal with dates in Java starting from Java8 and it's very well documented with lots of examples.
Also, using a
java.timeserves as a safe and solid baseline that provides an extra sanity check over our code and tests.Using human readable date and time definitions in our tests makes them easier to understand. When something goes wrong, it will be easier to detect problems if we can read the values involved, as opposed to using timestamps.
Although Using the
@Beforeand@Afterhooks to prevent side-effects due to setting default locales and time zones in test methods is super safe, using the methods provided bySystemHelper(new in this PR) make more explicit where this is actually required.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This PR doesn't change the implementation codebase, so no behavior change expected whatsoever.
Do we need any specific form for testing your changes? If so, please attach one.
No
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No