mamba-org / mamba

The Fast Cross-Platform Package Manager
https://mamba.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6.96k stars 359 forks source link

maint: Use Catch2 instead of doctest #3618

Open mathbunnyru opened 5 days ago

mathbunnyru commented 5 days ago

Why: mostly because doctest doesn't seem to be well supported: https://github.com/doctest/doctest/issues/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):

A bit more complicated:

Notes:

mathbunnyru commented 5 days ago

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.

JohanMabille commented 4 days ago

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 commented 4 days ago

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 commented 4 days ago

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 commented 3 days ago

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 commented 3 days ago
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 commented 3 days ago

This is ready for reviewing, if someone wants to 😆