cmake: Clean up testing code #1554

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:240626-ctest-cleanup changing 1 files +11 −9
  1. hebasto commented at 10:42 pm on June 26, 2024: member
    1. Delete CTest module.

    The CTest module handles CDash integration, which we do not use. It is not required for testing functionality.

    1. Clean up cases when to invoke enable_testing()

    The enable_testing() command invocation is required for add_test() commands, which are used only for {noverify_}tests, exhaustive_tests and examples.

  2. cmake: Delete `CTest` module
    The `CTest` module handles `CDash` integration, which we do not use. It
    is not required for testing functionality.
    6aa576515e
  3. real-or-random added the label assurance on Jun 27, 2024
  4. real-or-random added the label build on Jun 27, 2024
  5. real-or-random commented at 9:12 am on June 27, 2024: contributor

    The enable_testing() command invocation is required for add_test() commands, which are used only for {noverify_}tests, exhaustive_tests and examples.

    I don’t think it’s required. It’s just that add_test() has no effect without enable_testing().

    enable_testing() seems cheap. Couldn’t we just run it always? Or do you think this has drawbacks?

  6. cmake: Call `enable_testing()` unconditionally
    This change simplifies the code.
    Also comments has been added to highlight the code structure.
    7c987ec89e
  7. hebasto force-pushed on Jun 27, 2024
  8. hebasto commented at 10:35 am on June 27, 2024: member

    The enable_testing() command invocation is required for add_test() commands, which are used only for {noverify_}tests, exhaustive_tests and examples.

    I don’t think it’s required. It’s just that add_test() has no effect without enable_testing().

    enable_testing() seems cheap.

    I agree.

    Couldn’t we just run it always?

    Sure. The code has been adjusted.

    Or do you think this has drawbacks?

    I did some research and I haven’t found any drawbacks. When enable_testing() is invoked and no tests being added, the created CTestTestfile.cmake files in the binary tree are noop/empty.

    It is recommended:

    to call enable_testing() somewhere in the top level CMakeLists.txt file. This would typically be done early, soon after the first project() call.

  9. real-or-random approved
  10. real-or-random commented at 10:47 am on June 27, 2024: contributor
    utACK 7c987ec89e6cbee5d087683ba29b97012e679484
  11. theStack approved
  12. theStack commented at 10:40 am on September 18, 2024: contributor

    ACK 7c987ec89e6cbee5d087683ba29b97012e679484

    I don’t have too much knowledge about CMake, but following the discussion here and reading some CMake forum discussions (e.g. https://discourse.cmake.org/t/is-there-any-reason-to-prefer-include-ctest-or-enable-testing-over-the-other/1905/2), the simplification looks correct.

    I did some research and I haven’t found any drawbacks. When enable_testing() is invoked and no tests being added, the created CTestTestfile.cmake files in the binary tree are noop/empty.

    Verified this by creating a fresh build directory via

    0$ rm -rf ./build && cmake -B build -DSECP256K1_BUILD_BENCHMARK=OFF -DSECP256K1_BUILD_TESTS=OFF -DSECP256K1_BUILD_EXHAUSTIVE_TESTS=OFF -DSECP256K1_BUILD_CTIME_TESTS=OFF
    1$ cat ./build/src/CTestTestfile.cmake
    

    and checking that the file only contains comment. If one of these options is ON, the file contains add_test(...) calls.

  13. real-or-random merged this on Sep 18, 2024
  14. real-or-random closed this on Sep 18, 2024

  15. hebasto deleted the branch on Sep 18, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-06 05:15 UTC

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