fix: testUnits failure on MacOS

[rrsettgast]

MacOS truncates std::duration to microseconds. This PR changes the SystemClock to the std::chrono::high_resolution_clock to avoid truncation issues.
resolves #3263

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.91%. Comparing base (0cbc589) to head (9c35de9).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3269      +/-   ##
===========================================
- Coverage    55.91%   55.91%   -0.01%     
===========================================
  Files         1043     1043              
  Lines        88934    88934              
===========================================
- Hits         49729    49727       -2     
- Misses       39205    39207       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[timokoch]

mmh. Not 100% sure whether this test is robust. This here uses the default formatter:

GEOS/src/coreComponents/common/Units.cpp

Line 67 in b2e51fd

return GEOS_FMT( "{} s", m_totalSeconds );

I think this is implementation-defined (or depends on locale). I also don't think this has necessarily something to do with macOS. Could also be slight differences between fmt/std::format for example. Of course disabling the test for mac will pass on mac ;). Maybe a better and portable solution is to specify the format string in

GEOS/src/coreComponents/common/Units.cpp

Line 67 in b2e51fd

return GEOS_FMT( "{} s", m_totalSeconds );

? Something like {:.5e}?

[rrsettgast]

mmh. Not 100% sure whether this test is robust. This here uses the default formatter:

GEOS/src/coreComponents/common/Units.cpp

Line 67 in b2e51fd

return GEOS_FMT( "{} s", m_totalSeconds );

I think this is implementation-defined (or depends on locale). I also don't think this has necessarily something to do with macOS. Could also be slight differences between fmt/std::format for example. Of course disabling the test for mac will pass on mac ;). Maybe a better and portable solution is to specify the format string in

GEOS/src/coreComponents/common/Units.cpp

Line 67 in b2e51fd

return GEOS_FMT( "{} s", m_totalSeconds );

? Something like {:.5e}?

Hello @timokoch ,
This problem appears to be due to the use of std::chrono::system_clock which truncates to microsecond on MacOS.
here is the header from my MacOS system:

// -*- C++ -*-
//===----------------------------------------------------------------------===https://
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===https://

#ifndef _LIBCPP___CHRONO_SYSTEM_CLOCK_H
#define _LIBCPP___CHRONO_SYSTEM_CLOCK_H

#include <__chrono/duration.h>
#include <__chrono/time_point.h>
#include <__config>
#include <ctime>

#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
#  pragma GCC system_header
#endif

_LIBCPP_BEGIN_NAMESPACE_STD

namespace chrono
{

class _LIBCPP_EXPORTED_FROM_ABI system_clock
{
public:
    typedef microseconds                     duration;
    typedef duration::rep                    rep;
    typedef duration::period                 period;
    typedef chrono::time_point<system_clock> time_point;
    static _LIBCPP_CONSTEXPR_SINCE_CXX14 const bool is_steady = false;

    static time_point now() _NOEXCEPT;
    static time_t     to_time_t  (const time_point& __t) _NOEXCEPT;
    static time_point from_time_t(time_t __t) _NOEXCEPT;
};

#if _LIBCPP_STD_VER >= 20

template <class _Duration>
using sys_time    = time_point<system_clock, _Duration>;
using sys_seconds = sys_time<seconds>;
using sys_days    = sys_time<days>;

#endif

} // namespace chrono

_LIBCPP_END_NAMESPACE_STD

#endif // _LIBCPP___CHRONO_SYSTEM_CLOCK_H

[timokoch]

Ok seems like high_resolution_clock makes sense here and makes the test pass. Just out of interest: what it this test actually testing? Seems it (a) checks if the time formatter gives the expected format and (b) checks that fromSeconds and fromDuration gives the same. But why is the duration for the fromDuration test cast to the system clock duration type? Why not just use the duration type that chrono determines?

[rrsettgast]

Ok seems like high_resolution_clock makes sense here and makes the test pass. Just out of interest: what it this test actually testing? Seems it (a) checks if the time formatter gives the expected format and (b) checks that fromSeconds and fromDuration gives the same. But why is the duration for the fromDuration test cast to the system clock duration type? Why not just use the duration type that chrono determines?

I am actually not sure of the reason to do the conversion. It wouldn't be my choice, but I don't think I reviewed this when it went in to develop. I will as the originator of the reasons for this.