maint: Use Catch2 instead of doctest #3618

mathbunnyru · 2024-11-20T02:34:32Z

Why: mostly because doctest doesn't seem to be well supported: doctest/doctest#554
Last commit to main was March 15, 2023, to dev there were only 8 commits after that.
Catch2 last commit was last week, and the project is incredibly popular.

Please, let me know what you think. If we decide to switch to Catch2, I will switch in all the repo.

My personal opinion: Catch2 is easy to use, fast enough, well supported, and feels quite modern.

How to switch from doctest to Catch2 (easy things):

small changes to cmake for find_package and target_link_libraries
#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN -> #define CATCH_CONFIG_MAIN
#include <doctest/doctest.h> -> #include <catch2/catch_all.hpp>
SUBCASE -> SECTION
CHECK -> REQUIRE, CHECK_FALSE -> REQUIRE_FALSE
CHECK_EQ(a, b); and so on -> REQUIRE(a == b); (can be almost done with regex replacement, a bit messy when there are commas in a or b)

A bit more complicated:

TEST_SUITE - doesn't exist in Catch2, but we can use tags, a second arg to TEST_CASE. I also added brackets to tag, because this is how tags look like in Catch2.

Notes:

doctest is modeled after [Catch](https://github.com/catchorg/Catch2) and some parts of the code have been taken directly (https://github.com/doctest/doctest), that's why the switch is relatively easy
in places where TEST_SUITE was used, I added namespace {}, it allows to keep indentation the same so the diff is simple
There is both REQUIRE and CHECK in Catch2, the difference is: The REQUIRE family of macros tests an expression and aborts the test case if it fails. The CHECK family are equivalent but execution continues in the same test case even if the assertion fails. I use REQUIRE to fail fast.

libmamba/ext/solv-cpp/tests/src/test_scenarios.cpp

mathbunnyru · 2024-11-20T12:01:52Z

One more think to make it work - we will need to slightly change StringMaker - it should be easy: https://github.com/catchorg/Catch2/blob/devel/docs/tostring.md#catchstringmaker-specialisation

(in our case we will only need to change the namespace, make it return std::string instead of String and slightly fix return statements (they will become easier, and overall it will be more efficient)).

Before doing all that (I will need to change it for the whole repo), I would like to hear what mamba team thinks about changing test framework.

libmamba/ext/solv-cpp/tests/CMakeLists.txt

mathbunnyru · 2024-11-21T00:34:27Z

libmamba/ext/solv-cpp/tests/src/msvc_string_maker.cpp

@@ -0,0 +1,22 @@
+#ifdef _WIN32
+
+// Catch compiled on `conda-forge` for MSVC doesn't support outputting `string_view`.


This was a funny part

Catch compiled on conda-forge for MSVC doesn't support outputting string_view.

I think conda-forge compiles this lib for c++11 (or 14) under MSVC.
That's why automatic detection of the string_view feature in catch doesn't compile this part: https://github.com/catchorg/Catch2/blob/devel/src/catch2/catch_tostring.cpp#L131

On the other hand, when we include the header file, the automatic detection works fine, so the header part is included.

Unfortunately, an approach "1 binary works for everyone" doesn't work for C/C++ projects and by using these libraries published to conda-forge sometimes we will have problems like this.

JohanMabille · 2024-11-21T08:52:37Z

Thanks for tackling this! I agree that we might migrate from an unmaintai framework to a maintained one, and Catch2 seems to be the best candidate to me (I had really bad experiences with gtest in the past). The overall approach in this PR looks good to me.

henryiii · 2024-11-21T14:28:53Z

I've had a bad enough experience with gtest that I've done a gtest -> catch2 transition before in CLI11. It was worth it. :) I think doctest was really supposed to be a faster clone of catch2, and then catch2 got a lot faster with 3.0 (catch2 3.0 seems like odd versioning...), so doctest kind of doesn't have a point anymore.

mathbunnyru · 2024-11-21T14:58:55Z

Perfect, I'm glad to see support to my proposal.

Implementation update: I first thought I would have to replace TEST_SUITE using Catch2 tags.

It seems completely unnecessary - there is a --filenames-as-tags which allows to use test filenames as tags, and in most cases TEST_SUITE was simply having something similar to a file name.
So I can simply replace TEST_SUITE(name) { with namespace { for now - this will create an anonymous namespace (no harm in that in case of tests), but the diff will be minimal.

And moreover, only some tests were using the TEST_SUITE feature, but now it will be universal (and no need to think about the test suite name).

This is how it looks:

SOME_LONG_PATH/test_solv_cpp --list-tags --filenames-as-tags

All available tags:
   2  [#test_pool]
  15  [#test_queue]
   1  [#test_repo]
   2  [#test_scenarios]
   1  [#test_solvable]
   1  [#test_solver]
   1  [#test_transaction]
7 tags

On the left is the number of TEST_CASEs, which seems to be correct

mathbunnyru · 2024-11-21T20:45:08Z

libmamba/ext/solv-cpp/tests/CMakeLists.txt

+find_package(Catch2 REQUIRED)
+target_link_libraries(test_solv_cpp PRIVATE Catch2::Catch2WithMain solv::cpp)
+set_target_properties(
+    test_solv_cpp PROPERTIES COMPILE_DEFINITIONS CATCH_CONFIG_ENABLE_ALL_STRINGMAKERS


This allows us to enable nice output for many std types without having to write implementations ourselves (this is from Catch's source code):

#if defined(CATCH_CONFIG_ENABLE_ALL_STRINGMAKERS) # define CATCH_CONFIG_ENABLE_PAIR_STRINGMAKER # define CATCH_CONFIG_ENABLE_TUPLE_STRINGMAKER # define CATCH_CONFIG_ENABLE_VARIANT_STRINGMAKER # define CATCH_CONFIG_ENABLE_OPTIONAL_STRINGMAKER #endif

mathbunnyru · 2024-11-21T21:01:02Z

libmamba/tests/src/doctest-printer/flat_set.hpp

-namespace doctest
-{
-    template <typename K, typename C, typename A>
-    struct StringMaker<mamba::util::flat_set<K, C, A>>


We should delete this one, because Catch2 defines StringMaker for is_range types (also defined in Catch2)

mathbunnyru · 2024-11-21T21:03:52Z

libmamba/tests/src/core/test_history.cpp

    {
-        TEST_CASE("parse")
+        TEST_CASE("History parse")


Catch2 doesn't like TEST_CASE with the same name.
So I had to rename some tests in a couple of places.

I guess the same thing might have been true for doctest, but there were TEST_SUITEs, that's why they were considered not the same

I guess the same thing might have been true for doctest, but there were TEST_SUITEs, that's why they were considered not the same

Actually I am wondering here if we shouldn't give a name to the namespace replacing TEST_SUITE instead of an anonymous one.
That would make it more consistent with what we had before and will give a hint on what we are testing just like the purpose of TEST_SUITE.
I was also wondering how we would filter out tests with catch2 to run them individually?
Something equivalent to:
./test -ts=testsuite_name -tc=testcase_name -sc=subcase_name

@mathbunnyru Pinging here in case the comment was missed as we are planning to merge this soon. Thanks!

I guess the same thing might have been true for doctest, but there were TEST_SUITEs, that's why they were considered not the same

Actually I am wondering here if we shouldn't give a name to the namespace replacing TEST_SUITE instead of an anonymous one. That would make it more consistent with what we had before and will give a hint on what we are testing just like the purpose of TEST_SUITE. I was also wondering how we would filter out tests with catch2 to run them individually? Something equivalent to: ./test -ts=testsuite_name -tc=testcase_name -sc=subcase_name

I don't think having a proper namespace name will help in any way.
It is currently not consistent at all, so I would like just to remove it completely, in a separate commit, rather than have it in a few places inconsistently.

The reason it doesn't really help is because there are many ways to run Catch2 tests.
I will show it with an example of TEST_CASE("HTTPMirror") located in test_mirror.cpp file.

$TEST_BIN HTTPMirror will run this test case with all 3 SECTIONs

$TEST_BIN --section https HTTPMirror will only run https SECTION

$TEST_BIN --filenames-as-tags [#test_mirror] will run all the files in the test_mirror.cpp file

You can combine these options, for example $TEST_BIN --filenames-as-tags --section https [#test_mirror] HTTPMirror.

Note: $TEST_BIN --filenames-as-tags --section https [#test_mirror] will run sections named https, but also REQUIREs which are outside of SECTIONs (all that only in test_mirror.cpp file).

From what I saw most of the time TEST_SUITE was surrounding the whole file.
But we don't need to do that - we can just use the test filename.

Hope this helps.

mathbunnyru · 2024-11-21T21:08:15Z

libmamba/tests/src/validation/test_update_framework_v1.cpp

@@ -201,21 +201,21 @@ TEST_SUITE("validation::v1::RootImpl")
        out_file.close();

        // "2.sv0.6.root.json" is not compatible spec version (spec version N)
-        CHECK_THROWS_AS(v1::RootImpl root(p), role_file_error);


Catch2 doesn't allow things like this, which is not a huge problem

mathbunnyru · 2024-11-21T21:28:55Z

libmamba/tests/CMakeLists.txt

@@ -12,6 +12,10 @@ set(
    LIBMAMBA_TEST_SRCS
    include/mambatests.hpp
    src/test_main.cpp
+    # Catch utils
+    src/catch-utils/conda_url.hpp


This file was not included in a build tree, which is a bug. Fixed it.

mathbunnyru · 2024-11-21T21:29:56Z

libmamba/tests/src/catch-utils/conda_url.hpp

@@ -3,19 +3,22 @@
 // Distributed under the terms of the BSD 3-Clause License.
 //
 // The full license is in the file LICENSE, distributed with this software.
+#pragma once


Also was a bug

mathbunnyru · 2024-11-21T21:30:52Z

libmamba/tests/src/catch-utils/msvc_catch_byte.cpp

+
+    std::string StringMaker<std::byte>::convert(std::byte value)
+    {
+        return fmt::format("{}", value);


Unfortunately, the same problem for string_view under MSVC happened for std::byte

mathbunnyru · 2024-11-21T21:36:12Z

libmamba/tests/src/core/test_invoke.cpp

        }

        TEST_CASE("catches_any_exceptions")
        {
            const auto message = "expected failure";
            auto result = safe_invoke([&] { throw message; });
-            CHECK_FALSE(result);
-            CHECK_MESSAGE(


I think there is no CHECK_MESSAGE in Catch, so I rewrote this code a bit

mathbunnyru · 2024-11-21T21:52:08Z

I left CHECKs instead of REQUIREs in 3 places (meaning they will report as failed in the end but won't stop tests execution):

libmamba/tests/src/core/test_virtual_packages.cpp
77:                    CHECK(Version::parse(pkgs[1].version).value() > Version());

libmamba/tests/src/util/test_os_osx.cpp
24:            CHECK(maybe_version.has_value());
29:            CHECK(std ::regex_match(maybe_version.value(), version_regex));

These tests don't work properly on my M2 mac, unfortunately (the same is true with doctest).

mathbunnyru · 2024-11-21T22:17:46Z

libmamba/tests/src/core/test_cpp.cpp
91-        // Note: this was initially a value-parametrized test; unfortunately,
92:        // doctest does not support this feature yet.
93-        TEST_CASE("prompt")
94-        {

I think I will take a look if there is something nice in Catch, but I don't want to change the tests themselves, so I won't do it in this PR (and this is the only mentions of doctest in my branch, except CHANGELOGs)

mathbunnyru · 2024-11-21T22:19:03Z

This is ready for reviewing, if someone wants to 😆

jjerphan

LGTM.

Thank you, @mathbunnyru.

jjerphan · 2024-11-28T12:51:03Z

libmamba/tests/src/catch-utils/msvc_catch_byte.cpp

+// Catch compiled on `conda-forge` for MSVC doesn't support outputting `std::byte`.
+// So we have to define StringMaker for it ourselves.
+// The declaration is present though, so this only causes link errors.


Has this problem been reported upstream? If not can we report it?

I think I found relevant issue upstream: conda-forge/vc-feedstock#45

jjerphan · 2024-11-28T12:52:39Z

libmamba/tests/src/core/test_history.cpp

    {
-        TEST_CASE("parse")
+        TEST_CASE("History parse")


Hind-M · 2024-11-28T14:55:53Z

I left CHECKs instead of REQUIREs in 3 places (meaning they will report as failed in the end but won't stop tests execution):
libmamba/tests/src/core/test_virtual_packages.cpp
77:                    CHECK(Version::parse(pkgs[1].version).value() > Version());

libmamba/tests/src/util/test_os_osx.cpp
24:            CHECK(maybe_version.has_value());
29:            CHECK(std ::regex_match(maybe_version.value(), version_regex));
These tests don't work properly on my M2 mac, unfortunately (the same is true with doctest).

I'm not sure I get the reason of leaving CHECK here?
Does catch2 provide CHECK as well?
Did you just leave it because it's failing in your machine with doctest? What happens exactly if you use REQUIRE instead?

mathbunnyru · 2024-11-28T23:57:44Z

I left CHECKs instead of REQUIREs in 3 places (meaning they will report as failed in the end but won't stop tests execution):
libmamba/tests/src/core/test_virtual_packages.cpp
77:                    CHECK(Version::parse(pkgs[1].version).value() > Version());

libmamba/tests/src/util/test_os_osx.cpp
24:            CHECK(maybe_version.has_value());
29:            CHECK(std ::regex_match(maybe_version.value(), version_regex));
These tests don't work properly on my M2 mac, unfortunately (the same is true with doctest).
I'm not sure I get the reason of leaving CHECK here? Does catch2 provide CHECK as well? Did you just leave it because it's failing in your machine with doctest? What happens exactly if you use REQUIRE instead?

Both Catch2 and doctest have REQUIRE and CHECK: The REQUIRE family of macros tests an expression and aborts the test case if it fails. The CHECK family are equivalent but execution continues in the same test case even if the assertion fails.

In the main it seems that CHECK family was preferred, but not for some reason and I think it was copy-pasted.
For example:

        SUBCASE("foo V0.9.24")
        {
            auto ms = MatchSpec::parse("foo V0.9.24");
            CHECK_FALSE(ms.has_value());
            CHECK_EQ(std::string(ms.error().what()), "Found invalid version predicate in \"V0.9.24\"");
        }

Semantically, it should be REQUIRE_FALSE(ms.has_value()), because we call ms.error() on the next line.

Reasons I used REQUIRE:

It was easier to see where replacement hasn't been done yet
Also, we will have the tests fail fast if something is wrong
Most of the time, REQUIRE is semantically not worse than CHECK

Reasons I used 3 CHECK statements: they do fail on my machine.
There is a related issue, but the proposed solution didn't work for me, unfortunately.
#1758

mathbunnyru · 2024-12-04T17:47:01Z

CI fails the same way in main, and has nothing to do with my PR.

mathbunnyru · 2024-12-04T17:48:52Z

@Hind-M @jjerphan @JohanMabille I think this is ready to be merged.

I will get rid of the anonymous namespace in tests in a following PR (this way the diff of this PR is much more readable).

jjerphan · 2024-12-04T17:52:21Z

Thank you for the hands-up.

I am waiting for Hind's or Johan's approval. 🙂

mathbunnyru · 2024-12-05T23:35:25Z

@jjerphan Hind has approved 🎉

JohanMabille · 2024-12-06T07:57:30Z

Thanks for the involved work!

mathbunnyru commented Nov 20, 2024

View reviewed changes

libmamba/ext/solv-cpp/tests/src/test_scenarios.cpp Show resolved Hide resolved

JohanMabille added the release::ci_docs For PRs related to CI or documentation label Nov 20, 2024

mathbunnyru commented Nov 20, 2024

View reviewed changes

libmamba/ext/solv-cpp/tests/CMakeLists.txt Outdated Show resolved Hide resolved

mathbunnyru force-pushed the catch2_ext branch from 898b999 to c93c16e Compare November 20, 2024 23:47

maint: Use Catch2 instead of doctest

c08aaee

mathbunnyru force-pushed the catch2_ext branch from c93c16e to c08aaee Compare November 20, 2024 23:51

mathbunnyru commented Nov 21, 2024

View reviewed changes

mathbunnyru added 2 commits November 21, 2024 10:28

Use REQUIRE_THROWS_AS

1666415

Do not use tags

41c0003

mathbunnyru added 16 commits November 21, 2024 15:42

Merge branch 'main' into catch2_ext

bb7f592

WIP

a6d9613

More WIP

983f0a4

More WIP

21a772d

More WIP

87ad29d

Fix wrongdoing of regexp

5f69b0d

More fixes

a3e196a

Remove doctest array printer

5f61e06

More fixes

b2c117a

More fixes

26b4d3e

Fixes

1ab211d

Better section name

f1d10ac

Fix regex wrongdoing

ed34065

Delete doctest-printer

8950c37

Replace some REQUIREs with CHECKs

49e456f

Revert

1bf9ce0

mathbunnyru commented Nov 21, 2024

View reviewed changes

Add catch-printer/conda_url.hpp back

f392b7a

mathbunnyru commented Nov 21, 2024

View reviewed changes

Remove doctest completely

bad03cd

mathbunnyru commented Nov 21, 2024

View reviewed changes

mathbunnyru added 2 commits November 21, 2024 21:20

Add implementation for std::byte Catch output

f75b6e4

Better catch-utils

c37538f

mathbunnyru commented Nov 21, 2024

View reviewed changes

jjerphan approved these changes Nov 28, 2024

View reviewed changes

Merge branch 'main' into catch2_ext

e6fb1a7

mathbunnyru closed this Dec 4, 2024

mathbunnyru reopened this Dec 4, 2024

Merge branch 'main' into catch2_ext

a4160d0

Hind-M approved these changes Dec 5, 2024

View reviewed changes

JohanMabille merged commit c3a2c1c into mamba-org:main Dec 6, 2024
34 checks passed

mathbunnyru mentioned this pull request Dec 11, 2024

maint: Remove anonymous namespace in tests #3664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maint: Use Catch2 instead of doctest #3618

maint: Use Catch2 instead of doctest #3618

mathbunnyru commented Nov 20, 2024 •

edited

Loading

mathbunnyru commented Nov 20, 2024 •

edited

Loading

mathbunnyru Nov 21, 2024

JohanMabille Nov 21, 2024

mathbunnyru Nov 21, 2024

JohanMabille commented Nov 21, 2024

henryiii commented Nov 21, 2024

mathbunnyru commented Nov 21, 2024 •

edited

Loading

mathbunnyru Nov 21, 2024

mathbunnyru Nov 21, 2024

mathbunnyru Nov 21, 2024

jjerphan Nov 28, 2024

Hind-M Nov 28, 2024

Hind-M Dec 4, 2024

mathbunnyru Dec 4, 2024 •

edited

Loading

mathbunnyru Nov 21, 2024

mathbunnyru Nov 21, 2024

mathbunnyru Nov 21, 2024

mathbunnyru Nov 21, 2024 •

edited

Loading

mathbunnyru Nov 21, 2024

mathbunnyru commented Nov 21, 2024 •

edited

Loading

mathbunnyru commented Nov 21, 2024

mathbunnyru commented Nov 21, 2024

jjerphan left a comment

jjerphan Nov 28, 2024

mathbunnyru Dec 4, 2024

jjerphan Nov 28, 2024

Hind-M commented Nov 28, 2024

mathbunnyru commented Nov 28, 2024

mathbunnyru commented Dec 4, 2024

mathbunnyru commented Dec 4, 2024 •

edited

Loading

jjerphan commented Dec 4, 2024 •

edited

Loading

mathbunnyru commented Dec 5, 2024

JohanMabille commented Dec 6, 2024

		@@ -0,0 +1,22 @@
		#ifdef _WIN32

		// Catch compiled on `conda-forge` for MSVC doesn't support outputting `string_view`.

maint: Use Catch2 instead of doctest #3618

maint: Use Catch2 instead of doctest #3618

Conversation

mathbunnyru commented Nov 20, 2024 • edited Loading

mathbunnyru commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanMabille commented Nov 21, 2024

henryiii commented Nov 21, 2024

mathbunnyru commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathbunnyru Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathbunnyru Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathbunnyru commented Nov 21, 2024 • edited Loading

mathbunnyru commented Nov 21, 2024

mathbunnyru commented Nov 21, 2024

jjerphan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hind-M commented Nov 28, 2024

mathbunnyru commented Nov 28, 2024

mathbunnyru commented Dec 4, 2024

mathbunnyru commented Dec 4, 2024 • edited Loading

jjerphan commented Dec 4, 2024 • edited Loading

mathbunnyru commented Dec 5, 2024

JohanMabille commented Dec 6, 2024

mathbunnyru commented Nov 20, 2024 •

edited

Loading

mathbunnyru commented Nov 20, 2024 •

edited

Loading

mathbunnyru commented Nov 21, 2024 •

edited

Loading

mathbunnyru Dec 4, 2024 •

edited

Loading

mathbunnyru Nov 21, 2024 •

edited

Loading

mathbunnyru commented Nov 21, 2024 •

edited

Loading

mathbunnyru commented Dec 4, 2024 •

edited

Loading

jjerphan commented Dec 4, 2024 •

edited

Loading