From 8ed088316e3d9987cc41db211f6d20c3c94057c5 Mon Sep 17 00:00:00 2001 From: Raymond Chen Date: Fri, 17 Feb 2023 12:42:22 -0800 Subject: [PATCH] Fix unreliable clock epoch tests The clock epoch tests are sensitive to CPU speed and the precise moment the clock ticks over from one second to another. Change the way we do epoch tests so that they are not affected by either factor. This fixes a spurious CI test failure where the test environment was 3 milliseconds too slow. https://github.com/oldnewthing/cppwinrt/actions/runs/4195918696/jobs/7276654213 --- test/old_tests/UnitTests/clock.cpp | 65 ++++++++++++++++++------------ 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/test/old_tests/UnitTests/clock.cpp b/test/old_tests/UnitTests/clock.cpp index 3940cb1c3..e9e7108f3 100644 --- a/test/old_tests/UnitTests/clock.cpp +++ b/test/old_tests/UnitTests/clock.cpp @@ -17,6 +17,18 @@ namespace winrt } } +// To confirm that two clocks have the same epoch, we +// capture one clock, then the other, then the first again, +// and verify that the three timestamps are ordered. This avoids +// spurious failures if a test machine is slow or experiences a +// clock tick at just the wrong time. +// +// We use a duration_cast to milliseconds so that the assertion +// failure message tells us how many milliseconds of error we encountered. +#define REQUIRE_ORDERED(a, b, c) \ + REQUIRE(duration_cast((b) - (a)).count() >= 0); \ + REQUIRE(duration_cast((c) - (b)).count() >= 0) + namespace Catch { template @@ -46,11 +58,13 @@ namespace Catch TEST_CASE("clock, now") { - Calendar calendar; - calendar.SetToNow(); + // Confirm that clock::now agrees with Calendar::SetToNow. + Calendar calendar1; + calendar1.SetToNow(); auto time = clock::now(); - auto diff = calendar.GetDateTime() - time; - REQUIRE(abs(diff) < milliseconds{ 100 }); + Calendar calendar2; + calendar2.SetToNow(); + REQUIRE_ORDERED(calendar1.GetDateTime(), time, calendar2.GetDateTime()); } TEST_CASE("clock, units") @@ -66,73 +80,72 @@ TEST_CASE("clock, units") TEST_CASE("clock, time_t") { - const DateTime now_dt = clock::now(); + const DateTime now1_dt = clock::now(); const time_t now_tt = time(nullptr); + const DateTime now2_dt = clock::now(); // Round trip from DateTime to time_t and back. // confirm that nothing happens other than truncating the fractional seconds - REQUIRE(clock::from_time_t(clock::to_time_t(now_dt)) == time_point_cast(now_dt)); + REQUIRE(clock::from_time_t(clock::to_time_t(now1_dt)) == time_point_cast(now1_dt)); // Same thing in reverse REQUIRE(clock::to_time_t(clock::from_time_t(now_tt)) == now_tt); // Conversions are verified to be consistent. Now, verify that we're correctly converting epochs - const auto diff = duration_cast(abs(now_dt - clock::from_time_t(now_tt))).count(); - REQUIRE(diff < 1000); + // Note that time_t has only 1s resolution, so we need to add 1 second of slop on either side. + REQUIRE_ORDERED(now1_dt - 1s, clock::from_time_t(now_tt), now2_dt + 1s); } TEST_CASE("clock, FILETIME") { - const DateTime now_dt = clock::now(); + const DateTime now1_dt = clock::now(); FILETIME now_ft; ::GetSystemTimePreciseAsFileTime(&now_ft); + const DateTime now2_dt = clock::now(); // Round trip conversions - REQUIRE(clock::from_file_time(clock::to_file_time(now_dt)) == now_dt); + REQUIRE(clock::from_file_time(clock::to_file_time(now1_dt)) == now1_dt); REQUIRE(clock::to_file_time(clock::from_file_time(now_ft)) == now_ft); // Verify epoch - const auto diff = abs(now_dt - clock::from_file_time(now_ft)); - REQUIRE(diff < milliseconds{ 100 }); + REQUIRE_ORDERED(now1_dt, clock::from_file_time(now_ft), now2_dt); } TEST_CASE("clock, system_clock") { - DateTime const now_dt = clock::now(); - auto const now_sys = system_clock::now(); + DateTime const now1_dt = clock::now(); + auto const now1_sys = system_clock::now(); + DateTime const now2_dt = clock::now(); + auto const now2_sys = system_clock::now(); // Round trip DateTime to std::chrono::system_clock::time_point and back - REQUIRE(clock::from_sys(clock::to_sys(now_dt)) == now_dt); + REQUIRE(clock::from_sys(clock::to_sys(now1_dt)) == now1_dt); // Round trip other direction - REQUIRE(clock::to_sys(clock::from_sys(now_sys)) == now_sys); + REQUIRE(clock::to_sys(clock::from_sys(now1_sys)) == now1_sys); // Round trip with custom resolution { - auto const now_dt_sec = time_point_cast(now_dt); + auto const now_dt_sec = time_point_cast(now1_dt); REQUIRE(clock::from_sys(clock::to_sys(now_dt_sec)) == now_dt_sec); } { - auto const now_dt_mins = time_point_cast(now_dt); + auto const now_dt_mins = time_point_cast(now1_dt); REQUIRE(clock::from_sys(clock::to_sys(now_dt_mins)) == now_dt_mins); } { - auto const now_sys_sec = time_point_cast(now_sys); + auto const now_sys_sec = time_point_cast(now1_sys); REQUIRE(clock::to_sys(clock::from_sys(now_sys_sec)) == now_sys_sec); } { - auto const now_sys_mins = time_point_cast(now_sys); + auto const now_sys_mins = time_point_cast(now1_sys); REQUIRE(clock::to_sys(clock::from_sys(now_sys_mins)) == now_sys_mins); } // Verify that the epoch calculations are correct. { - auto const diff = now_dt - clock::from_sys(now_sys); - REQUIRE(abs(diff) < milliseconds{ 100 }); - } - { - auto const diff = now_sys - clock::to_sys(now_dt); - REQUIRE(abs(diff) < milliseconds{ 100 }); + REQUIRE_ORDERED(now1_dt, clock::from_sys(now1_sys), now2_dt); + REQUIRE_ORDERED(clock::from_sys(now1_sys), now2_dt, clock::from_sys(now2_sys)); } }