Remove the boost test runner

[rustaceanrob]

tl;dr: Make an equivalent test runner to boost with simpler macros and tighter coupling to bitcoin-specific code.

Motivation

There are a number of problems with using dependencies in general. Bugs must either be promptly upstreamed or patched, projects may be abandoned, and they are not tailored to a particular use case. bitcoind has a small number of dependencies and may have fewer very soon (bitcoin#34411). One of the final remaining dependencies is boost, which is responsible for the unit tests and multi-index. This PR removes boost for writing and running unit tests.

My problems with boost

Macros

When using boost, there are 3 ways of writing the same thing, each with different outcomes

• BOOST_CHECK(expr1 == expr2): failures show the expression, but no values for expr1 and expr2
• BOOST_CHECK_EQUAL(expr1, expr2): failures show the expression and values, but requires developers remember CHECK_EQUAL
• BOOST_TEST(expr1 == expr2): failures show the expression and values, and developers may use familiar operands (==, !=)

All three are preserved to maintain backwards compatibility - presumably, but this results in all 3 being used throughout the repository. As a bonus there is a 4th way, with added context

• BOOST_CHECK_MESSAGE(expr1 == expr2, "the context"): failures show the expression and message, but no values for expr1 and expr2

To make debugging and review easier, all of these should be unified to a single macro. Test sites use operands, print helpful messages - values are always included, and do not vary in approach (deciding between BOOST_TEST, CHECK, and BOOST_CHECK_EQUAL is unintuitive)

Example boost outputs

BOOST_AUTO_TEST_CASE(mustfail)
{
    auto a{1};
    auto b{2};
    BOOST_CHECK(a == b);
    BOOST_CHECK_EQUAL(a, b);
    BOOST_TEST(a == b);
    COutPoint outpoint_1{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 0};
    COutPoint outpoint_2{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 1};
    BOOST_CHECK(outpoint_1 == outpoint_2);
    // Doesn't even compile
    // BOOST_TEST(outpoint_1 == outpoint_2);
    // BOOST_CHECK_EQUAL(outpoint_1, outpoint_2);
}

Running 1 test case...
test/argsman_tests.cpp(31): error: in "argsman_tests/mustfail": check a == b has failed
test/argsman_tests.cpp(32): error: in "argsman_tests/mustfail": check a == b has failed [1 != 2]
test/argsman_tests.cpp(33): error: in "argsman_tests/mustfail": check a == b has failed [1 != 2]
test/argsman_tests.cpp(36): error: in "argsman_tests/mustfail": check outpoint_1 == outpoint_2 has failed

New outputs

TEST_CASE(mustfail)
{
    auto a{1};
    auto b{2};
    CHECK(a == b);
    COutPoint outpoint_1{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 0};
    COutPoint outpoint_2{Txid{"0000000000000000000000000000000000000000000000000000000000000100"}, 1};
    CHECK(outpoint_1 == outpoint_2);
}

Running 1 test cases...
[FAIL]: test/argsman_tests.cpp:30: CHECK(a == b)
1 == 2

[FAIL]: test/argsman_tests.cpp:33: CHECK(outpoint_1 == outpoint_2)
COutPoint(0000000000, 0) == COutPoint(0000000000, 1)

[FAIL] mustfail (2/2 checks failed)

No repository context

Many types implement ToString, but boost has no way of meaningfully using these representations (will change w/ future std::format). With a repository-specific test runner we can use these today. See example above.

Not extensible

With the test runner in this repository we can add things such as bitcoin#35139, implement string representations for more types, and implement ideas from bitcoin#8670.

Other issues

More issues with boost are raised in bitcoin#34666, bitcoin#8670. This PR does not resolve all of them, but one it does address immediately is banning comparisons of integers with different signs, previously allowed by boost. Using a test runner within the source also makes IWYU easier to work with.

High level changes

For those running tests, not a tremendous amount has changed in this PR. For instance, the doc page remains valid. Changes to the runner options include anything that is not help, run_test, log_level, list_content. This includes catch_system_errors which is always no. The log_levels have been reduced to 5 levels.

Changes in writing tests

• CHECK to compare two values with any ==, !=, >, etc
• CHECK(a == b, "my message") to append a message
• REQUIRE, same as CHECK, but will fail the test immediately
• CHECK_EQUAL_RANGES(it1, it2) to compare two iterators
• TEST_CASE(name) to add a test
• FIXTURE_TEST_CASE(name, Fixture) to add a test with a fixture
• TEST_SUITE_BEGIN/END to declare a suite

FIXTURE_TEST_SUITE_* is intentionally omitted. The readability of this macro is unclear as to if all unit tests share the same fixture or reinitialize the fixture. All test cases should be declared with FIXTURE if they use one.

Anticipated questions

Q: Why not name the macros BOOST_*?
A: This would defeat the advantages of using a new framework, which unifies CHECK calls. We should just rip the bandaid off now.

Q: Why not run the last commit as a scripted-diff?
A: The migration script is essentially a small tokenizer and would be difficult to review. IMO it is easier to review each callsite change.

Q: Why no concurrency?
A: The unit tests rely on the "current test case," so this would have to change to adopt a concurrent runner. I considered this out of scope for an already large change.

Migration

Migration script

import re
import sys
from pathlib import Path

_TOKEN_RE = re.compile(
    r'"(?:\\.|[^"\\])*"'
    r"|(?<![0-9a-fA-F'])'(?:\\.|[^'\\])*'"
    r"|(?P<open>[(\[{])"
    r"|(?P<close>[)\]}])"
    r"|(?P<id>[A-Za-z_][A-Za-z0-9_]*)"
    r"|(?P<op>&&|\|\||==|!=|<=|>=)"
    r"|(?P<other>.)",
    re.DOTALL,
)

def _walk(text, start=0):
    depth = 0
    for m in _TOKEN_RE.finditer(text, start):
        kind = m.lastgroup
        if kind == "close":
            depth -= 1
        yield depth, m
        if kind == "open":
            depth += 1

def find_close(text, open_idx):
    for depth, m in _walk(text, open_idx):
        if m.lastgroup == "close" and depth == 0:
            return m.end()

def split_args(args):
    parts, prev = [], 0
    for depth, m in _walk(args):
        if depth == 0 and m.lastgroup == "other" and m.group() == ",":
            parts.append(args[prev:m.start()])
            prev = m.end()
    parts.append(args[prev:])
    return parts

_ALWAYS = {"&&", "||", "?", "|", "&", "^"}
_COMPARE = {"==", "!=", "<=", ">="}

def has_top_level(text, ops):
    for depth, m in _walk(text):
        if depth != 0:
            continue
        tok = m.group()
        if tok not in ops:
            continue
        if m.lastgroup == "op" or tok == "?":
            return True
        if tok in "|&^" and text[m.end():m.end() + 1] != "=":
            prev = text[:m.start()].rstrip()
            if prev and (prev[-1].isalnum() or prev[-1] in "_)]}\"'"):
                return True
    return False

def wrap(expr, ops):
    expr = expr.strip()
    return f"({expr})" if has_top_level(expr, ops) else expr

_IDENT_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
_BOOST_RE = re.compile(r"\b(BOOST_[A-Z0-9_]+)\s*\(")
_FILE_NS_RE = re.compile(
    r"^[ \t]*namespace[ \t]+([A-Za-z_][A-Za-z0-9_]*)[ \t]*\{", re.MULTILINE
)

def ident(arg):
    name = arg.strip()
    if not _IDENT_RE.match(name):
        raise ValueError(f"not a valid C++ identifier: {name!r}")
    return name

def iter_pair(begin, end):
    """`X.begin()`/`X.end()` collapse to `X`; `->begin/end` to `*X`; else subrange."""
    b, e = begin.strip(), end.strip()
    for sep, deref in ((r"\.", ""), (r"->", "*")):
        mb = re.match(rf"^(.+){sep}begin\s*\(\s*\)$", b)
        me = re.match(rf"^(.+){sep}end\s*\(\s*\)$", e)
        if mb and me and (base := mb.group(1).strip()) == me.group(1).strip():
            return f"*({base})" if deref else base
    return f"std::ranges::subrange({b}, {e})"

_RENAMES = {
    "BOOST_CHECK_NO_THROW": "CHECK_NOTHROW",
    "BOOST_REQUIRE_NO_THROW": "REQUIRE_NOTHROW",
    "BOOST_CHECK_THROW": "CHECK_THROWS_AS",
    "BOOST_CHECK_EXCEPTION": "CHECK_EXCEPTION",
    "BOOST_TEST_MESSAGE": "TEST_MESSAGE",
    "BOOST_WARN_MESSAGE": "WARN_MESSAGE",
}

_COMPARE_OP = {
    f"BOOST_{k}_{n}": (k, o)
    for k in ("CHECK", "REQUIRE")
    for n, o in (("EQUAL", "=="), ("NE", "!="), ("LT", "<"), ("LE", "<="), ("GT", ">"), ("GE", ">="))
}

def rewrite_macros(text):
    while True:
        declared_ns = set(_FILE_NS_RE.findall(text))
        suite_stack = []
        out, cursor = [], 0

        for m in _BOOST_RE.finditer(text):
            start = m.start()
            if start < cursor:
                continue
            macro = m.group(1)
            close_end = find_close(text, m.end() - 1)
            if close_end is None:
                continue
            args = text[m.end():close_end - 1]
            replacement = None

            if macro in ("BOOST_CHECK", "BOOST_TEST", "BOOST_REQUIRE", "BOOST_TEST_REQUIRE"):
                target = "REQUIRE" if "REQUIRE" in macro else "CHECK"
                body = f"({args})" if has_top_level(args, _ALWAYS) else args
                replacement = f"{target}({body})"
            elif macro in _RENAMES:
                replacement = f"{_RENAMES[macro]}({args})"
            elif macro == "BOOST_ERROR":
                replacement = f"RECORD_ERROR({args.strip()})"
            elif macro in ("BOOST_CHECK_MESSAGE", "BOOST_REQUIRE_MESSAGE"):
                parts = split_args(args)
                if len(parts) >= 2:
                    target = "REQUIRE" if "REQUIRE" in macro else "CHECK"
                    replacement = f"{target}({wrap(parts[0], _ALWAYS)}, {','.join(parts[1:]).strip()})"
            elif macro in _COMPARE_OP:
                target, op = _COMPARE_OP[macro]
                parts = split_args(args)
                if len(parts) == 2:
                    a, b = wrap(parts[0], _ALWAYS | _COMPARE), wrap(parts[1], _ALWAYS | _COMPARE)
                    replacement = f"{target}({a} {op} {b})"
            elif macro == "BOOST_CHECK_EQUAL_COLLECTIONS":
                parts = split_args(args)
                if len(parts) == 4:
                    replacement = f"CHECK_EQUAL_RANGES({iter_pair(parts[0], parts[1])}, {iter_pair(parts[2], parts[3])})"
            elif macro == "BOOST_AUTO_TEST_CASE":
                name = ident(split_args(args)[0])
                fixture = suite_stack[-1][0] if suite_stack else None
                replacement = (f"FIXTURE_TEST_CASE({name}, {fixture})"
                               if fixture else f"TEST_CASE({name})")
            elif macro == "BOOST_FIXTURE_TEST_CASE":
                parts = split_args(args)
                if len(parts) >= 2:
                    replacement = f"FIXTURE_TEST_CASE({ident(parts[0])}, {parts[1].strip()})"
            elif macro in ("BOOST_AUTO_TEST_SUITE", "BOOST_FIXTURE_TEST_SUITE"):
                parts = split_args(args)
                name = ident(parts[0])
                fixture = parts[1].strip() if macro == "BOOST_FIXTURE_TEST_SUITE" else None
                # Re-open the namespace Boost's *_TEST_SUITE opened implicitly,
                # if the suite name matches a file-scope namespace.
                ns = name if name in declared_ns else None
                suite_stack.append((fixture, ns))
                prefix = f"namespace {ns} {{\n" if ns else ""
                replacement = f"{prefix}TEST_SUITE_BEGIN({name})"
            elif macro == "BOOST_AUTO_TEST_SUITE_END":
                ns = suite_stack.pop()[1] if suite_stack else None
                suffix = f"\n}} // namespace {ns}" if ns else ""
                replacement = f"TEST_SUITE_END(){suffix}"
            elif macro in ("BOOST_TEST_INFO", "BOOST_TEST_INFO_SCOPE"):
                replacement = f"/* {macro}({args}) */"

            if replacement is None:
                continue
            out.append(text[cursor:start])
            out.append(replacement)
            cursor = close_end

        out.append(text[cursor:])
        new = "".join(out)
        if new == text:
            return new
        text = new

def rewrite_includes(text):
    text = re.sub(r"^#include\s*<boost/test/[^>]+>\s*$",
                  "#include <test/util/framework.h>\n", text, flags=re.MULTILINE)
    text = re.sub(r"^#define\s+BOOST_TEST_MODULE\b.*$",
                  "#define BITCOIN_TEST_MAIN\n", text, flags=re.MULTILINE)
    text = re.sub(r"(?:#include <test/util/framework\.h>\n+)+",
                  "#include <test/util/framework.h>\n\n", text)
    return text.replace("boost::unit_test::framework::master_test_suite().argv[0]",
                        "framework::executable_path()")

_FILE_SWAPS = {
    "test/lint/lint-includes.py": [
        ("boost/test/included/unit_test.hpp", None),
        ("boost/test/unit_test.hpp", None),
    ],
    "test/lint/lint-tests.py": [
        ("grep_boost_test_suites", "grep_test_suites"),
        (r'r"^(BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)\("', r'r"^TEST_SUITE_BEGIN\("'),
        (r'r"/(.*?)\.cpp:(?:BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)\(\1(_[a-z0-9]+)?[,)]"',
         r'r"/(.*?)\.cpp:TEST_SUITE_BEGIN\(\1(_[a-z0-9]+)?\)"'),
        (r're.search(r"\((.*?)[,)]", x)', r're.search(r"\((.*?)\)", x)'),
    ],
}

def main():
    paths = [Path(a) for a in sys.argv[1:]] or sys.exit(__doc__.strip())
    files = []
    for p in paths:
        if p.is_file():
            files.append(p)
        elif p.is_dir():
            files.extend(f for f in p.rglob("*") if f.is_file())

    for suffix in _FILE_SWAPS:
        target = Path(suffix)
        if target.exists() and target not in files:
            files.append(target)

    for f in files:
        if f.name == "new_main.cpp":
            f.with_name("main.cpp").write_text(f.read_text())
            f.unlink()
            print(f"renamed {f}")
            continue
        if f.name == "CMakeLists.txt":
            old = f.read_text()
            new = (old
                   .replace("(BOOST_FIXTURE_TEST_SUITE|BOOST_AUTO_TEST_SUITE)", "TEST_SUITE_BEGIN")
                   .replace("--catch_system_error=no ", ""))
        elif f.suffix in (".cpp", ".h"):
            old = f.read_text()
            new = rewrite_macros(rewrite_includes(old))
        elif any(str(f).endswith(s) for s in _FILE_SWAPS):
            old = new = f.read_text()
        else:
            continue
        for suffix, swaps in _FILE_SWAPS.items():
            if str(f).endswith(suffix):
                for o, r in swaps:
                    if r is None:
                        new = re.sub(rf"^.*{re.escape(o)}.*\n", "", new, flags=re.MULTILINE)
                    else:
                        new = new.replace(o, r)
        if new != old:
            f.write_text(new)
            print(f"rewrote {f}")

if __name__ == "__main__":
    main()

[ismaelsadeeq]

Concept ACK, nice work

❯ build/bin/test_bitcoin
Running 658 test cases...

658 tests: 658 passed, 0 failed, 0 skipped (26431755 checks)

[josibake]

Concept ACK

Overall, looking great! Did some testing and uncovered a lil bug, will continue reading through the code tomorrow. There are also some stale document comments , not a priority but something to fix before the final pass.

[josibake]

I don't think this is being used. I ran some old justfile commands I had laying around and testdatadir wasn't used, but the tests ran. I suspect this is because test/main.cpp reads framework::user_args() but I don't see opts.passthrough being copied in.

[rustaceanrob]

Should be fixed in eccc7db. Please give 77bf05d a spin when you get a chance.

[rustaceanrob]

Was this fixed? Haven't looked for myself yet

[josibake]

Did another pass, apologies if the review is a bit nitty. I am trying to be a bit more thorough considering I do think this is an excellent candidate for upstreaming to Bitcoin Core.

[josibake]

Lots of globals here. I'd suggest instead introducing a TestContext, something like:

 struct TestContext {
    std::string full_name;
    TestStats stats;
    LogLevel log_level;
};

inline thread_local TestContext* active_context = nullptr;

Not too invasive, and now we can do things like:

TestContext ctx{
    .full_name = test_name(test_case),
    .log_level = opts.log_level,
};

active_context = &ctx;
test_case.fn();
active_context = nullptr;

// in record_check
active_context->stats

I didn't implement this but it seems simple enough to add as some future proofing for concurrency.

[rustaceanrob]

I'll mess with this tomorrow but makes sense conceptually

[josibake]

I think there might be some globals elsewhere that I didn't mention explicitly. I agree with trying to keep this design simple, so feel free to push back if I'm over complicating. But I do think its worth making sure that adding concurrency later is additive to this design and not a full rewrite

[rustaceanrob]

After poking at this I don't think we gain much from this refactor in the sequential case. I'm going to table this for now.

[josibake]

heh, I said feel free to push back but looking at this again, I realise I feel pretty strongly about this. I don't think we gain anything from my suggestion if we intend to keep this framework sequential, forever. But I don't think that's wise or a good design. By using global state you are backing this into a design corner prematurely. This necessarily means if/when someone comes along later to add concurrency, they need to redesign the entire framework because you built it around global state.

I think it would be much better to have a more robust architecture that allows us to add concurrency as a feature and not a total rewrite. I think my suggestion is simple enough and gets us there, but I'm open to other suggestions that are simpler. What I'm not open to is baking in sequential processing by building this around global state.

[rustaceanrob]

Comparison is the thief of joy, but FWIW none of the major frameworks support concurrent tests natively (Catch2, doctest - can run concurrently by running multiple processes with ranges of tests, something we can consider). There is also potential to use CMake for this.

[josibake]

Good catch with the child thread case. Based on our discussion on the call, I do think this is less about concurrency, and more about where mutable state lives. If we forget about concurrency for a second, I still think a test context is the right thing to do here since per test global state is bad, even for a sequential runner.

A good indication of this is some of the comments in the child thread tests that we have, and bypassing the old test framework with asserts. I got something working that I'm pretty happy with. Earlier I said it was only 46 lines but thats because I was using a mutex and your comment on the call made me realise this was indeed a performance hit (not a big one but enough to make me go blegh).

While whate I came up with makes things a little more complex, it feels conceptually like a better design and being able to remove all the asserts (done in a second commit) is evidence of that. I also had to change a few of the assert loops because CHECK is a bit heavier (and they didn't need to be run like that anyways, imo).

I pushed to this branch so you can easily check that all the tests pass but of course feel free to re-write / squash in this code however you see fit. I'd consider this very much hacked together proof of concept code:

https://github.com/2140-dev/bitcoin/tree/pr-38-test-context

[rustaceanrob]

Had a look at the example. I'm a bit confused with:

inline TestCaseContext& active_test_context()
{
    if (auto* context{local_test_context()}) return *context;
    if (auto* context{active_test_context_storage().load(std::memory_order_acquire)}) return *context;
    throw std::logic_error{"test framework assertion used outside an active test case"};
}

This is also global state for the current test case. What exactly does the architecture buy us? I see the main function uses a scoped context, but that one inevitably uses globals internally. Is this just to make a refactor easier in the future?

[josibake]

Fair question. The last push was a bit hasty, I tried to clean it up to make the benefit more clear. What we get from this approach imo is this: even for the sequential runner, test mutable state should have a per test lifetime. There is still a global because the assertion macros need to find the active test, but the state itself is now owned by the running TestCaseContext instead of living as separate globals.

I pushed a cleaned up version that tries to make that distinction clear:

• TestCaseContext owns the test name and check counters
• CHECK works from joined child threads via atomic counters
• REQUIRE / REQUIRE_NOTHROW are limited to the owning test thread, since throwing from a child thread cannot abort the parent test
• exception failures now preserve the checks recorded before the exception

This keeps the global strictly for the lookup mechanism for the currently active test. As evidence that this is the right direction: this lets us remove the raw asserts from the threaded tests and use framework checks there too. Failures from those paths now go through the same reporting/counting machinery as the rest of the test suite.

Pushed the commits to the same branch as before, and added a doc fixup commit for some stale wording: https://github.com/2140-dev/bitcoin/commits/pr-38-test-context/

[rustaceanrob]

11d0e67 adopts this style in my interpretation

[rustaceanrob]

In the spirit of not breaking scripts, 210cb9d changes the shorthand from -t= to -t and -l= to -l , same as boost, and --list to --list_content, also same as boost.

[ismaelsadeeq]

I did a quick pass at this and have a few comments, nothing major.

https://github.com/2140-dev/bitcoin/commits/pr-38-test-context/ seems to remove the need for the assert in threaded tests, and I like the per-test framework context.
I think it's a bit cleaner.

[ismaelsadeeq]

In "test: Add header-only test framework
" 8a52141

Perhaps add an example test file that demonstrates usage?

[rustaceanrob]

I added a doc comment. Example test file is overkill IMO, the usage is similar to the boost counterpart.

[ismaelsadeeq]

In Remove remaining BOOST references 7665580

There seems to be some remaining boost test references in the lint file and univalue test.

src/univalue/test/object.cpp:    BOOST_CHECK(!v.read("@{}"));
src/univalue/test/object.cpp:    BOOST_CHECK(!v.read("{} garbage"));
src/univalue/test/object.cpp:    BOOST_CHECK(!v.read("[]{}"));
src/univalue/test/object.cpp:    BOOST_CHECK(!v.read("{}[]"));
src/univalue/test/object.cpp:    BOOST_CHECK(!v.read("{} 42"));
test/lint/lint-includes.py:EXPECTED_BOOST_INCLUDES = [
test/lint/lint-includes.py:    for expected_boost in EXPECTED_BOOST_INCLUDES:
test/lint/lint-includes.py:    for expected_boost in EXPECTED_BOOST_INCLUDES:
test/lint/lint-includes.py:                       "Please remove it from EXPECTED_BOOST_INCLUDES in test/lint/lint-includes.py "
test/lint/test_runner/src/lint_cpp.rs:            r"BOOST_ASSERT\(",
test/lint/test_runner/src/lint_cpp.rs:BOOST_ASSERT must be replaced with Assert, REQUIRE, or CHECK to avoid an unnecessary
test/lint/test_runner/src/lint_cpp.rs:            r"BOOST_(AUTO_TEST_|FIXTURE_TEST_|GLOBAL_FIXTURE|DATA_TEST_|CHECK|REQUIRE|TEST|WARN)",
test/lint/test_runner/src/lint_cpp.rs:Boost.Test macros (BOOST_CHECK*, BOOST_REQUIRE*, BOOST_AUTO_TEST_*, BOOST_FIXTURE_TEST_*,
test/lint/test_runner/src/lint_cpp.rs:BOOST_TEST*, BOOST_WARN*, BOOST_GLOBAL_FIXTURE, BOOST_DATA_TEST_*) are no longer used.

[rustaceanrob]

The lints are to inform the user to stop using BOOST_*, the lint includes are still required as multi-index is used in the benches, and univalue is a subtree that needs a separate update.

[rustaceanrob]

958f817 addressed review comments

[rustaceanrob]

PR description updated

[josibake]

Looks great, thanks for incorporating the suggested changes. It feels like the require check on child threads is still incomplete, so I offered a suggestion that I think does what we actually want.

I also noticed the test cleanups are not in this PR, which makes sense considering this today just a replacement and meant to show that its drop in for the existing framework. I do think, however, its worth highlighting in the PR description that this new framework has better functionality than boost in that it has better thread safety and enforcement, which would allow us to remove the asserts in a follow up PR and make the entire test suite use the framework checks.

Overall, stellar work dude!

[josibake]

This detects child-thread REQUIRE, but it still stores that as a failed Result and then runs the shared failure path:

record_check(...);
if (!btc_test_res_.is_ok()) throw RequireFailed{};

So from a child thread REQUIRE(...) still throws RequireFailed. The test runner only catches RequireFailed around test_case.fn() on the owner thread; it cannot catch exceptions escaping a spawned std::thread unless that thread catches them itself.

I think this would still lead to an uncaught exception in a std::thread which calls std::terminate (brutal shutdown of the whole suite). Perhaps a better direction for a a graceful enforcement of the condition:

diff --git a/src/test/util/framework.hpp b/src/test/util/framework.hpp
index 327f4f505d..aa66e848fd 100644
--- a/src/test/util/framework.hpp
+++ b/src/test/util/framework.hpp
@@ -629,25 +629,27 @@ inline int run(int argc, char** argv)
     } while (false)

 /** Like CHECK, but aborts the current test on failure by throwing.
- * Subsequent checks in the test are skipped.
+ * Subsequent checks in the test are skipped. Use CHECK from child threads,
+ * because throwing in a child thread cannot abort the parent test.
  *
  * Accepts the same optional stream-style failure message as CHECK. */
 #define REQUIRE(expr, ...)                                                                                        \
     do {                                                                                                          \
         auto& btc_test_ctx_{::framework::test_context()};                                                         \
-        BITCOIN_TEST_DIAG_PUSH                                                                                    \
-        ::framework::Result btc_test_res_ = ::framework::Result::ok();                                            \
-        if (btc_test_ctx_.on_owner_thread()) {                                                                    \
-            btc_test_res_ = ::framework::Decomposer{} <= expr;                                                    \
+        if (!btc_test_ctx_.on_owner_thread()) {                                                                   \
+            ::framework::Result btc_test_res_{::framework::Result::failed(                                        \
+                "REQUIRE used from a child thread; use CHECK in child threads.")};                                \
+            ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE", #expr, __FILE__, __LINE__);       \
         } else {                                                                                                  \
-            btc_test_res_ = ::framework::Result::failed("REQUIRE checks must not be thrown on spawned threads."); \
+            BITCOIN_TEST_DIAG_PUSH                                                                                \
+            ::framework::Result btc_test_res_ = ::framework::Decomposer{} <= expr;                                \
+            BITCOIN_TEST_DIAG_POP                                                                                 \
+            std::ostringstream btc_test_os_;                                                                      \
+            __VA_OPT__(if (!btc_test_res_.is_ok()) btc_test_os_ << __VA_ARGS__;)                                  \
+            ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE", #expr, __FILE__, __LINE__,         \
+                                      btc_test_res_.is_ok() ? std::string{} : btc_test_os_.str());                \
+            if (!btc_test_res_.is_ok()) throw ::framework::RequireFailed{};                                       \
         }                                                                                                         \
-        BITCOIN_TEST_DIAG_POP                                                                                     \
-        std::ostringstream btc_test_os_;                                                                          \
-        __VA_OPT__(if (!btc_test_res_.is_ok()) btc_test_os_ << __VA_ARGS__;)                                      \
-        ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE", #expr, __FILE__, __LINE__,             \
-                                  btc_test_res_.is_ok() ? std::string{} : btc_test_os_.str());                    \
-        if (!btc_test_res_.is_ok()) throw ::framework::RequireFailed{};                                           \
     } while (false)

 /** Passes if `expr` throws any exception. */
@@ -679,22 +681,26 @@ inline int run(int argc, char** argv)
                                   #expr, __FILE__, __LINE__);                    \
     } while (false)

-/** Like CHECK_NOTHROW, but aborts the current test on failure. */
+/** Like CHECK_NOTHROW, but aborts the current test on failure. Use
+ * CHECK_NOTHROW from child threads. */
 #define REQUIRE_NOTHROW(expr)                                                                                     \
     do {                                                                                                          \
         auto& btc_test_ctx_{::framework::test_context()};                                                         \
-        ::framework::Result btc_test_res_{::framework::Result::ok()};                                             \
-        try {                                                                                                     \
-            expr;                                                                                                 \
-        } catch (...) {                                                                                           \
-            btc_test_res_ = ::framework::Result::failed("unexpectedly threw");                                    \
-        }                                                                                                         \
         if (!btc_test_ctx_.on_owner_thread()) {                                                                   \
-            btc_test_res_ = ::framework::Result::failed("REQUIRE checks must not be thrown on spawned threads."); \
+            ::framework::Result btc_test_res_{::framework::Result::failed(                                        \
+                "REQUIRE_NOTHROW used from a child thread; use CHECK_NOTHROW in child threads.")};                \
+            ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE_NOTHROW", #expr, __FILE__, __LINE__); \
+        } else {                                                                                                  \
+            ::framework::Result btc_test_res_{::framework::Result::ok()};                                         \
+            try {                                                                                                 \
+                expr;                                                                                             \
+            } catch (...) {                                                                                       \
+                btc_test_res_ = ::framework::Result::failed("unexpectedly threw");                                \
+            }                                                                                                     \
+            ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE_NOTHROW",                            \
+                                      #expr, __FILE__, __LINE__);                                                 \
+            if (!btc_test_res_.is_ok()) throw ::framework::RequireFailed{};                                       \
         }                                                                                                         \
-        ::framework::record_check(btc_test_ctx_, btc_test_res_, "REQUIRE_NOTHROW",                                \
-                                  #expr, __FILE__, __LINE__);                                                     \
-        if (!btc_test_res_.is_ok()) throw ::framework::RequireFailed{};                                           \
     } while (false)

 /** Passes only if `expr` throws an exception derived from `ExceptionType`. */

(I resisted the urge to write a helper function out of respect for u @rustaceanrob 🥲 )

[rustaceanrob]

Applied this feedback in abdf6c3

[josibake]

ACK 35b7ded

[ismaelsadeeq]

bitcoind has a small number of dependencies and may have fewer very soon (bitcoin#34411).

It does already :)

[ismaelsadeeq]

LGTM, just a minor issue noticed.

[ismaelsadeeq]

In "scripted-diff: Unroll && conditions in tests" 4cdcaf2

It is worth northing that this changes behaviour a bit, splitting BOOST_CHECK(a && b && c) to BOOST_CHECK(a); BOOST_CHECK(b); BOOST_CHECK(c); pinpoints which sub-condition failed.

If you don't want to change the behaviour u can unroll like the subsequent commit, or mention this in the commit body with why.

[ismaelsadeeq]

In "test: Add header-only test framework" abdf6c3

Hmm, this line is smelly, a mutable and not thread safe! I think a comment on why this bool is not atomic and thread safe would be helpful.

I would suggest to move it out?

suggestion

diff --git a/src/test/util/framework.hpp b/src/test/util/framework.hpp
index 9e8e79de48..d0bf3e7d69 100644
--- a/src/test/util/framework.hpp
+++ b/src/test/util/framework.hpp
@@ -155,7 +155,6 @@ class TestContext
 private:
     std::atomic<int> m_checks{0};
     std::atomic<int> m_failed{0};
-    bool m_did_throw{false};
     const std::string m_full_name;
     const std::thread::id m_owner_thread{std::this_thread::get_id()};

@@ -183,14 +182,9 @@ public:
         }
     }

-    void exception_occured()
+    bool did_fail() const noexcept
     {
-        m_did_throw = true;
-    }
-
-    bool did_fail()
-    {
-        return m_did_throw || m_failed.load(std::memory_order_relaxed) > 0;
+        return m_failed.load(std::memory_order_relaxed) > 0;
     }

     TestStats stats() const noexcept
@@ -502,25 +496,30 @@ inline int run(int argc, char** argv)
         }

         TestContext ctx{test_name(test_case)};
+        bool threw = false;
         try {
             test_case.fn();
         } catch (const RequireFailed&) {
         } catch (const std::exception& e) {
-            ctx.exception_occured();
+            threw = true;
             log(LogLevel::Error, "EXCEPTION in %s: %s\n", test_case.name, e.what());
         } catch (...) {
-            ctx.exception_occured();
+            threw = true;
             log(LogLevel::Error, "EXCEPTION in %s: %s\n", test_case.name, "unknown exception");
         }
         const auto stats = ctx.stats();
         summary.total_checks += stats.checks;
-        if (!ctx.did_fail()) {
+        // A test fails if it threw an unexpected exception or any check failed.
+        // `threw` is reported separately because an exception aborts the body
+        // and can leave zero failed checks recorded; the exception text itself
+        // was already logged above.
+        if (!threw && !ctx.did_fail()) {
             ++summary.passed;
             log(LogLevel::Info, "[ OK ] %s (%d checks)\n", test_case.name, stats.checks);
         } else {
             ++summary.failed;

[rustaceanrob]

I like the concept of encapsulating all test state within TestContext, but indeed I think this should be an atomic bool in that case. Maybe it can be left as a quick follow up or otherwise I can get to it tomorrow.

[ismaelsadeeq]

Yeah, it doesn't feel like a test state since it's not mutated within the class itself.
m_failed belongs to the context because it is mutated within apply_result when the result is not ok.

You can also make m_did_throw atomic and expose a setter, but that just makes it more obvious :P. Except if you are going to somehow set m_did_throw within apply_result as well.

Note: I don't feel strongly about this. I am fine when it's atomic with a setter, just bringing the issue up for discussion.

[josibake]

Personally, I'd be fine with a quick follow-up. I agree it is test state because it is relevant when a single test is running and would need to be reset when we move to the next test.

Why I'd be fine with a follow-up: for me the structure of the framework is solid. This feels like a "fix racy bool" follow-up. Good catch, tho, @ismaelsadeeq !

[josibake]

lgtm + ack, sounds like a merge :D