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

Improve timezone tests #3240

Merged
merged 3 commits into from
Dec 25, 2022
Merged

Improve timezone tests #3240

merged 3 commits into from
Dec 25, 2022

Conversation

phprus
Copy link
Contributor

@phprus phprus commented Dec 22, 2022

Issues present after commit 115001a

  1. macOS has broken std::time_put:
[ RUN      ] chrono_test.system_clock_time_point
/Users/phprus/Devel/fmt/test/chrono-test.cc:287: Failure
Expected equality of these values:
  sys_output
    Which is: "+0500"
  fmt::format(fmt::runtime(fmt_spec), t1)
    Which is: "+0000"
/Users/phprus/Devel/fmt/test/chrono-test.cc:288: Failure
Expected equality of these values:
  sys_output
    Which is: "+0500"
  fmt::format(fmt::runtime(fmt_spec), tm)
    Which is: "+0000"
/Users/phprus/Devel/fmt/test/chrono-test.cc:299: Failure
Expected equality of these values:
  sys_output
    Which is: "+05:00"
  fmt::format("{:%Ez}", t1)
    Which is: "+00:00"
/Users/phprus/Devel/fmt/test/chrono-test.cc:300: Failure
Expected equality of these values:
  sys_output
    Which is: "+05:00"
  fmt::format("{:%Ez}", tm)
    Which is: "+00:00"
/Users/phprus/Devel/fmt/test/chrono-test.cc:302: Failure
Expected equality of these values:
  sys_output
    Which is: "+05:00"
  fmt::format("{:%Oz}", t1)
    Which is: "+00:00"
/Users/phprus/Devel/fmt/test/chrono-test.cc:303: Failure
Expected equality of these values:
  sys_output
    Which is: "+05:00"
  fmt::format("{:%Oz}", tm)
    Which is: "+00:00"
[  FAILED  ] chrono_test.system_clock_time_point (0 ms)
  1. Windows has not normal support UTC timezone for std::tm.
    We need to use std::chrono to format time with timezone.

@phprus phprus marked this pull request as ready for review December 22, 2022 18:01
@phprus
Copy link
Contributor Author

phprus commented Dec 23, 2022

Demo:
See errors in CI for branch https://github.com/phprus/fmt/tree/chrono-tz-test-demo

@phprus
Copy link
Contributor Author

phprus commented Dec 24, 2022

Rebased

@vitaut vitaut added this to the zone milestone Dec 24, 2022
Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I noticed the timezone issues too but didn't have time to look into them myself.

What is the purpose of setting the timezone in CI and can we avoid it to keep configs simpler?

Comment on lines 290 to 301
#if defined(__MINGW32__) && !defined(_UCRT)
spec_list = {"%Z"};
#else
spec_list = {"%z", "%Z"};
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving this code to a separate function and reuse here and elsewhere in chrono-test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment clarifying why these are handled separately would be nice too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For loops are not equivalent.

First for loop tests std::chrono::time_point (std::chrono::system_clock::now()) and std::tm (in UTC).
Second for loop tests timezone formatters only for std::tm (in localtime).

I think we should not do this, since timezone support is a weak point of fmt and the standard library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut
Comments added.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't mean merging the for loops, just moving the selected block, namely

#if defined(__MINGW32__) && !defined(_UCRT)
  spec_list = {"%Z"};
#else
  spec_list = {"%z", "%Z"};
#endif

into a separate function (say, get_timezone_specs) to keep the system-specific conditional compilation in one place.

Other than that, all looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut
I don't understand how this will simplify the code.
The condition defined(__MINGW32__) && !defined(_UCRT) is used 4 times in this test. Partially with precondition ifndef _WIN32.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not perfect but would be slightly cleaner because we could reuse this function in two of these places. Another option is to move the defined(__MINGW32__) && !defined(_UCRT) condition into one place, define our own macro (e.g. FMT_HAS_C99_STRFTIME) and reuse that in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I added macro FMT_HAS_C99_STRFTIME.

@phprus
Copy link
Contributor Author

phprus commented Dec 24, 2022

@vitaut
By default GitHub Actions uses UTC and possible time zone related issues are not tested in CI.
I think this change makes sense for testing future versions of fmt.

@vitaut
Copy link
Contributor

vitaut commented Dec 24, 2022

By default GitHub Actions uses UTC and possible time zone related issues are not tested in CI.
I think this change makes sense for testing future versions of fmt.

Sounds reasonable but is it possible to move changing the timezone to code instead of CI?

@phprus
Copy link
Contributor Author

phprus commented Dec 24, 2022

@vitaut

Sounds reasonable but is it possible to move changing the timezone to code instead of CI?

Unfortunately, I am not aware of a cross-platform and universal API for changing the timezone settings. So I changed it at the CI settings level. I think it's more reliable.

@phprus
Copy link
Contributor Author

phprus commented Dec 24, 2022

I understand that this is not are best solution, but I have no other universal solution.
Chrono supports timezones only in C++20 and only in MSVC.

Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
Signed-off-by: Vladislav Shchapov <vladislav@shchapov.ru>
@vitaut vitaut merged commit 4841784 into fmtlib:master Dec 25, 2022
@vitaut
Copy link
Contributor

vitaut commented Dec 25, 2022

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

2 participants