snitch-org / snitch

Lightweight C++20 testing framework.
Boost Software License 1.0
268 stars 10 forks source link

Allow using check macros outside of tests #80

Open cschreib opened 1 year ago

cschreib commented 1 year ago

Currently, the check macros (REQUIRE(...), CHECK(...), etc.) must be called inside a TEST_CASE* (could be in another function, but there must be a TEST_CASE* higher up the call stack). They rely on thread-local state, through get_current_test(), which is currently set up by registry. If no test is running, get_current_test() cannot return a valid value, and thus terminates.

This would be easy to change so that, if no test is running, get_current_test() returns a fallback, which is then used as the "outside of test" reporting mechanism. The user could even access the fallback reporter, and customize the reporting function (as is currently possible for registry). This would allow snitch to be used as a general-purpose assertion checking library.

Our current design is not optimal for this however. All that is needed at this stage is access to the reporting function; no other data of registry is actually being accessed or modified. Yet the reporting function has the following signature: void(const registry&, const event::data&) noexcept. Ideally, this should have only taken the const event::data& argument. If external state was required (which is why we have the const registry& argument), it could be achieved by implementing the reporter function as a member function of a class).

But changing this breaks our API, so it would have to wait for a v2.

bitrunner commented 6 months ago

Another limitation of the current thread-local state design is that test cases that spawn threads that check things cannot be written. In other words, the thread-safety of something under test cannot be checked from spawned threads. Even though there is a test running, get_current_test() triggers std::terminate from the spawned threads.

Consider this contrived example that tests the thread-safety of std::atomic:

TEST_CASE("atomic uint64_t")
{
    static constexpr uint32_t ITERATIONS = 10000;
    static constexpr uint32_t THREADS = 10;

    std::atomic<uint64_t> count(0);

    auto increment_it =
        [&]()
        {
            auto last_value = count.fetch_add(1, std::memory_order_relaxed);
            for (uint32_t iteration(1); iteration < ITERATIONS; ++iteration)
            {
                const auto x = count.fetch_add(1, std::memory_order_relaxed);
                CHECK(x > last_value);
                last_value = x;
            }
        };

    {
        std::vector<std::jthread> threads;
        while (threads.size() < THREADS) { threads.emplace_back(increment_it); }
    }

    CHECK(count == (ITERATIONS * THREADS));
}

The CHECK at the end is fine, but the CHECK inside the lambda that spawned threads run triggers std::terminate. In this sort of test, one would expect a CHECK that evaluates to false in the spawned thread would fail the test case the parent thread is running.

cschreib commented 6 months ago

That's a good point. In fact the reporting isn't thead-safe (e.g., counters etc aren't atomics), so it is not safe to report assertions from multiple threads within the same test anyway. So far, snitch was only designed to allow parallel test execution (although it can't do it yet, see #33), but not parallel assertions within a test. So std::terminate is the right thing to do until this is sorted. This is a different topic though; would you mind copying your message into a new issue?

Edit: I've done it myself https://github.com/snitch-org/snitch/issues/188