Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using check macros outside of tests #80

Open
cschreib opened this issue Apr 10, 2023 · 2 comments
Open

Allow using check macros outside of tests #80

cschreib opened this issue Apr 10, 2023 · 2 comments
Labels
enhancement New feature or request
Milestone

Comments

@cschreib
Copy link
Member

cschreib commented Apr 10, 2023

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.

@cschreib cschreib added the enhancement New feature or request label Apr 10, 2023
@cschreib cschreib added this to the v2.0 milestone Apr 10, 2023
@bitrunner
Copy link

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
Copy link
Member Author

cschreib commented May 27, 2024

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 #188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants