Introduce (mini) unit test framework #1734

The commit message says that the function is moved to testutil.h but it’s actually moved to util.h, which is included in the library. But we don’t want to add a dependency on headers like <time.h> in the real library (we currently call only malloc, free, printf, and abort and even these can be avoided if the user insists).

I think it’s perfectly fine to include testutil.h in the benchmarks if this is necessary to avoid code duplication.

The commit message says that the function is moved to testutil.h but it’s actually moved to util.h, which is included in the library. But we don’t want to add a dependency on headers like <time.h> in the real library (we currently call only malloc, free, printf, and abort and even these can be avoided if the user insists).

I think it’s perfectly fine to include testutil.h in the benchmarks if this is necessary to avoid code duplication.

Yeah, I initially did that but ended up moving it to util.h (without updating the commit message…) because including testutil.h alone in the bench.h file was not enough. I also needed to include testrand_impl.h to get access to the testrand implementation (which was blocking me from doing it because I wasn’t expecting to have to include implementation files manually at the binary top level file - it just felt odd), as well as time.h, and change cmake for the benchmark target to get access to the precomputed tables files.

But all good now, got some clarifications from Pieter. PR updated per feedback. Thanks!

Sorry, NACK this.

The (external) benchmark should link against the production-compiled library, not compile its own version.

done, solved. Introduced a new file tests_common.h, to hold general-purpose utility functions shared across test programs that do not depend on the internal library. Which helps avoid code duplication.

Note: we should probably consider renaming testutil.h to testutil_internals.h (or similar) to make it clear that it has access to library internals.

Is there a specific reason to overload the -target option? If not, could a different one be used instead?

Mainly because both are targets; the only diff is that one target runs more test cases than the other one. In general, it’s similar to how most unit test frameworks work. For example, on boost and gtest you can run groups of tests and single tests with the same command (--run_test and --gtest_filter respectively); the only diff with them is that they add the module name as a prefix when specifying single test case (which we could also do here).

a759cf5b9408ca630654e04faea56ff4f6944d45:

Include <stdint.h> for int64_t?

e262ba546a9d7b1e7a5977613efe36ff2c67b313:

I believe there’s no reason to keep everything in a single translation unit. Tests are not such a performance-critical task. And this pattern really confuses static analysis tools.

Let’s keep it as a separate module, for example, as done in this branch.

Cross-posting from https://github.com/hebasto/secp256k1/commit/082b928b7d16f83d8749a695f2a4b4e293e322a4#r165610965: @sipa wrote:

The design is there because of performance, to allow inlining across the layers.

That’s indeed unnecessary in tests, but that’s not a reason to deviate from the design; consistency is nice. @hebasto wrote: While consistency is indeed nice, I still see a reason for doing this, namely, more efficient use of static code analysis tools.

And this pattern really confuses static analysis tools. @furszy asked me offline to provide an example of such a tool in use.

Let’s take IWYU as an example and run it as follows:

0cmake -B build -DCMAKE_C_COMPILER=clang-19 -DCMAKE_C_INCLUDE_WHAT_YOU_USE=include-what-you-use
1cmake --build build -t tests

When run on the branch with a separate TU, it correctly identifies a missing header:

 0[ 60%] Building C object src/CMakeFiles/unit_test.dir/unit_test.c.o
 1Warning: include-what-you-use reported diagnostics:
 2
 3(/home/hebasto/dev/secp256k1/secp256k1/src/unit_test.h has correct #includes/fwd-decls)
 4
 5/home/hebasto/dev/secp256k1/secp256k1/src/unit_test.c should add these lines:
 6#include <stdint.h>         // for int64_t
 7
 8/home/hebasto/dev/secp256k1/secp256k1/src/unit_test.c should remove these lines:
 9
10The full include-list for /home/hebasto/dev/secp256k1/secp256k1/src/unit_test.c:
11#include "unit_test.h"
12#include <stdint.h>         // for int64_t
13#include <stdio.h>          // for fprintf, printf, NULL, stderr, perror
14#include <stdlib.h>         // for EXIT_FAILURE, strtol, EXIT_SUCCESS, getenv
15#include <string.h>         // for strcmp, strchr, strlen
16#include <sys/types.h>      // for pid_t
17#include <sys/wait.h>       // for waitpid
18#include <unistd.h>         // for close, _exit, fork, pipe, read, write
19#include "testrand_impl.h"  // for testrand_init
20#include "tests_common.h"   // for gettime_i64
21---

On this branch, unit_test.c is skipped during analysis (and I assume the same would happen with other tools).

e262ba546a9d7b1e7a5977613efe36ff2c67b313:

Why is this necessary?

nit, for consistency with other headers:

0#ifndef SECP256K1_TEST_COMMON_H

91eac9c9411bf27607ffc1e114fec039a7fca00a: a typo:

0#ifndef SECP256K1_TESTS_COMMON_H

I’m not sure about this implementation:

1. __has_include works fine with modern compilers. However, it is a C23 feature. Should we rely on it?

2. The availability of headers and symbols is usually a task for the build system, which defines SUPPORTS_CONCURRENCY based on the header / symbol checks.

I’m not sure about this implementation:

1. __has_include works fine with modern compilers. However, it is a C23 feature. Should we rely on it?
2. The availability of headers and symbols is usually a task for the build system, which defines SUPPORTS_CONCURRENCY based on the header / symbol checks.

Hmm, yeah. I was trying to avoid touching the build systems (since we have two now), but can move the code there too. The guards here are mainly to postpone Windows support for future work (if we ever want to add support for such platform).

318460c39aec04aa3580df5eadc7a935355968de

nit: It would be good to check return values of the pipe() and fork() calls consistently, for example ...==-1.

6162b7617d3731e08d2b59baf79ba602182cf85e:

It’s unfortunate to have such a pair of confusing macros. Perhaps their naming could be improved, although I don’t have a concrete suggestion.

6162b76:

It’s unfortunate to have such a pair of confusing macros. Perhaps their naming could be improved, although I don’t have a concrete suggestion.

A small scripted-diff would improve the situation and let us use only one of them. We just need to rename the test functions that start with “run_*” so that is not included in the name. I just tried to avoid expanding the scope of the PR further.

This always fails for me as written, but works when cache variables are used instead:

if test "x$ac_cv_header_sys_types_h" = xyes -a "x$ac_cv_header_sys_wait_h" = xyes -a "x$ac_cv_header_unistd_h" = xyes; then

0423532: weird, not able to use the environment variable option. I’m seeing:

• time ./build/bin/tests -iter=64 in 1m10.047s
• time SECP256K1_TEST_ITERS=64 ./build/bin/tests in 0m26.908s

0423532: weird, not able to use the environment variable option. I’m seeing:

• time ./build/bin/tests -iter=64 in 1m10.047s
• time SECP256K1_TEST_ITERS=64 ./build/bin/tests in 0m26.908s

Had it on my to do list and it seems I never enabled it (which kinda shows how much I like env vars..). Thanks!. Enabled now.

0423532: also don’t see the Iterations count = log in the CI. I guess this is related to the environment variable not being set.

maybe useful to always print the log even if it’s the default value being used/-iters isn’t used ?

0423532: also don’t see the Iterations count = log in the CI. I guess this is related to the environment variable not being set.

Yeah. We only print when the arg is provided by the user.

maybe useful to always print the log even if it’s the default value being used/-iters isn’t used ?

We should probably unify all args prints within a single place too.

0423532: is there any particular reason for separating the tests which don’t require context setup? many other tests in the other registry_modules like integer, scalar, field, group don’t require a context either.

These are just tests that must run before the RNG initialization, which itself happens before everything else, hence my “no context” naming, though that phrase has a different connotation here, agree. Could call it registry_no_rng? unless you have a better suggestion.

Nit: this fails when the number of targets is exactly MAX_ARGS (or greater) rather than strictly > MAX_ARGS, which I’d think is more natural.

The array has indices 0..MAX_ARGS-1, so when size == MAX_ARGS-1 we already have MAX_ARGS elements. Using only > would allow size == MAX_ARGS (which actually means having one more than MAX_ARGS), causing the next write to overflow the array before the error is thrown.

Sorry, wasn’t suggesting simply replacing >= with > there, but rather just pointing out that, eg, doing exactly 5 tests would fail if MAX_ARGS == 5, which seems unnatural.

Right now the code does the write before the guard. I’d switch to before. Something like:

0if (tf->args.targets.size >= MAX_ARGS) {
1      fprintf(stderr, ...);
2      return -1;
3}
4tf->args.targets.slots[tf->args.targets.size++] = i;

Nit: should it be sequential for 1 as well?

that was actually my testing trick to exercise the concurrent path with a single process. Removed now. Thanks.

There’s different exit code behavior between running concurrently and sequentially. Sequentially, the CHECK failure will call abort and kill the main process and return an error code. Concurrently, the abort and error code are printed but discarded and the parent returns EXIT_SUCCESS unconditionally. I think if jobs were ever auto-detected (or anyone ever relied on exit codes for the concurrent path), this would result in a test harness incorrectly reporting success even on failure.

Maybe collect the exit codes in this loop and return failure if any fail?

There’s different exit code behavior between running concurrently and sequentially. Sequentially, the CHECK failure will call abort and kill the main process and return an error code. Concurrently, the abort and error code are printed but discarded and the parent returns EXIT_SUCCESS unconditionally. I think if jobs were ever auto-detected (or anyone ever relied on exit codes for the concurrent path), this would result in a test harness incorrectly reporting success even on failure.

Maybe collect the exit codes in this loop and return failure if any fail?

Hmm yeah, good catch. This is something I introduced during the run_concurrent decoupling. The code actually collects the exit values in the status variable but misses to return it. Fixed now. Thanks!

Same issue here with the MAX_ARGS check. I’d also move the guard to before the write. Sorry for the piecemeal comments!

Don’t worry, keep them coming :). Updated per feedback.

Is there a reason for not using the GNU standard of using -- for multi character options?

Not a fully rational reason, just way too many years working on Core. Happy to switch to the double-dash style to follow the GNU convention.

Is there any good reason for not just failing here? Allowing unknown options for command-line tools seems to be a very unusual practice in general (I think I’ve only seen that in the slightly different context of config file parsing for forward-compatibility).

The idea is to support downgrades and cross-version testing without forcing changes to automation/people scripts. It gives us more flexibility to change options in the future without breaking the existing setups.

That being said, if there’s a strong preference to fail here, could change it.

Personally I will very likely never have an use-case for running tests on past versions (this IMHO makes much more sense for benchmarks).

I usually compare testing times across versions too. Caught a few regressions in that way.

If I’ve grepped the codebase correctly, this will be the only external definition used as #if DEFINITION.

The standard mandates that undefined macros evaluate to 0 in an #if.

Nevertheless, this approach has proven fragile. I would kindly ask to reconsider the design and use #if defined(DEFINITION) instead, which would also require slight adjustment in both build systems.

The standard mandates that undefined macros evaluate to 0 in an #if.

Nevertheless, this approach has proven fragile. I would kindly ask to reconsider the design and use #if defined(DEFINITION) instead, which would also require slight adjustment in both build systems.

Happy to change it either way, but let’s try to reach a consensus first.

0 * The caller must set 'registry_modules' and 'num_modules' before calling.

(guess these fields origin from an earlier iteration of the PR and were then renamed at some point)

meta-nit: seeing this fancy single-character arrow made me wonder if this file is UTF-8 encoded, and according to the file utility it indeed is:

0$ file ./src/unit_test.c
1./src/unit_test.c: C source, Unicode text, UTF-8 text

Seems that other source files in the repo are not (see $ file ./src/*.h ./src/*.c, resulting all in “C source, ASCII text”). I could imagine that in theory really old compilers could struggle with that and using ASCII encoding would be the safe choice, but not sure if that concern is practically relevant at all.

It’s C89, but hey, at least we got emojis in the source code! :rofl:

Serious: I don’t think it’s a problem in practice but it can’t be wrong to stick to ASCII for consistency.

nit:

0        fputs("Parallel execution not supported on your system. Running sequentially...\n", stderr);

The CheckIncludeFile module (in singular) appears more suitable:

0  include(CheckIncludeFile)
1  check_include_file(sys/types.h HAVE_SYS_TYPES_H)
2  check_include_file(sys/wait.h HAVE_SYS_WAIT_H)
3  check_include_file(unistd.h HAVE_UNISTD_H)

It may sound like overkill, but we’re actually interested in the symbols provided by headers, not the headers themselves. For example, when cross-compiling for Windows, unistd.h is available, but fork() is not.

So something like the following would be more robust:

0  include(CheckSymbolExists)
1  check_symbol_exists(waitpid "sys/wait.h" HAVE_WAITPID)
2  check_symbol_exists(fork "unistd.h" HAVE_FORK)
3  ...

Did you mark this “resolved” unintentionally?

I pushed hebasto’s initial CheckIncludeFile suggestion after discussing the CheckSymbolExists one via DM, and forgot to update this thread (sorry for that). It just seemed to be an overkill for a feature we might introduce in a follow-up. Adding Windows support is not hard, it is just a bit more code.

Adding Windows support is not hard, it is just a bit more code.

My apologies if I was misunderstood. I mentioned cross-compiling for Windows only as an example, not as the basis of my comment. I do believe the build system should check the actual functionality we intend to use, which is the functions, rather than just the headers.

Here are a couple of examples for the fork() function from Bitcoin Core:

• in Autotools:

0AC_CHECK_DECLS([fork])

• in CMake:

0check_cxx_symbol_exists(fork "unistd.h" HAVE_DECL_FORK)

I’m fine with leaving this improvement for a follow-up PR.

Following the overkill rationale; If we go with a more granular approach, we would have to check for every relevant symbol?. E.g. pid_t, write, read, and close too. The reason I included <sys/types.h> (in addition to <unistd.h>) is that historically, pid_t has sometimes been declared in one header vs. the other, depending on platform and era.

In any case, I just unsolved this thread so we don’t forget it.

Following the overkill rationale; If we go with a more granular approach, we would have to check for every relevant symbol?. E.g. pid_t, write, read, and close too.

I think a pragmatic middle ground is to check for symbols where we know that some common platforms don’t have them, e.g., fork on Windows. Something like pid_t never needs to be checked because it’s in the signature of fork.

const-correctness nit

0        const TestRef* index = &tf->args.targets.slots[it];

“as early as possible in the program.”

That seems imprecise. What’s the reason for this requirement?

“as early as possible in the program.”

That seems imprecise. What’s the reason for this requirement?

It disables stdout and stderr buffering (the setbuf() calls).

style nit: These struct names should be lowercase and snake_case for consistency with the rest of the code base.

My concern with that is that it’s easy for struct names to clash with variable names using that style. In the rest of the codebase, this isn’t a problem because we prefix structs with secp256k1, but here that would mean more boilerplate without much gain?. I could add the prefix + change them to lowercase if you feel strongly about it. I’m just thinking out loud before entering a large rebase.

My concern with that is that it’s easy for struct names to clash with variable names using that style.

Hehe, technically, the struct tags are in different namespaces than variables. :) But I see the point, of course.

In the rest of the codebase, this isn’t a problem because we prefix structs with secp256k1, but here that would mean more boilerplate without much gain?.

Perhaps we could just prefix it just with tf_ like the tf functions? I think this would actually add clarity to it. If you’re concerned about boilerplate, you could also typedef the struct away. I think we do this for (almost?) all struct defs in the code base, but it’s certainly not a requirement.

I could add the prefix + change them to lowercase if you feel strongly about it. I’m just thinking out loud before entering a large rebase.

I have to admit that I feel a bit strongly about the style convention, but I think it’s totally reasonable to add a commit on top of the current PR instead of needing to go through all the commits and edit them.

nit: This is above the “Public API” sectioning comment, but this struct seems to be part of the public API,

That’s because I was initially thinking of adding a factory function that returns an initialized TestFramework, but it ended up feeling like extra boilerplate with no real benefit. Will move it.

micronit:

0#define MAKE_TEST_MODULE(name) { \

Actually, below in REPEAT_TEST_MULT, there’s a different style. I think the “one space” style is more common in this code base.

nit: This is totally fine to have the year and your name here if you want to keep it, but we recently came to the conclusion that simply omitting the “Copyright (c)” line for new files makes maintenance easier (see the ellswift module for example).

Rationale: Adding the year and name is legally not required to obtain the copyright. It’s merely a hint on who has the copyright and when it will expire, but we have git for this. Updating the date is annoying, so no one does it in practice. And it’s never entirely clear when to update the name list, so no one does it in practice. So after a few years, this line will probably be outdated.

Perhaps you missed this one because it’s hidden by github. If you ignored it intentionally, nevermind, it’s totally not a blocker.

Yeah, I was going to reply you and never did. Thanks for the reminder.

It would be nice to support the GNU style print-tests here as well now that we support --. (Not sure if it must be documented but I guess it won’t hurt.)

The change is not really big but have thought about this for a while, and I’d rather keep - reserved only for introducing new command-line options. Mainly because allowing dashes inside names blurs that boundary and makes syntax less clear. Using underscores keeps it simple and consistent: “dashes always mean new option, underscores always separate words”. If you feel strongly about adding it, we could but it feels a bit messy to me.

The change is not really big but have thought about this for a while, and I’d rather keep - reserved only for introducing new command-line options. Mainly because allowing dashes inside names blurs that boundary and makes syntax less clear.

Agreed, let’s avoid everything that makes parsing more complicated.

feel-free-to-ignore yocto-nit: wouldn’t this currently only be a one-character change?

 0diff --git a/src/unit_test.c b/src/unit_test.c
 1index a1858a1..715fb60 100644
 2--- a/src/unit_test.c
 3+++ b/src/unit_test.c
 4@@ -239,7 +239,7 @@ static int read_args(int argc, char** argv, int start, struct tf_framework* tf)
 5                 tf->args.help = 1;
 6                 return 0;
 7             }
 8-            if (strcmp(key, "l") == 0 || strcmp(key, "list_tests") == 0) {
 9+            if (strcmp(key, "l") == 0 || strcmp(key, "list-tests") == 0) {
10                 tf->args.list_tests = 1;
11                 return 0;
12             }

seems to work just fine. Would in doubt slightly prefer to adhere to widely accepted standards (in Bitcoin Core we also use dashes in parameter names, fwiw). I guess it’s not worth it block the PR at all, but if maintainers and other reviewers feel strongly about dash-vs-underscores, could also sidestep the topic by simply renaming to “–list” :stuck_out_tongue:

If I pass -- as arg to this function, then key[1] == arg[3] and this is beyond the end of the string, so this is UB.

(On my machine, I get “unknown error” as the error message. I think having this as a fallback is not good. It simply hides coding errors, and it would be better to assert that err_msg is set when an error is indicated via the return value.)

If I pass – as arg to this function, then key[1] == arg[3] and this is beyond the end of the string, so this is UB.

Nice catch. Fixed.

(On my machine, I get “unknown error” as the error message. I think having this as a fallback is not good. It simply hides coding errors, and it would be better to assert that err_msg is set when an error is indicated via the return value.)

You know, I actually avoided using runtime assertions because I didn’t find any throughout the library.

You know, I actually avoided using runtime assertions because I didn’t find any throughout the library.

We don’t have it in the library itself, but we obviously have CHECK in the test-only code, so you could use that one. Even on current master, it’s used in tests.c in the setup code for purposes different from testing the library itself, e.g., this line: https://github.com/bitcoin-core/secp256k1/blob/baa265429fa8f1686138380e52a75c25b0344719/src/tests.c#L7699-L7700

Been thinking about this. If we use CHECK, we lose the arg key-value error logging. Which will make debugging slightly harder (we will know where it failed but not under which values). We probably could extend CHECK to also print a message crafted at runtime when provided.

Maybe we can leave this for a follow-up? I’m unsure we should keep expanding this PR further.