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.

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

Fix ISO8601UtilsTest failing on systems with UTC+X #1687

Merged
merged 1 commit into from
May 14, 2021

Conversation

Marcono1234
Copy link
Collaborator

Previously ISO8601UtilsTest.testDateFormatString() would fail on systems where the time zone is UTC+X because getTime() returned "2018-06-24" for them.

Additionally the tests which previously changed the system locale and time zone have been rewritten to create a UTC calendar instead. Setting locale seems to not be necessary because ISO8601Utils.parse(...) does not do that either.

Previously ISO8601UtilsTest.testDateFormatString() would fail on systems
where the time zone is UTC+X because getTime() returned "2018-06-24" for them.

Additionally the tests which previously changed the system locale and time
zone have been rewritten to create a UTC calendar instead. Setting locale
seems to not be necessary because ISO8601Utils.parse(...) does not do that
either.
@LorenzNickel
Copy link
Contributor

Hey, I had the same problem while running the test and also created a PR for it (#1665), where I use another approach to the same problem

@Marcono1234
Copy link
Collaborator Author

Thanks for pointing this out, I did not search before creating the pull request, which I should probably have done.

I am biased since I created this pull request, but I prefer this approach because

  • It also sets Locale.US, so it behaves the same as ISO8601Utils.format, not sure if otherwise the test could fail for certain Locales
  • It removes changing the system time zone and locale since that is error-prone if a test forgets to reset them and also prevent concurrent use

@eamonnmcmanus
Copy link
Member

Thanks! I agree that the approach here seems reasonable, even if it is a bit more complicated than the simplest possible fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants