Remove the boost test runner

[rustaceanrob]

Replaces the boost test runner with a simple header-only variant. Simplifies the macros to the following:

• CHECK, valid with any comparison operator (e.g. ==, !=), optional message
• REQUIRE, valid with any comparison operator, optional message
• CHECK_EQUAL_RANGES, better debugging for vectors
• THROW_*, macros for checking throwing conditions
• Info and warn messages

This framework has 4 log levels, offers test filtering, and also includes suites and fixtures. The runner executes in approximately the same time as boost, and should compile faster as well.

A list of a few advantages over boost in addition to macro simplification:

• Compilation fails when attempting to compare integers with different sign types
• Types that implement ToString are printed when CHECK or REQUIRE fails
• Flexible to additions (ABORT or ASSERT macros that fail-fast)

To try it out:

./build/bin/test_bitcoin -l info

./build/bin/test_bitcoin -t versionbits_tests

On commit 1 and commit 2, these are given by guidance of Catch2. && conditions are unrolled into two separate checks.

Replace all boost macros (third commit)

import argparse
import re
import sys
from pathlib import Path


# ---------------------------------------------------------------------------
# Parser primitives
# ---------------------------------------------------------------------------

_DIGIT_SEP_PREV = set("0123456789abcdefABCDEF'")


def _is_char_literal_open(text: str, i: int) -> bool:
    """A `'` at position i is a char-literal opener unless the previous
    character is a digit, hex letter, or another digit separator (C++14
    digit-separator usage like 1'000'000 or 0xFF'FF)."""
    return i == 0 or text[i - 1] not in _DIGIT_SEP_PREV


def find_matching_paren(text: str, open_idx: int) -> int | None:
    """Return index of ')' that matches '(' at open_idx, respecting brackets,
    braces, and string/char literals."""
    assert text[open_idx] == "("
    depth = 0
    i = open_idx
    in_str = False
    in_char = False
    while i < len(text):
        c = text[i]
        if in_str:
            if c == "\\":
                i += 2
                continue
            if c == '"':
                in_str = False
        elif in_char:
            if c == "\\":
                i += 2
                continue
            if c == "'":
                in_char = False
        elif c == '"':
            in_str = True
        elif c == "'" and _is_char_literal_open(text, i):
            in_char = True
        elif c in "([{":
            depth += 1
        elif c in ")]}":
            depth -= 1
            if depth == 0:
                return i
        i += 1
    return None


def _is_binary_context(text: str, i: int) -> bool:
    """At position i (start of `|`, `&`, or `^`), is the previous non-whitespace
    character one that makes this a binary operator (not unary `&`)?"""
    j = i - 1
    while j >= 0 and text[j] in " \t\n":
        j -= 1
    if j < 0:
        return False
    return text[j].isalnum() or text[j] in "_)]}\"'"


def needs_parens(expr: str) -> bool:
    """True if expr contains a top-level operator that would interact badly
    with the framework's Decomposer (which captures via `<=`, applies one
    comparison via CapturedExpression's overloads, then yields a Result that
    has no operators).

    Detected: `&&`, `||`, `==`, `!=`, `<=`, `>=`, `|`, `&`, `^`. Bare `<` /
    `>` are intentionally skipped because they are commonly template
    brackets (`vector<int>`, `static_cast<T>`) which this scanner can't
    distinguish from comparison without a real C++ parser; if a real
    comparison with bare `<`/`>` ends up in an operand, the resulting build
    error is easy to spot and fix at the call site.

    Compound assignments (|=, &=, ^=) and unary `&` (address-of) are skipped."""
    depth = 0
    i = 0
    in_str = False
    in_char = False
    n = len(expr)
    while i < n:
        c = expr[i]
        if in_str:
            if c == "\\":
                i += 2
                continue
            if c == '"':
                in_str = False
            i += 1
            continue
        if in_char:
            if c == "\\":
                i += 2
                continue
            if c == "'":
                in_char = False
            i += 1
            continue
        if c == '"':
            in_str = True
            i += 1
            continue
        if c == "'" and _is_char_literal_open(expr, i):
            in_char = True
            i += 1
            continue
        if c in "([{":
            depth += 1
            i += 1
            continue
        if c in ")]}":
            depth -= 1
            i += 1
            continue
        if depth == 0:
            two = expr[i : i + 2] if i + 1 < n else ""
            if two in ("&&", "||", "==", "!=", "<=", ">="):
                return True
            if c in "|&^" and two[1:] != "=" and _is_binary_context(expr, i):
                return True
        i += 1
    return False


# Backwards-compat alias for has_top_level_bitwise's old name.
has_top_level_bitwise = needs_parens


def wrap_if_bitwise(expr: str) -> str:
    expr = expr.strip()
    return f"({expr})" if needs_parens(expr) else expr


def split_top_level_args(args: str) -> list[str]:
    """Split macro arg-list on top-level commas."""
    parts: list[str] = []
    depth = 0
    last = 0
    i = 0
    in_str = False
    in_char = False
    while i < len(args):
        c = args[i]
        if in_str:
            if c == "\\":
                i += 2
                continue
            if c == '"':
                in_str = False
        elif in_char:
            if c == "\\":
                i += 2
                continue
            if c == "'":
                in_char = False
        elif c == '"':
            in_str = True
        elif c == "'" and _is_char_literal_open(args, i):
            in_char = True
        elif c in "([{":
            depth += 1
        elif c in ")]}":
            depth -= 1
        elif c == "," and depth == 0:
            parts.append(args[last:i])
            last = i + 1
        i += 1
    parts.append(args[last:])
    return parts


# ---------------------------------------------------------------------------
# Macro tables
# ---------------------------------------------------------------------------

SIMPLE_RENAMES = {
    "BOOST_CHECK": "CHECK",
    "BOOST_REQUIRE": "REQUIRE",
    "BOOST_TEST": "CHECK",
    "BOOST_TEST_REQUIRE": "REQUIRE",
    "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",
}

MESSAGE_RENAMES = {
    "BOOST_CHECK_MESSAGE": "CHECK",
    "BOOST_REQUIRE_MESSAGE": "REQUIRE",
}

COMPARISON_REWRITES = {
    "BOOST_CHECK_EQUAL":   ("CHECK", "=="),
    "BOOST_REQUIRE_EQUAL": ("REQUIRE", "=="),
    "BOOST_CHECK_NE":      ("CHECK", "!="),
    "BOOST_CHECK_LT":      ("CHECK", "<"),
    "BOOST_CHECK_LE":      ("CHECK", "<="),
    "BOOST_CHECK_GT":      ("CHECK", ">"),
    "BOOST_CHECK_GE":      ("CHECK", ">="),
    "BOOST_REQUIRE_NE":    ("REQUIRE", "!="),
    "BOOST_REQUIRE_LT":    ("REQUIRE", "<"),
    "BOOST_REQUIRE_LE":    ("REQUIRE", "<="),
    "BOOST_REQUIRE_GT":    ("REQUIRE", ">"),
    "BOOST_REQUIRE_GE":    ("REQUIRE", ">="),
}

COMMENTED_OUT = (
    "BOOST_TEST_INFO",
    "BOOST_TEST_INFO_SCOPE",
)

REPORTED_ONLY = (
    "BOOST_CHECK_CLOSE",
)

STRUCTURAL_MACROS = (
    "BOOST_AUTO_TEST_CASE",
    "BOOST_FIXTURE_TEST_CASE",
    "BOOST_AUTO_TEST_SUITE",
    "BOOST_FIXTURE_TEST_SUITE",
    "BOOST_AUTO_TEST_SUITE_END",
    "BOOST_CHECK_EQUAL_COLLECTIONS",
    "BOOST_ERROR",
)

ALL_KNOWN = (
    list(SIMPLE_RENAMES)
    + list(MESSAGE_RENAMES)
    + list(COMPARISON_REWRITES)
    + list(COMMENTED_OUT)
    + list(REPORTED_ONLY)
    + list(STRUCTURAL_MACROS)
)
# Longer names first so the alternation doesn't match a prefix.
KNOWN_SORTED = sorted(set(ALL_KNOWN), key=len, reverse=True)
MACRO_RE = re.compile(r"\b(" + "|".join(re.escape(m) for m in KNOWN_SORTED) + r")\s*\(")


# ---------------------------------------------------------------------------
# Helpers
# ---------------------------------------------------------------------------

def quote_name(name_arg: str) -> str:
    return f'"{name_arg.strip()}"'


def collapse_iter_pair(begin_expr: str, end_expr: str) -> str:
    """X.begin()/X.end() -> X. X->begin()/X->end() -> *X. Else subrange()."""
    b = begin_expr.strip()
    e = end_expr.strip()
    m_b = re.match(r"^(.+)\.begin\s*\(\s*\)$", b)
    m_e = re.match(r"^(.+)\.end\s*\(\s*\)$", e)
    if m_b and m_e and m_b.group(1).strip() == m_e.group(1).strip():
        return m_b.group(1).strip()
    m_b = re.match(r"^(.+)->begin\s*\(\s*\)$", b)
    m_e = re.match(r"^(.+)->end\s*\(\s*\)$", e)
    if m_b and m_e and m_b.group(1).strip() == m_e.group(1).strip():
        return f"*({m_b.group(1).strip()})"
    return f"std::ranges::subrange({b}, {e})"


# ---------------------------------------------------------------------------
# Main rewrite
# ---------------------------------------------------------------------------

def rewrite_includes_and_module(text: str) -> str:
    # Any boost/test include -> framework include.
    text = re.sub(
        r"^#include\s*<boost/test/[^>]+>\s*$",
        "#include <test/util/framework.hpp>",
        text,
        flags=re.MULTILINE,
    )
    # BOOST_TEST_MODULE in standalone test files -> framework main hook.
    text = re.sub(
        r"^#define\s+BOOST_TEST_MODULE\b.*$",
        "#define BITCOIN_TEST_MAIN",
        text,
        flags=re.MULTILINE,
    )
    # Collapse adjacent duplicate framework includes (a boost file may have
    # included two headers from boost/test/).
    text = re.sub(
        r"(#include <test/util/framework\.hpp>\n)(?:[ \t]*#include <test/util/framework\.hpp>\n)+",
        r"\1",
        text,
    )
    return text


def file_namespaces(text: str) -> set[str]:
    """Return the set of namespace names declared anywhere in the file. Used
    to decide whether to wrap a test suite body in a matching namespace —
    Boost.Test opens `namespace <suite>` implicitly, but the framework does
    not, so a fixture/helper defined in `namespace foo` won't be visible
    inside the suite body without an explicit re-open."""
    return set(re.findall(r"^[ \t]*namespace[ \t]+([A-Za-z_][A-Za-z0-9_]*)[ \t]*\{", text, re.MULTILINE))


def rewrite_macros(text: str):
    """Walk macro calls in order, applying conversions while tracking the
    current fixture-suite stack. Returns (new_text, reports)."""
    out: list[str] = []
    cursor = 0
    # Each entry: (fixture-or-None, opened-namespace-or-None)
    suite_stack: list[tuple[str | None, str | None]] = []
    declared_namespaces = file_namespaces(text)
    reports: dict[str, list[int]] = {}

    for m in MACRO_RE.finditer(text):
        macro = m.group(1)
        call_start = m.start()
        paren_open = m.end() - 1
        paren_close = find_matching_paren(text, paren_open)
        if paren_close is None:
            continue
        args_text = text[paren_open + 1 : paren_close]
        line = text.count("\n", 0, call_start) + 1

        replacement: str | None = None

        if macro in SIMPLE_RENAMES:
            target = SIMPLE_RENAMES[macro]
            # CHECK/REQUIRE go through Decomposer; a stray top-level bitwise
            # op would be applied to a Result. Wrap the whole expression.
            if target in ("CHECK", "REQUIRE") and has_top_level_bitwise(args_text):
                replacement = f"{target}(({args_text}))"
            else:
                replacement = f"{target}({args_text})"

        elif macro in MESSAGE_RENAMES:
            args = split_top_level_args(args_text)
            if len(args) >= 2:
                expr = wrap_if_bitwise(args[0])
                msg = ",".join(args[1:]).strip()
                replacement = f"{MESSAGE_RENAMES[macro]}({expr}, {msg})"

        elif macro in COMPARISON_REWRITES:
            new_name, op = COMPARISON_REWRITES[macro]
            args = split_top_level_args(args_text)
            if len(args) == 2:
                a = wrap_if_bitwise(args[0])
                b = wrap_if_bitwise(args[1])
                replacement = f"{new_name}({a} {op} {b})"

        elif macro == "BOOST_CHECK_EQUAL_COLLECTIONS":
            args = split_top_level_args(args_text)
            if len(args) == 4:
                lhs = collapse_iter_pair(args[0], args[1])
                rhs = collapse_iter_pair(args[2], args[3])
                replacement = f"CHECK_EQUAL_RANGES({lhs}, {rhs})"

        elif macro == "BOOST_ERROR":
            replacement = f"CHECK(false, {args_text.strip()})"

        elif macro == "BOOST_AUTO_TEST_CASE":
            args = split_top_level_args(args_text)
            name = quote_name(args[0])
            fixture = suite_stack[-1][0] if suite_stack else None
            if fixture:
                replacement = f"FIXTURE_TEST_CASE({name}, {fixture})"
            else:
                replacement = f"TEST_CASE({name})"

        elif macro == "BOOST_FIXTURE_TEST_CASE":
            args = split_top_level_args(args_text)
            if len(args) >= 2:
                name = quote_name(args[0])
                fixture = args[1].strip()
                replacement = f"FIXTURE_TEST_CASE({name}, {fixture})"

        elif macro == "BOOST_AUTO_TEST_SUITE":
            args = split_top_level_args(args_text)
            suite_name = args[0].strip()
            ns = suite_name if suite_name in declared_namespaces else None
            suite_stack.append((None, ns))
            prefix = f"namespace {ns} {{\n" if ns else ""
            replacement = f'{prefix}TEST_SUITE_BEGIN("{suite_name}")'

        elif macro == "BOOST_FIXTURE_TEST_SUITE":
            args = split_top_level_args(args_text)
            if len(args) >= 2:
                suite_name = args[0].strip()
                fixture = args[1].strip()
                ns = suite_name if suite_name in declared_namespaces else None
                suite_stack.append((fixture, ns))
                prefix = f"namespace {ns} {{\n" if ns else ""
                replacement = f'{prefix}TEST_SUITE_BEGIN("{suite_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 COMMENTED_OUT:
            replacement = f"/* {macro}({args_text}) */"

        elif macro in REPORTED_ONLY:
            reports.setdefault(macro, []).append(line)

        if replacement is None:
            continue

        out.append(text[cursor:call_start])
        out.append(replacement)
        cursor = paren_close + 1

    out.append(text[cursor:])
    return "".join(out), reports


def find_remaining_boost(text: str, exclude: set[str]) -> dict[str, list[int]]:
    """Scan for any BOOST_* identifier still present, excluding known-allowed
    ones (e.g. macros we deliberately left untouched)."""
    remaining: dict[str, list[int]] = {}
    for m in re.finditer(r"\bBOOST_[A-Z_]+", text):
        name = m.group(0)
        if name in exclude:
            continue
        line = text.count("\n", 0, m.start()) + 1
        remaining.setdefault(name, []).append(line)
    return remaining


def transform(text: str) -> tuple[str, dict[str, list[int]]]:
    text = rewrite_includes_and_module(text)
    text, reports = rewrite_macros(text)
    leftover = find_remaining_boost(text, exclude=set(REPORTED_ONLY))
    for macro, lines in leftover.items():
        reports.setdefault(macro, []).extend(lines)
    return text, reports


# ---------------------------------------------------------------------------
# CLI
# ---------------------------------------------------------------------------

def iter_source_files(paths: list[Path]):
    for p in paths:
        if p.is_file():
            if p.suffix in (".cpp", ".h", ".hpp", ".cc"):
                yield p
            continue
        for f in p.rglob("*"):
            if f.is_file() and f.suffix in (".cpp", ".h", ".hpp", ".cc"):
                yield f


def main() -> int:
    ap = argparse.ArgumentParser(
        description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter
    )
    ap.add_argument("paths", nargs="+", type=Path)
    ap.add_argument("--check", action="store_true", help="Exit 1 if any file would change")
    ap.add_argument("--quiet", action="store_true")
    args = ap.parse_args()

    total = 0
    changed = 0
    all_reports: dict[Path, dict[str, list[int]]] = {}

    for path in iter_source_files(args.paths):
        total += 1
        original = path.read_text()
        new_text, reports = transform(original)
        if new_text != original:
            changed += 1
            if not args.check:
                path.write_text(new_text)
            if not args.quiet:
                verb = "would rewrite" if args.check else "rewrote"
                print(f"{verb} {path}")
        if reports:
            all_reports[path] = reports

    if all_reports and not args.quiet:
        print("\nUnconverted BOOST_* identifiers (manual review):")
        for path in sorted(all_reports):
            for macro in sorted(all_reports[path]):
                for ln in all_reports[path][macro]:
                    print(f"  {path}:{ln}: {macro}")

    if not args.quiet:
        verb = "would rewrite" if args.check else "rewrote"
        print(f"\n{verb} {changed}/{total} file(s)")

    return 1 if args.check and changed > 0 else 0


if __name__ == "__main__":
    sys.exit(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.

[josibake]

Perhaps we are talking past each other, or my wording before was too vague. I am not suggesting everything go into a context. My concrete suggestion was:

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

which captures per test state. I think there is an argument that user args and log level could be per test state, as well, but would accept the push back that they are not (at this stage).

I disagree, however, that introducing a TestContext (or perhaps TestCaseContext) is intrusive. This facilitates concurrency in the future by allowing a parallel runner to create one context per test case, run it on a worker thread, and collect the results back (test_name, stats). By doing this now, we avoid needing to redesign record_check, REQUIRE , CHECK etc later.

I was suspicious of registry and current_test_suite, which is why I mentioned other globals, but I don't see any risks as of now. The main concern I am calling out is a future concurrency model will operate at the test case level, so we should absolutely avoid using global state for any test case level state.

Happy to elaborate more or throw up a patch that shows what I'm talking about.

[rustaceanrob]

I see the distinction now. I was particularly confused as I don't see a strong motivation for log level to be test-level at the moment, but encapsulating the statistics and other test-level state makes sense. Even so, I think we will have to live with a global current_test_name, as the fixtures depend on it, but making the internals like record_check thread-local seems reasonable.

[rustaceanrob]

I experimented a bit with this change. The simplest construction inline thread_local TestContext* active_context = nullptr;, does not work, as the unit tests themselves spawn threads. If these child threads call CHECK, for example, the thread_local context will be nullptr even though the parent thread has a context set. To circumvent this I tried making a custom framework::spawn that passes the current context to child threads, but that also does not work as some threads are spawned outside of the test case.

I'll fallback on the opinion that I'm not sure this needs to be accounted for now. A concurrent design would likely have to implement some form of shared state or message passing between the children processes to the parent. I am open to other suggestions, but I suspect accounting for concurrency would be a far more involved endeavor. The tests take around 70-75 seconds to run on my machine which I find reasonable.

[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]

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.