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

ZonedDateTime test vs self failing in non-UTC timezone #32

Closed stewbis closed 4 years ago

stewbis commented 4 years ago

When the below code is tested in a non-UTC timezone it fails whereas testing against self should always return true for matchers such as sameDay, sameYear, etc.

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 4 years ago

Found this issue at ZoneId.of("America/New_York"). The actual code is final ZonedDateTime date = ZonedDateTime.of(2000,1,1,0,0,0,0,ZoneId.of("UTC")); assertThat(date, ZonedDateTimeMatchers.sameDay(date));

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

Alecsvan commented 4 years ago

The error was found under hamcrest-date-2.0.5 taken from maven, but I cannot reproduce it under 2.0.7-snapshot from this repository.

stewbis commented 4 years ago

The code has reworked between 2.0.5 and 2.0.6 to make the logic more consistent and reusable. I'll add some further tests to confirm that treating self vs self gives the right answer

On Tue, 24 Sep 2019, 08:38 Alecsvan, notifications@github.com wrote:

The error was found under hamcrest-date-2.0.5 taken from maven, but I cannot reproduce it under 2.0.7-snapshot from this repository.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/eXparity/hamcrest-date/issues/32?email_source=notifications&email_token=AAPH3VPMILM7WCL7ONJLY3LQLG7XPA5CNFSM4IZ3I4JKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7NMYZQ#issuecomment-534432870, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPH3VNLCM6SYSOXGCLMWTDQLG7XPANCNFSM4IZ3I4JA .

Alecsvan commented 4 years ago

When 2.0.6 works correctly comparing the same zoned date to itself, there is still a confusion in fail reporting as timezone shifts. E.g. for UTC-5 system timezone:

ZonedDateTime date1 = ZonedDateTime.of(2000,1,1,0,0,0,0,ZoneId.of("UTC")); ZonedDateTime date2 = ZonedDateTime.of(2001,1,1,0,0,0,0,ZoneId.of("UTC")); assertThat(date1, sameInstant(date2));

Assert correctly fails but reporting displayes totally incorrect dates:

java.lang.AssertionError: Expected: the same date as Sun, 31 Dec 2000 07:00:00.000 PM -0500 but: the date is Fri, 31 Dec 1999 07:00:00.000 PM -0500

stewbis commented 4 years ago

Hi @Alecsvan, thanks for continuing to look into it and yes the nature of the bug has changed; the comparison is fine but message is using the current timezone.

I'm looking at the application of the atZone change which added atZone to all matchers and was related to issue https://github.com/eXparity/hamcrest-date/issues/20. It may be redundant now for some of the matcher cases with the changes that have made between 2.0.5 -> 2.0.6. These changes removed any code which added missing information such as times or timezones to the actual dates (These fudges were added to try and provide comparable temporal objects).

The original issue https://github.com/eXparity/hamcrest-date/issues/20 which added atZone was failing with the test below because the date was shifting:

@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()));
  }

I'll confirm the result of this test is still working and then begin reviewing the error messages

stewbis commented 4 years ago

This has now been fixed in master and will be release shortly. There are still issues with java Date but they're wider issues of moving around timezones so i'll raise a separate issue