moved from Catch to doctest for unit tests

[onqtam]

I did this migration since there was interest for moving from Catch to doctest more than 1 year ago:
a8233c7

Here are the changes I had to make:

• introduce a compatibility header (called doctest_compatibility.h) which includes the actual doctest.h header and introduces/changes a few macros in order to have the least amount of source control modifications in the commit.
• had to add some STL includes to test files because doctest doesn't drag any but catch does
• had to artificially get an STL header included before #define private public because MSVC STL headers complain about redefining C++ keywords (otherwise the first STL header would come from json.hpp which is after the defining)
  • also had to move a few headers above that hack which were previously included by Catch anyway
• 3 places where SECTION was used inside of a loop with different filenames were rewritten to be just normal scopes with additional logging since it is practically the same (and doctest doesn't support sections (subcases) in loops with changing names) - it is weird anyway.
• Had to use a local variable a few places where CAPTURE (or INFO) was using a temporary - doctest forbids this - see notes in docs (done for runtime performance reasons):
  https://github.com/onqtam/doctest/blob/master/doc/markdown/logging.md#logging-macros
• the [!throws] test cases are conditionally not compiled if exceptions are disabled (was done that way in 1 other place already)
• [udt] user defined tag migrated to a test suite decorator (can be filtered from the command line using test suite filters)
• had to introduce some variables (for example in unit-allocator.cpp) because code such as bad_json(bad_json::value_t::object) wouldn't compile with MSVC 2017 - even outside of a macro such as CHECK_THROWS_AS and into a simple function (without even including the framework header). Errors and warnings such as these were present:

  error C2761: 'nlohmann::detail::value_t object': redeclaration of member is not allowed
  warning C4456: declaration of 't' hides previous local declaration

some notes:

• Consider using a JSON_PRIVATE macro which is either private or public for the test framework internals - this definition of a C++ keyword is nasty
• Runtime is also improved (not only compile time): the unicode tests should run much faster mainly because the use of CAPTURE inside of hot functions such as check_utf8string doesn't make allocations and construct strings unless an assert fails!

It seems that tests are ran around 10-35% faster on the CI - I was hoping for more but this is good as well. Even if doctest is 100 times faster (which isn't true), if only 30% of the total time is due to the testing framework, then the maximum gains expected are up to 30% for the whole build time. The Total time on travis drops down from around 12 hours 20 minutes to 9 hours 30 minutes.

When building the tests locally on a Windows machine (single threaded build) with GCC 8.1 (default cmake config) it takes for me 4 minutes 50 seconds for doctest and 7 minutes 46 seconds for Catch. Perhaps the use of a precompiled header for the json header would be of some help (10-20 seconds? ...) but would be harder to employ the #define private public hack.

In the case of some of the OSX builds - more than half of the time is installing stuff...

[coveralls]

Coverage remained the same at 100.0% when pulling da5b783 on onqtam:doctest into b21c04c on nlohmann:develop.

[nlohmann]

Wow! Thanks for the PR. I need some time to evaluate this. Thanks for your patience in advance.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[onqtam]

@nlohmann if there is any other feature you might need - I'm open to suggestions :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[nlohmann]

Sorry for letting this get stale twice... I am now ready to have a deeper look at this. I just restarted AppVeyor to see whether the build now succeeds.

[onqtam]

Just merged develop again and the problems went away. Also updated to doctest 2.3.1 (released today)

[nlohmann]

(Ignore my previous post - I checked the wrong branch)

[nlohmann]

I compared the compile times, and I am impressed!

doctest / Debug

ninja 179.30s user 11.26s system 633% cpu 30.100 total
ninja 174.62s user 10.89s system 635% cpu 29.206 total
ninja 178.38s user 11.29s system 621% cpu 30.508 total

doctest / Release

ninja 337.00s user 11.86s system 639% cpu 54.532 total
ninja 326.88s user 10.87s system 649% cpu 52.003 total
ninja 355.54s user 13.22s system 611% cpu 1:00.31 total

catch / Debug

ninja 242.05s user 13.68s system 650% cpu 39.308 total
ninja 246.96s user 13.82s system 665% cpu 39.214 total
ninja 247.50s user 14.06s system 655% cpu 39.892 total

catch / Release

ninja 464.38s user 14.94s system 669% cpu 1:11.55 total
ninja 453.82s user 15.09s system 615% cpu 1:16.23 total
ninja 472.26s user 15.90s system 668% cpu 1:13.06 total

[nlohmann]

I also compared the runtimes, and I am even more impressed!

doctest / Release

Total Test time (real) = 197.95 sec
Total Test time (real) = 193.12 sec
Total Test time (real) = 202.71 sec

catch / Release

Total Test time (real) = 242.26 sec
Total Test time (real) = 228.90 sec
Total Test time (real) = 228.51 sec

[nlohmann]

Are you sure you do not need the if here? This may be an issue for CMake versions prior 3.8.0.

[onqtam]

You are correct - I assumed initially that the 3.8 check was for cxx_range_for but it was indeed if cxx_std_11 is supported. Adding it back again.

[onqtam]

Actually it was without that check up until commit 4fd9b52 and it used to work - guess people didn't try the tests with a CMake version older than 3.8.

For doctest 2.x C++11 is actually mandatory. For new compilers such as gcc 7/8 C++11 is on by default, but for older ones like 4.x/5 (and maybe 6) C++11 has to be explicitly stated. So I'll add -std=c++0x to the CXX_FLAGS property for the target if cmake < 3.8 (or the CXX_STANDARD property)

[onqtam]

I get it - cxx_range_for is not for that particular feature but for "c++0x" support in general - and was one of the first C++11 features to be added to compilers so cxx_range_for was added as a compile feature long before cxx_std_11...... So I'll just revert the code to have that same exact if/else so I don't have to deal with CXX_STANDARD

[nlohmann]

I just tried the pedantic_clang and pedantic_gcc targets. Both yield several warnings, also in the doctest header.

[onqtam]

I had totally missed that makefile with the pedantic warnings. Currently I don't have a Unix machine on hand to test it... I'll try to get my hands on one in a few days. Could you paste a build log? I might be able to fix some (or most) of them just by looking at it. Have you thought about integrating that makefile to the CI or perhaps moving its function to CMake? This is what doctest does for setting the highest warning levels that I thought of.

EDIT: I also just ran a build locally on a windows machine with GCC 8.1 and I got 303 warnings -Wunused-result and they are all about calling json functions who have JSON_NODISCARD inside of CHECK_NOTHROW()/CHECK_THROWS()/CHECK_THROWS_AS()/CHECK_THROWS_WITH(). doctest could silence the warning but I think it should not. If this is the only warning you are getting - we could modify these 4 asserts inside of doctest_compatibility.h

[onqtam]

I actually found why the Catch code didn't give the -Wunused-result warning - this static_cast to void was the reason. Doctest doesn't do that and the result is that you can insert a whole { block(); of = code(); } inside of a CHECK_NOTHROW()/CHECK_THROWS()/CHECK_THROWS_AS() macro (even the block curly braces aren't necessary - multiple statements can be given without trouble).

As a solution for the json warning we could indeed do the following in the compatibility header:

#undef CHECK_THROWS
#undef CHECK_THROWS_AS
#undef CHECK_THROWS_WITH
#undef CHECK_NOTHROW

#define CHECK_THROWS(expr) DOCTEST_CHECK_THROWS(static_cast<void>(expr))
#define CHECK_THROWS_AS(expr, e) DOCTEST_CHECK_THROWS_AS(static_cast<void>(expr), e)
#define CHECK_THROWS_WITH(expr, e) DOCTEST_CHECK_THROWS_WITH(static_cast<void>(expr), e)
#define CHECK_NOTHROW(expr) DOCTEST_CHECK_NOTHROW(static_cast<void>(expr))

// TODO: Same with WARN and REQUIRE levels of asserts which aren't yet used by json

This also has the effect that the auto tmp = which I had inserted previously in calls such as CHECK_THROWS_AS(auto tmp = my_json::json_value(v), std::bad_alloc&); in unit-allocator.cpp are no longer necessary and can be removed.

Let me know how you would want to proceed with this specific issue and if there are any other warnings to take care of.

EDIT: the doozer CI buil failed - no idea what that is.

[nlohmann]

Sorry for the noise with doozer - it is a CI which I am evaluating. I now disabled building pull requests. The message should disappear with the next commit.

[nlohmann]

Here are the warnings from clang: clang_warnings.txt. There are no warnings reported in the develop branch with Catch.

[onqtam]

So here is the breakdown:

• total: 413

• Wnewline-eof - 30 - easily fixed by adding a new line at the end of doctest (although I would feel awkward releasing an entire new version just for a new line so it could be done locally? idk...) - or just silencing that warning

• Wunused-comparison - 150 - fixed by what I suggest in the previous comments

• Wunused-result - 228 - fixed by what I suggest in the previous comments

• only 5 other left

  • Wexit-time-destructors - 4 - seems that those are generated from the json code -

    src/unit-conversions.cpp:1376:1: warning: declaration requires an exit-time destructor [-Wexit-time-destructors]
    NLOHMANN_JSON_SERIALIZE_ENUM(cards,
    

    json/test/src/unit-conversions.cpp

    Line 1374 in 1126c9c

    NLOHMANN_JSON_SERIALIZE_ENUM(cards,

  • Wdouble-promotion - 1 - https://github.com/nlohmann/json/blob/develop/test/src/unit-regression.cpp#L290

• So the majority of the warnings can be fixed using what was suggested in a previous comment (+ the newline nuisance)

• Wexit-time-destructors was being silenced before the move to doctest - you could look how doctest avoids that warning in its DOCTEST_TEST_CASE macro - I removed the -Wno-exit-time-destructors warning from the makefile because doctest doesn't cause such trouble

• that double promotion one can be easily fixed by a cast - its just a diagnostic because of performance pessimization

Let me know how you think we should proceed.

[nlohmann]

I'm fine with whatever changes you propose, and I'd rather have changes locally (best: at one place like the compatibility header) than silencing warnings.

[onqtam]

Ok, I'll look into this tonight and probably push something.

[nlohmann]

And here are the warnings from GCC: gcc_warnings.txt.

[onqtam]

I fixed most warnings in commit 5d511a6 but the last 10-20 (-Wfloat-equal and -Wsign-promo) are fixed by the last commit a0000c3 and it ain't the most gracious way to do it - you be the judge. I could switch to using doctest::Approx for the -Wfloat-equal.

[onqtam]

Try the latest version of the branch and let me know

[nlohmann]

There is one Clang warning left:

In file included from src/unit.cpp:31:
In file included from thirdparty/doctest/doctest_compatibility.h:5:
thirdparty/doctest/doctest.h:1601:9: warning: '~ContextScope' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
        ~ContextScope();
        ^
thirdparty/doctest/doctest.h:694:13: note: overridden virtual function is here
    virtual ~IContextScope();
            ^

These are the remaining GCC warnings:
gcc_warnings.txt

Meanwhile, also #1551 is merged, which should resolve all nodiscard-related issues.

[onqtam]

let me know if there is anything left

[nlohmann]

Looks good to me.

[nlohmann]

---

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

• ✨ New Feature
• 🐛 Bug Fix
• ⚡️ Improvement
• 🔨 Further Change
• 🔥 Deprecated function

Description

• Moved the test suite from Catch to doctest. moved from Catch to doctest for unit tests #1439

Contributor entry

• Viktor Kirilov moved the test suite from Catch to doctest.

---

[nlohmann]

Thanks a lot for your patience!

[onqtam]

Thanks for the interest in doctest in the first place - almost 1.5 years ago :)