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

isEqualTo on date can lead to unexpected failures when switching time zones #3382

Closed
wimdeblauwe opened this issue Mar 1, 2024 · 6 comments
Closed

isEqualTo on date can lead to unexpected failures when switching time zones #3382

wimdeblauwe opened this issue Mar 1, 2024 · 6 comments
Assignees
@joel-costigliola
Milestone
3.26.0

Comments

@wimdeblauwe
Copy link
Contributor

Describe the bug
The behaviour of isEqualTo(String) when asserting a java.util.Date can fail if you switch the default time zone during a test run.

  • assertj core version: 3.24.2
  • java version: 21
  • test framework version: JUnit 5
  • os (if relevant):

Test case reproducing the bug

Add a test case showing the bug that we can run

public class Test {
 @Test
    void testWithIsEqualTo() {
//      TimeZone.setDefault(TimeZone.getTimeZone("CET"));
//      assertThat(Date.from(Instant.parse("2024-03-01T00:00:00.000+01:00")))
//          .describedAs("Using CET time zone")
//          .isEqualTo("2024-03-01");
      TimeZone.setDefault(TimeZone.getTimeZone("WET"));
      assertThat(Date.from(Instant.parse("2024-03-01T00:00:00.000+00:00")))
          .describedAs("Using WET time zone")
          .isEqualTo("2024-03-01");
    }

    @Test
    void test() {
//      TimeZone.setDefault(TimeZone.getTimeZone("CET"));
//      assertThat(Date.from(Instant.parse("2024-03-01T00:00:00.000+01:00")))
//          .describedAs("Using CET time zone")
//          .hasYear(2024)
//          .hasMonth(3)
//          .hasDayOfMonth(1);
      TimeZone.setDefault(TimeZone.getTimeZone("WET"));
      assertThat(Date.from(Instant.parse("2024-03-01T00:00:00.000+00:00")))
          .describedAs("Using WET time zone")
          .hasYear(2024)
          .hasMonth(3)
          .hasDayOfMonth(1);
    }
}

Run this test with the code commented out. The test will be ok.

Now enable the code in comments and run the test again. The test testWithIsEqualTo now fails, but on the part that was already active before.

(I found the issue with using @DefaultTimeZone, but I can reproduce it without it, so I believe it might be an AssertJ issue).
@joel-costigliola joel-costigliola added this to the 3.26.0 milestone Mar 1, 2024
@joel-costigliola
Copy link
Member

joel-costigliola commented Mar 1, 2024 •
edited
Loading

Thanks for reporting this @wimdeblauwe, we will have a look at it shortly.

@joel-costigliola
Copy link
Member

joel-costigliola commented May 19, 2024 •
edited
Loading

AssertJ uses various date format to get parse the date from the given string, in the test case, it is the yyyy-MM-dd date pattern (as expected).

AssertJ delegates the parsing to SimpleDateFormat parse method, there is not much AssertJ can do at this point, it"s in the hand of the java runtime.

My understanding is that the parsing has to use the default timezone because 2024-03-01 is not a Date but a LocalDate, to make it a Date it has to be interpreted within a timezone which can be no other than the default timezone.

Passing string instead of date was added to avoid building dates by hand which was pain before java 8, it"s now less useful with the new date/time API which provides a bunch of factory methods.

@wimdeblauwe what behavior would you have liked to see here?

This was referenced May 20, 2024
Merged
Open
@scordio
Copy link
Member

scordio commented May 20, 2024

@wimdeblauwe FYI, we"ll evaluate the internal usage of DateTimeFormatter in #3482.

Any input you may provide us on expected behavior can help us ensure we don"t go off-track 🙂

@wimdeblauwe
Copy link
Contributor Author

My understanding is that the parsing has to use the default timezone because 2024-03-01 is not a Date but a LocalDate, to make it a Date it has to be interpreted within a timezone which can be no other than the default timezone.

I agree, but shouldn"t it keep working when you switch the default timezone ? It feels like some kind of caching is done and the updated default timezone is not taken into account.

@joel-costigliola
Copy link
Member

joel-costigliola commented May 22, 2024 •
edited
Loading

Indeed this is bug in AssertJ, we store default date formats as SimpleDateFormat, they are created only once at the first date assertion, they are stateful and store the timezone in their underlying calendar.

The problem arises when you change the timezone, the default date formats have already been created with the previous timezone and thus interprets incorrectly the dates as string, (evil stateful SimpleDateFormat!).

well spotted @wimdeblauwe !

@joel-costigliola joel-costigliola self-assigned this May 22, 2024
@wimdeblauwe
Copy link
Contributor Author

Thanks @joel-costigliola . That is great with open-source, you can get a peek "inside" to guess what might be issue. Actually solving it is usually a step harder :-)

genuss pushed a commit to genuss/assertj that referenced this issue May 28, 2024
@joel-costigliola @genuss
Rebuild default date formats used to parse string as dates when the d…
cee4f11 
…efault timezone or the lenient flag changes.

Fix assertj#3382
@scordio scordio removed the type: bug label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@joel-costigliola joel-costigliola

Labels
None yet
Projects
None yet
Milestone
3.26.0
Development

No branches or pull requests
3 participants
@joel-costigliola @wimdeblauwe @scordio