Repository: bitcoin/bitcoin
File: src/test/util_string_tests.cpp (C++)

Code before:
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <util/string.h>

#include <boost/test/unit_test.hpp>
#include <test/util/setup_common.h>

using namespace util;

BOOST_AUTO_TEST_SUITE(util_string_tests)

// Helper to allow compile-time sanity checks while providing the number of
// args directly. Normally PassFmt<sizeof...(Args)> would be used.
template <unsigned NumArgs>
inline void PassFmt(util::ConstevalFormatString<NumArgs> fmt)
{
    // Execute compile-time check again at run-time to get code coverage stats
    util::detail::CheckNumFormatSpecifiers<NumArgs>(fmt.fmt);
}
template <unsigned WrongNumArgs>
inline void FailFmtWithError(const char* wrong_fmt, std::string_view error)
{
    BOOST_CHECK_EXCEPTION(util::detail::CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*, HasReason{error});
}

BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
{
    PassFmt<0>("");
    PassFmt<0>("%%");
    PassFmt<1>("%s");
    PassFmt<0>("%%s");
    PassFmt<0>("s%%");
    PassFmt<1>("%%%s");
    PassFmt<1>("%s%%");
    PassFmt<0>(" 1$s");
    PassFmt<1>("%1$s");
    PassFmt<1>("%1$s%1$s");
    PassFmt<2>("%2$s");
    PassFmt<2>("%2$s 4$s %2$s");
    PassFmt<129>("%129$s 999$s %2$s");
    PassFmt<1>("%02d");
    PassFmt<1>("%+2s");
    PassFmt<1>("%.6i");
    PassFmt<1>("%5.2f");
    PassFmt<1>("%5.f");

Review comment (bug):
I like the idea of testing the validator together with the implementation!

But I'm not in love with the current approach.

Why are we testing `PassFmt<1>("%02d");` with `foo` - I don't think it helps with understanding how formatter works.

Also `PassFmt<1>("%s")` made sense when we were only validating the number of args, but we've extended it since, I think we should extend examples with the actual parameters, e.g.
* `PassFmt("%s", "test");`
* `PassFmt("%02d", 42);`
* `PassFmt("%12$s %2$s %1$s", "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12");`

In which case each example would document the format that we're exercising more specifically.

It would also obviate the parameter count by examples, would all be runtime (seems to me that would simplify a lot) and comparison would simply depend on the behavior of `tfm::format`, e.g. validation would be something like:
```C++
template <typename... Args>
inline bool ValidateFmt(const char* fmt, const char* expected_error, const Args&... args)
{
    try {
        tfm::format(fmt, args...);
        ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt);
        return true;
    } catch (const tfm::format_error&) {
        using ErrType = const char*;
        auto check_throw = [expected_error](const ErrType& str) { return str == expected_error; };
        BOOST_CHECK_EXCEPTION(ConstevalFormatString<sizeof...(Args)>::Detail_CheckNumFormatSpecifiers(fmt), ErrType, check_throw);
        return false;
    }
}

template <typename... Args>
void PassFmt(const char* fmt, const Args&... args) { BOOST_CHECK(ValidateFmt(fmt, nullptr, args...)); }

template <typename... Args>
void FailFmt(const char* fmt, const char* expected_error, const Args&... args) { BOOST_CHECK(!ValidateFmt(fmt, expected_error, args...)); }
```

and usage would be something like
```
    PassFmt("%%");
    PassFmt("%s", "test");

    auto err_num{"Format specifier count must match the argument count!"};
    FailFmt("%s", err_num);
    FailFmt("%s", err_num, "test", "extra");
```

Code after:
// Copyright (c) 2024-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <util/string.h>

#include <boost/test/unit_test.hpp>
#include <test/util/setup_common.h>

using namespace util;
using util::detail::CheckNumFormatSpecifiers;

BOOST_AUTO_TEST_SUITE(util_string_tests)

template <unsigned NumArgs>
void TfmFormatZeroes(const std::string& fmt)
{
    std::apply([&](auto... args) {
        (void)tfm::format(fmt, args...);
    }, std::array<int, NumArgs>{});
}

// Helper to allow compile-time sanity checks while providing the number of
// args directly. Normally PassFmt<sizeof...(Args)> would be used.
template <unsigned NumArgs>
void PassFmt(ConstevalFormatString<NumArgs> fmt)
{
    // Execute compile-time check again at run-time to get code coverage stats
    BOOST_CHECK_NO_THROW(CheckNumFormatSpecifiers<NumArgs>(fmt.fmt));

    // If ConstevalFormatString didn't throw above, make sure tinyformat doesn't
    // throw either for the same format string and parameter count combination.
    // Proves that we have some extent of protection from runtime errors
    // (tinyformat may still throw for some type mismatches).
    BOOST_CHECK_NO_THROW(TfmFormatZeroes<NumArgs>(fmt.fmt));
}
template <unsigned WrongNumArgs>
void FailFmtWithError(const char* wrong_fmt, std::string_view error)
{
    BOOST_CHECK_EXCEPTION(CheckNumFormatSpecifiers<WrongNumArgs>(wrong_fmt), const char*, HasReason{error});
}

BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
{
    PassFmt<0>("");
    PassFmt<0>("%%");
    PassFmt<1>("%s");