Modernizing the Botan test framework

[reneme]

While working on TLS 1.3 we wanted to apply Test-Driven-Development for some components that need to be built from scratch. For that, a more convenient unit testing framework would be much appreciated.

For our end-user product, we use and love Catch2 for its flexibility and expressiveness. Furthermore, Catch2 is pluggable and could even be integrated with the current home brew test framework. We're rather confident that this could be set up in a way that Catch2 uses the existing framework as the "reporting backend" to avoid having multiple independent unit test binaries.

Of course this comes at the expense of a mandatory build dependency. To the best of our knowledge, actually the first such dependency. Though, Catch2 can be used as a header-only library, so integration should be fairly straight forward. Another potential drawback is compilation speed, where Botan's current test framework will definitely have the upper hand.

Our two cents: The added testing convenience is worth it for sure!

[randombit]

For a period of time Catch was used in some tests. But I really hated how badly it hurt compilation speed. And it was never sufficient since Catch doesn't support features we need such as reading test data from files, running tests under multiple CPUID configurations, running tests across multiple threads, etc etc, so we even with Catch we are still maintaining a lot of test infrastructure.

That said I understand the current framework isn't as helpful as it could be. It was largely written in one spurt in 2015 (cf05aea) to put all the then-existing tests in a single framework, with a few minor extensions since then.

I would much rather identify specific areas where Catch offers a superior experience and steal them, especially when they don't involve elaborate template metaprogramming that blows up compilation time, or macros that sometimes accidentally double-evaluate their arguments (this was a bug in Catch at the time, which when I ran into it put me in such a state that I wrote rewrote all the tests in one sitting in order to remove it).

This is probably also a good time to mention my general feelings on external dependencies.

Some things that bother me about the current design

• Have to define a class for every test. (Not actually true with BOTAN_REGISTER_TEST_FN, but that is barely used at all atm)
• The various test_eq, test_ne functions on Result pretty random and largely driven by what was needed at the time.
• Have to reference result. everywhere (hard to avoid without macros though)
• In file based tests it's possible to request fields that don't exist (eg vars.get_opt_bin("BadFieldName") should err if the test doesn't define that field)
• Distinct test definition and test registration steps.
• Your complaints here :)

Not meaning to shut this conversation down! A problem with the test system is a problem with the library, because it means the library will inevitably become less well tested than it otherwise would have been. I am just not convinced adopting Catch is the right approach, as compared to identifying the pain points and then addressing them. Since our tests are the only user of the test framework, there is no limits wrt changes.

[reneme]

Thanks for sharing your views on this in such detail. I understand that adding Catch as a dependency would fail to tackle all requirements the library has, as you pointed out. Hence, forcing contributers to understand and master two test frameworks. Adding the compilation time overhead (which I think should be re-evaluated with the latest versions of Catch, though), rule out the adoption of Catch.

Therefore, I changed the title of this discussion to "Modernizing the Botan test framework" and repurpose it to collect pain points and discuss improvements.

Together with @hrantzsch, I started to play around with BOTAN_REGISTER_TEST_FN, yesterday. We added support to register multiple free-standing test functions and allow individual test functions to return a std::vector<Test::Result> in-lieu of a single Test::Result.

Also we wrote a simple CHECK wrapper that makes managing Test::Result slightly more convenient. See this new test file in the ongoing TLS 1.3 branch for a show case (and the required patch in tests.h). Those small changes simplify the grouping of tests quite a bit already, to write test like this:

std::vector<Test::Result>
basic_sanitization() {
    // generic test fixture goes here
    auto rl = TLS::Record_Layer();

    // list of individual tests goes here
    return {
        CHECK("record overflow detection", [&](auto &result) {
            result.confirm(rl.parse_records(/* ... */), /* ... */);
        }),
        CHECK("invalid record type", [&](auto &result) {
            /* ... */
        })
    };
}

std::vector<Test::Result>
handling_of_fragmented_records() {
    return {
        CHECK(/* ... */),
        CHECK(/* ... */),
        CHECK(/* ... */)
    };
}

BOTAN_REGISTER_TEST_FN("tls", "record_layer", basic_sanitization, handling_of_fragmented_records);

Obviously, this is work in progress and criticism is appreciated. E.g. the lambda in CHECK that receives the result feels pretty clumsy.

[reneme]

For reference: we added a PR implementing the proposed additions independently from the TLS 1.3 branch: #2941

[reneme]

We had the need to merge fine-grained Test::Result objects into single umbrella-results. A solution was proposed and accepted but we agreed that it is sub-par. I'll leave that here for future reference: #2967

[reneme]

For reference: We're working on more sophisticated CI integration of the test framework via a JUnit5-based test reporting: #3028.