eXparity / hamcrest-date

A Java library which provides a suite hamcrest matchers for matching dates, times, and moments in time
BSD 3-Clause "New" or "Revised" License
70 stars 11 forks source link

DateMatchers.isDay(int, Month, int) not adapted for non-zero time zones #20

Closed tnaskali closed 5 years ago

tnaskali commented 6 years ago

When using DateMatchers.isDay(int, Month, int) with a date at start of day in GMT+1, the results are not as expected. Consider the following test :

  @Test
  public void isDay_WhenStartOfDayInGmtPlus1_ThenOffsetShouldBeIgnored() {
    LocalDate firstDayOf2018 = LocalDate.of(2018, Month.JANUARY, 1);
    ZonedDateTime startOfFirstDayOf2018InGmtPlus1 = ZonedDateTime.of(firstDayOf2018, LocalTime.MIDNIGHT, ZoneId.of("GMT+1"));

    Date dateToCompare = Date.from(startOfFirstDayOf2018InGmtPlus1.toInstant());
    assertThat(dateToCompare,
      isDay(firstDayOf2018.getYear(), firstDayOf2018.getMonth(), firstDayOf2018.getDayOfMonth()));
  }

It fails with the following error :

java.lang.AssertionError: 
Expected: the same day as lun., 01 janv. 2018
     but: the day is lun., 01 janv. 2018
    at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        ...

Although understandable, there are a multiple issues with this result :

Maybe a new isLocalDay(int, Month, int) method would be nice ?

stewbis commented 6 years ago

Date doesn't have a concept of timezone, it stores the milliseconds since the unix epoch based of UTC. The date you created is 23:00 31st Dec 2017 so when checking if it's on the 1st Jan 2018 it doesn't match. The error message is wrong because it's using the locale timezone but the result is correct.

To support cases where you want to check the isDay in a specific timezone using Java Date it would be clearer to add an overload to support passing ZoneId or TimeZone into the isDay call which then is used as the basis for the test. So in effect the test would be like:

assertThat(date, isDay(2018, JANUARY, 1, ZoneId.of("GMT+1"));

This makes the timezone in explicit and supports a wider range of use cases if you want to test if it's a specific date in timezones other that your Local one.

tnaskali commented 6 years ago

I've started to implement this feature and here's my first thoughts :

stewbis commented 5 years ago

This issue can now be close;d the atZone fluent method allows for replacement of the default timezone

Alecsvan commented 5 years ago

I beleive the following example is related to the same issue: final ZonedDateTime date = ZonedDateTime.of(2000,1,1,0,0,0,0,ZoneId.of("UTC")); assertThat(date, ZonedDateTimeMatchers.sameDay(date));

java.lang.AssertionError: Expected: the same day as Fri, 31 Dec 1999 but: the day is Sat, 01 Jan 2000

One of the days in this assertion somehow moved from UTC to my local timezone.

stewbis commented 5 years ago

Hi, the test would need to be changed to anchor it in UTC e.g.

final ZonedDateTime date = ZonedDateTime.of(2000,1,1,0,0,0,0,ZoneId.of("UTC")); assertThat(date, ZonedDateTimeMatchers.sameDay(date).atZone(ZoneIds.UTC);

Alecsvan commented 5 years ago

I should say that is quite an unexpected behavior to add zone to an already zoned date. It is even more painful if I try to use sameInstant method and getting the same result.

stewbis commented 5 years ago

Hi Alecsvan, that's a good point and I missed the fact that in your example test you used the same date. I agree the date should always match itself, the matchers make no sense otherwise. I'm raising a new issue for this because it should work

stewbis commented 5 years ago

New issue https://github.com/eXparity/hamcrest-date/issues/32 raises for sameDay not matching using zoned date time. @Alecsvan would you be OK to supply your timezone so that I can make it explicit in the issue. Feel free to update the issue 32 with the information. Thanks, @stewbis

Alecsvan commented 5 years ago

Sure, added the timezone. Thanks