[GitHub] [nifi] adenes commented on pull request #4677: NIFI-8023 Test failure after NIFI-7996 commit if system default TZ is…

[GitBox <gi...@apache.org>]

adenes commented on pull request #4677:
URL: https://github.com/apache/nifi/pull/4677#issuecomment-731271678


   Thanks @exceptionfactory for the review. Actually your recommended change is the revert of the original commit that caused these test to fail (see: https://github.com/apache/nifi/commit/83948db989bf2f682dfa18c56293ec431f546e1c#diff-98d8c798133bf7b6d8a2b49a35139863b6ac85c6001787a62b3c6117abafb440L669)
   
   I think we have a more generic issue, using `java.sql.Date` for representing dates is not the best solution. Being a subclass of `java.util.Data` means that under the hood it stores the time too, just doesn't expose it.
   Storing the time leads to a "date skew" issue: when 2020-11-20 is represented as 2020-11-20 00:00 UTC but if later it's converted to a different timezone (e.g. EST, UTC-5) the same instant is presented as 2020-11-19 19:00 EST, thus the date part has changed.
   This case is covered by a newly added test case: https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/TestConvertRecord.java#L378
   
   My opinion is that in case of dates time zone doesn't make sense (i.e. 2020-11-19 is the same date in every time zone, even if in some it starts earlier), that's why I removed the `toLocalDate()` call in the previously mentioned commit to fix this issue.
   
   Unfortunately this led to these two failing tests, but I think the tests are incorrect.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org