Fix failing util_time_GetTime test on Windows #32318

pull VolodymyrBg wants to merge 1 commits into bitcoin:master from VolodymyrBg:bgg changing 1 files +2 −9
  1. VolodymyrBg commented at 3:51 pm on April 21, 2025: contributor

    Remove unreliable steady clock time checking from the test that was causing CI failures primarily on Windows. The test previously tried to verify that steady_clock time increases after a 1ms sleep, but this approach is not reliable on all platforms where such a short sleep interval may not consistently result in observable clock changes.

    This addresses issue #32197 where the test was reporting failures in the cross-built Windows CI environment. As noted in the discussion, the test is not critical to the functionality of Bitcoin Core, and removing the unreliable part is the most straightforward solution.

  2. DrahtBot commented at 3:51 pm on April 21, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32318.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK laanwj, maflcko, achow101
    Stale ACK shahsb

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. shahsb commented at 2:43 am on April 22, 2025: none
    Concept ACK
  4. shahsb commented at 2:44 am on April 22, 2025: none
    Approach ACK
  5. laanwj added the label Tests on Apr 22, 2025
  6. laanwj commented at 12:36 pm on April 24, 2025: member
    Looks like a follow-up to #32212 (first thought it was a duplicate).
  7. VolodymyrBg commented at 12:37 pm on April 24, 2025: contributor

    Looks like a follow-up to #32212 (first thought it was a duplicate). But issue is opened

  8. VolodymyrBg commented at 12:40 pm on April 24, 2025: contributor

    Looks like a follow-up to #32212 (first thought it was a duplicate).

    #32212 was merged 3 weeks ago and the author is maflcko, but he left a comment that this test can be deleted 2 weeks ago

  9. laanwj approved
  10. laanwj commented at 12:42 pm on April 24, 2025: member
    Code review ACK 51e4c5b42260855e5b5306d677ad34560976b9fd, timing-based tests are brittle, and i don’t think this is a very important thing to test as part of our test suite.
  11. in src/test/util_tests.cpp:588 in 51e4c5b422 outdated
    589-    const auto steady_0 = std::chrono::steady_clock::now();
    590-    UninterruptibleSleep(1ms);
    591-    BOOST_CHECK(steady_ms_0 < Now<SteadyMilliseconds>());
    592-    BOOST_CHECK(steady_0 + 1ms <= std::chrono::steady_clock::now());
    593+    // The steady clock time check has been removed because it can be unreliable on some platforms
    594+    // Particularly on Windows, the 1ms sleep may not be sufficient to reliably observe clock changes
    


    maflcko commented at 1:00 pm on April 24, 2025:
    not sure we need to keep a comment about tests that were removed. What is the goal here? If the goal is to prevent re-introducing the test, it seems weak, because the comment can be missed easily. Also, the remainder of the code here in this function is concerned about mocktime, so it would be better to remove the comment and rename the test name to clarify it is about mocktime only.

    VolodymyrBg commented at 1:15 pm on April 24, 2025:
    Done
  12. in src/test/util_tests.cpp:572 in e31269e006 outdated
    566@@ -567,7 +567,7 @@ BOOST_AUTO_TEST_CASE(strprintf_numbers)
    567 #undef B
    568 #undef E
    569 
    570-BOOST_AUTO_TEST_CASE(util_time_GetTime)
    571+BOOST_AUTO_TEST_CASE(util_mocktime)
    572 {
    573     SetMockTime(111);
    


    maflcko commented at 1:25 pm on April 24, 2025:
    0    SetMockTime(111s);
    

    nit: Could clarify while touching

  13. maflcko approved
  14. maflcko commented at 1:25 pm on April 24, 2025: member
    lgtm
  15. laanwj commented at 1:26 pm on April 24, 2025: member
    Would make sense to squash this to one commit imo, no need to introduce the comment just to remove it again.
  16. Fix failing util_time_GetTime test on Windows
    Remove unreliable steady clock time checking from the test that was causing
    CI failures primarily on Windows. The test previously tried to verify that
    steady_clock time increases after a 1ms sleep, but this approach is not reliable
    on all platforms where such a short sleep interval may not consistently result
    in observable clock changes.
    
    This addresses issue #32197 where the test was reporting failures in the
    cross-built Windows CI environment. As noted in the discussion, the test is not
    critical to the functionality of Bitcoin Core, and removing the unreliable part
    is the most straightforward solution.
    
    Rename and refocus util_time_GetTime test to util_mocktime
    
    Co-Authored-By: maflcko <6399679+maflcko@users.noreply.github.com>
    3dbd50a576
  17. VolodymyrBg force-pushed on Apr 24, 2025
  18. VolodymyrBg commented at 1:35 pm on April 24, 2025: contributor

    Would make sense to squash this to one commit imo, no need to introduce the comment just to remove it again.

    squashed

  19. laanwj approved
  20. laanwj commented at 1:43 pm on April 24, 2025: member

    re-ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c

    Changed since last: renamed test to util_mocktime, added seconds to SetMockTime arg, and faulty test is removed instead of replaced by a comment.

  21. DrahtBot requested review from shahsb on Apr 24, 2025
  22. maflcko commented at 1:55 pm on April 24, 2025: member
    lgtm ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c
  23. achow101 commented at 10:03 pm on April 24, 2025: member
    ACK 3dbd50a576be55941cb4b5034dc2171c03afb07c
  24. achow101 merged this on Apr 24, 2025
  25. achow101 closed this on Apr 24, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-05 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me