Introduce (mini) unit test framework

furszy commented at 5:12 pm on September 4, 2025: member

Early Note: Don’t be scared by the PR’s line changes count — most of it’s just doc or part of the test framework API.

Context: Currently, all tests run single-threaded sequentially and the library lacks the ability to specify which test (or group of tests) you would like to run. This is not only inconvenient as more tests are added but also time consuming during development and affects downstream projects that may want to parallelize the workload (such as Bitcoin-Core CI).

PR Goal: Introduce a lightweight, extensible C89 unit test framework with no dynamic memory allocations, providing a structured way to register, execute, and report tests. The framework supports named command-line arguments in -key=value form, parallel test execution across multiple worker processes, granular test selection (selecting tests either by name or by module name), and time accumulation reports.

The introduced framework supports:

-help or -h: display list of available commands along with their descriptions.
-jobs=<num>: distribute tests across multiple worker processes (default: sequential if 0).
-target=<name> or -t=<name>: run only specific tests by name; can be repeated to select multiple tests.
-target=<module name>, -t=<module> Run all tests within a specific module (can be provided multiple times)
-seed=<hex>: set a specific RNG seed (defaults to random if unspecified).
-iter=<n>: specify the number of iterations.
-print_tests: display list of available tests and modules you can run.
-log=<0|1>: enable or disable test execution logging (default: 0 = disabled).

Beyond these features, the idea is to also make future developments smoother, as adding new tests require only a single entry in the central test registry, and new command-line options can be introduced easily by extending the framework’s parse_arg() function.

Compatibility Note: The framework continues accepting the two positional arguments previously supported (iterations and seed), ensuring existing workflows remain intact.

Testing Notes: Have fun. You can quickly try it through ./tests -j=<workers_num> for parallel execution or ./tests -t=<test_name> to run a specific test (call ./tests -print_tests to display all available tests and modules).

Extra Note: I haven’t checked the exhaustive tests file so far, but I will soon. For now, this only runs all tests declared in the tests binary.

Testing Results: (Current master branch vs PR in seconds)

Raspberry Pi 5: master ~100 s → PR ~38 s (5 jobs)
MacBook Pro M1: master ~30 s → PR ~10 s (6 jobs)

furszy renamed this:
~~Introduce (mini) unit test framework~~
WIP: Introduce (mini) unit test framework
on Sep 4, 2025

furszy force-pushed on Sep 4, 2025

furszy force-pushed on Sep 5, 2025

real-or-random added the label assurance on Sep 5, 2025

real-or-random added the label feature on Sep 5, 2025

real-or-random commented at 6:36 am on September 5, 2025: contributor

Nice!

I looked at existing unit test frameworks in the past, but nothing seemed appropriate for us. There are not that many for C, and they were either overkill or too simple (just handful of ifdefs) so they didn’t add any functionality. I thought writing our own is too annoying (or I was just lazy). But the framework is ~300 lines, that seems fine to me.

in src/util.h:1 in 8a3b9d3141 outdated

real-or-random commented at 6:40 am on September 5, 2025:

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.

furszy commented at 5:24 pm on September 5, 2025:

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!

furszy commented at 8:19 pm on September 9, 2025:

just in case; connecting this comment to #1734 (review) for the rationale behind the latest change.

real-or-random commented at 8:34 am on September 5, 2025: contributor

see also #1211

furszy force-pushed on Sep 5, 2025

furszy force-pushed on Sep 6, 2025

jonasnick commented at 3:21 pm on September 7, 2025: contributor

Thanks @furszy. I played around with this a little bit. It reduces the execution time on my machine from 26 seconds to 10 seconds (-jobs=16). Very nice! Some observations:

It would be helpful if a helptext would be output when tests is run with -h or --help.
I think showing all the tests that have passed is a bit overkill. I’m already assuming that the tests pass if they do not show up in the output. I only need to see tests that don’t pass.
Maybe a future PR can autodetect the number of cores and set -jobs automatically by default?
There’s an -iter command line flag, but the test output shows “test count”. It would be better if we were consistent.

jonasnick commented at 3:29 pm on September 7, 2025: contributor

The available “targets” seem to be a bit arbitary. Maybe we can try to put them into groups (perhaps similar to the [grouping in #1211](https://github.com/bitcoin-core/secp256k1/pull/1211/files#diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8R7495))?

 0./tests -print_tests
 1Available tests (58):
 2    --------------------------------------------------
 3    [  1] selftest_tests
 4    [  2] all_proper_context_tests
 5    [  3] all_static_context_tests
 6    [  4] deprecated_context_flags_test
 7    [  5] scratch_tests
 8    [  6] int128_tests
 9    [  7] ctz_tests
10    [  8] modinv_tests
11    [  9] inverse_tests
12    [ 10] hsort_tests
13    [ 11] sha256_known_output_tests
14    [ 12] sha256_counter_tests
15    [ 13] hmac_sha256_tests
16    [ 14] rfc6979_hmac_sha256_tests
17    [ 15] tagged_sha256_tests
18    [ 16] scalar_tests
19    [ 17] field_half
20    [ 18] field_misc
21    [ 19] field_convert
22    [ 20] field_be32_overflow
23    [ 21] fe_mul
24    [ 22] sqr
25    [ 23] sqrt
26    [ 24] ge
27    [ 25] gej
28    [ 26] group_decompress
29    [ 27] ecmult_pre_g
30    [ 28] wnaf
31    [ 29] point_times_order
32    [ 30] ecmult_near_split_bound
33    [ 31] ecmult_chain
34    [ 32] ecmult_constants
35    [ 33] ecmult_gen_blind
36    [ 34] ecmult_const_tests
37    [ 35] ecmult_multi_tests
38    [ 36] ec_combine
39    [ 37] endomorphism_tests
40    [ 38] ec_pubkey_parse_test
41    [ 39] eckey_edge_case_test
42    [ 40] eckey_negate_test
43    [ 41] ecdh_tests
44    [ 42] ec_illegal_argument_tests
45    [ 43] pubkey_comparison
46    [ 44] pubkey_sort
47    [ 45] random_pubkeys
48    [ 46] ecdsa_der_parse
49    [ 47] ecdsa_sign_verify
50    [ 48] ecdsa_end_to_end
51    [ 49] ecdsa_edge_cases
52    [ 50] ecdsa_wycheproof
53    [ 51] extrakeys_tests
54    [ 52] schnorrsig_tests
55    [ 53] musig_tests
56    [ 54] ellswift_tests
57    [ 55] secp256k1_memczero_test
58    [ 56] secp256k1_is_zero_array_test
59    [ 57] secp256k1_byteorder_tests
60    [ 58] cmov_tests

furszy force-pushed on Sep 8, 2025

furszy commented at 2:10 pm on September 9, 2025: member

Thanks for the review jonasnick!

Thanks @furszy. I played around with this a little bit. It reduces the execution time on my machine from 26 seconds to 10 seconds (-jobs=16). Very nice!

Awesome :). I think we can actually do even better, will do some changes.

It would be helpful if a helptext would be output when tests is run with -h or --help.

The help message was actually already there, but for -help only (with a single - and I forgot to add it to the PR description). I just pushed support for -h as well.

I think showing all the tests that have passed is a bit overkill. I’m already assuming that the tests pass if they do not show up in the output. I only need to see tests that don’t pass.

Sure. Will hide the logging behind a -log option (or -silent if we want the opposite behavior). From my experience, logging sometimes helps spot regressions or areas that can be improved (like when a test suddenly takes longer than usual), and it’s also reassuring to see them run and pass when you have a large number of them. I’ve also been thinking about adding per-test execution time loggings, might be a good opportunity to include it too.

Maybe a future PR can autodetect the number of cores and set -jobs automatically by default?

I’m not sure we want that. Sequential execution is usually “standard” on any system because we don’t know what else the user might be running. Picking a number of parallel tasks automatically (even if it is a low number) could hang the CPU or even make it run slower than sequential if the system is overloaded.

There’s an -iter command line flag, but the test output shows “test count”. It would be better if we were consistent.

Sure 👍🏼. That was carried over from the previous code; will improve it.

furszy commented at 2:21 pm on September 9, 2025: member

The available “targets” seem to be a bit arbitary. Maybe we can try to put them into groups (perhaps similar to the [grouping in #1211](https://github.com/bitcoin-core/secp256k1/pull/1211/files#diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8R7495))?

Yeah. Just reworked the framework to support registering and running groups of tests in a generic manner. This means we can now run specific tests and/or specific groups of tests via the -target/-t arg.

On top of that, made the framework reusable across binaries and improved the overall API (we can now easily connect the tests_exhaustive binary to it too — probably something for a follow-up), along with improvements to the consumers’ structure, enforcing consistency and a specific pattern that all consumers will follow.

Other than that, the -print_tests option was improved to display the available modules and tests.

A simple usage example: ./tests -print_tests → pick any module (like field) and run: ./tests -t=field You can also combine this with -j=<num_workers>, plus specify multiple targets as needed.

furszy force-pushed on Sep 9, 2025

in src/bench.c:12 in 4900aee07d outdated

 6@@ -7,9 +7,13 @@
 7 #include <stdio.h>
 8 #include <stdlib.h>
 9 #include <string.h>
10+#include <time.h>
11 
12+#include "secp256k1.c"

sipa commented at 3:58 pm on September 9, 2025:

Sorry, NACK this.

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

furszy commented at 4:31 pm on September 9, 2025:

np, that’s from the first move-only commit that is merely there to gain access to the existing gettime() function from the test framework. Worst-case scenario I can dedup the code (or create an util_time.h file). Will see how to adapt it. I remember that this had some extra complications because including testutil.h requires secp256k1_pubkey_save which only exists on secp256k1.c. But will check it again, I got more insights of the library now.

furszy commented at 7:51 pm on September 9, 2025:

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.

furszy force-pushed on Sep 9, 2025

furszy renamed this:
~~WIP: Introduce (mini) unit test framework~~
Introduce (mini) unit test framework
on Sep 9, 2025

hebasto commented at 1:49 pm on September 10, 2025: member

Context: Currently, all tests run single-threaded sequentially and the library lacks the ability to specify which test (or group of tests) you would like to run. This is not only inconvenient as more tests are added but also time consuming during development and affects downstream projects that may want to parallelize the workload (such as Bitcoin-Core CI).

I agree with this assessment.

For now, I slightly prefer #1211 for the following reasons:

smaller diff;
adding a new test does not seem convoluted in that branch;
it scales reasonably well (though not perfectly).

In general, I believe parallelism management could (and I believe it should) be offloaded from the C code to the build system. Moreover, CTest provides a range of options to customize test execution and output. This functionality could of course be ported to the Autotools-based build system, but I am not sure it would be worth the effort.

furszy commented at 2:33 pm on September 10, 2025: member

smaller diff;

I wouldn’t consider the larger diff a complication. The size comes from reorganizing tests under a proper framework and introducing features like granular test/group selection and a named command-line options. In my view, it should be seen as a one-off restructuring that makes testing scalable and smoother for development, and it sets us up much better long term.

adding a new test does not seem convoluted in that branch;

Adding new tests is not hard but granularly selecting them to run in isolation is not possible. You will continue be forced to comment code from a long main function if you just want to run a single test or a mix of single tests from different groups.

it scales reasonably well (though not perfectly).

Have you tested how fast this one gets if you parallelize it in the same way you did for #1211? This one should go faster because we can select and run each individual test on a different process (in #1211 you can only run groups of tests).

In general, I believe parallelism management could (and I believe it should) be offloaded from the C code to the build system. Moreover, CTest provides a range of options to customize test execution and output. This functionality could of course be ported to the Autotools-based build system, but I am not sure it would be worth the effort.

It could be added here for CI too (I would be very interested to see your results), but I don’t think ctest is very useful for daily development. It adds another layer of complexity; for example, you can’t easily pass command-line arguments to the binaries you’re running (as far as I have seen, it requires you to enable the “script mode” and even that is not enough). I think both approaches can coexist as long as the maintenance burden stays minimal.

in src/unit_test.c:86 in aa5f041f2f outdated

85+    printf("    -print_tests                        Display list of all available tests and modules\n");
86+    printf("    -j=<num>, -jobs=<num>               Number of parallel worker processes (default: 0 = sequential)\n");
87+    printf("    -iter=<num>, -iterations=<num>      Number of iterations for each test (default: 16)\n");
88+    printf("    -seed=<hex>                         Set a specific RNG seed (default: random)\n");
89+    printf("    -target=<test name>, -t=<name>      Run a specific test (can be provided multiple times)\n");
90+    printf("    -target=<module name>, -t=<module>  Run all tests within a specific module (can be provided multiple times)\n");

hebasto commented at 2:54 pm on September 10, 2025:

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

furszy commented at 3:13 pm on September 10, 2025:

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).

hebasto commented at 5:31 pm on September 10, 2025:

Right. Let’s keep it this way. It works nicely with CMake code. See https://github.com/hebasto/secp256k1/commit/3ff3105edc5e78aaceae0dddba5f788ec83a163d.

furszy commented at 2:21 pm on September 11, 2025:

nice!

hebasto commented at 3:13 pm on September 10, 2025: member

In case of a disable module, could the output be made more explanatory instead of this:

0$ ./build/bin/tests -target=recovery
1Error: target not found: 'recovery'

For example (from #1211): https://github.com/bitcoin-core/secp256k1/blob/3e404f2b8643493854e664389dbc7817763ad4ac/src/tests.c#L7475-L7482

furszy commented at 3:19 pm on September 10, 2025: member

In case of a disable module, could the output be made more explanatory instead of this:
0$ ./build/bin/tests -target=recovery
1Error: target not found: 'recovery'

Yeah sure.

hebasto commented at 4:09 pm on September 10, 2025: member

~~Removed~~

nm

hebasto commented at 4:24 pm on September 10, 2025: member

Have you tested how fast this one gets if you parallelize it in the same way you did for #1211? This one should go faster because we can select and run each individual test on a different process (in #1211 you can only run groups of tests).

This branch with splitting by modules:

 0$ ctest --test-dir build -j 16 --output-on-failure
 1Test project /home/hebasto/dev/secp256k1/secp256k1/build
 2      Start  1: secp256k1_tests::general
 3      Start  2: secp256k1_tests::integer
 4      Start  3: secp256k1_tests::hash
 5      Start  4: secp256k1_tests::scalar
 6      Start  5: secp256k1_tests::field
 7      Start  6: secp256k1_tests::group
 8      Start  7: secp256k1_tests::ecmult
 9      Start  8: secp256k1_tests::ec
10      Start  9: secp256k1_tests::ecdh
11      Start 10: secp256k1_tests::ecdsa
12      Start 11: secp256k1_tests::recovery
13      Start 12: secp256k1_tests::extrakeys
14      Start 13: secp256k1_tests::schnorrsig
15      Start 14: secp256k1_tests::musig
16      Start 15: secp256k1_tests::ellswift
17      Start 16: secp256k1_tests::utils
18 1/33 Test  [#1](/bitcoin-core-secp256k1/1/): secp256k1_tests::general ...............   Passed    0.01 sec
19 2/33 Test [#11](/bitcoin-core-secp256k1/11/): secp256k1_tests::recovery ..............***Skipped   0.00 sec
20      Start 17: secp256k1_noverify_tests::general
21      Start 18: secp256k1_noverify_tests::integer
22 3/33 Test [#16](/bitcoin-core-secp256k1/16/): secp256k1_tests::utils .................   Passed    0.00 sec
23 4/33 Test [#17](/bitcoin-core-secp256k1/17/): secp256k1_noverify_tests::general ......   Passed    0.00 sec
24      Start 19: secp256k1_noverify_tests::hash
25      Start 20: secp256k1_noverify_tests::scalar
26 5/33 Test  [#3](/bitcoin-core-secp256k1/3/): secp256k1_tests::hash ..................   Passed    0.02 sec
27      Start 21: secp256k1_noverify_tests::field
28 6/33 Test  [#8](/bitcoin-core-secp256k1/8/): secp256k1_tests::ec ....................   Passed    0.03 sec
29      Start 22: secp256k1_noverify_tests::group
30 7/33 Test [#12](/bitcoin-core-secp256k1/12/): secp256k1_tests::extrakeys .............   Passed    0.03 sec
31 8/33 Test [#19](/bitcoin-core-secp256k1/19/): secp256k1_noverify_tests::hash .........   Passed    0.02 sec
32      Start 23: secp256k1_noverify_tests::ecmult
33      Start 24: secp256k1_noverify_tests::ec
34 9/33 Test [#20](/bitcoin-core-secp256k1/20/): secp256k1_noverify_tests::scalar .......   Passed    0.03 sec
35      Start 25: secp256k1_noverify_tests::ecdh
3610/33 Test [#24](/bitcoin-core-secp256k1/24/): secp256k1_noverify_tests::ec ...........   Passed    0.02 sec
37      Start 26: secp256k1_noverify_tests::ecdsa
3811/33 Test  [#4](/bitcoin-core-secp256k1/4/): secp256k1_tests::scalar ................   Passed    0.05 sec
39      Start 27: secp256k1_noverify_tests::recovery
4012/33 Test [#27](/bitcoin-core-secp256k1/27/): secp256k1_noverify_tests::recovery .....***Skipped   0.00 sec
41      Start 28: secp256k1_noverify_tests::extrakeys
4213/33 Test [#28](/bitcoin-core-secp256k1/28/): secp256k1_noverify_tests::extrakeys ....   Passed    0.01 sec
43      Start 29: secp256k1_noverify_tests::schnorrsig
4414/33 Test [#13](/bitcoin-core-secp256k1/13/): secp256k1_tests::schnorrsig ............   Passed    0.07 sec
45      Start 30: secp256k1_noverify_tests::musig
4615/33 Test [#25](/bitcoin-core-secp256k1/25/): secp256k1_noverify_tests::ecdh .........   Passed    0.05 sec
47      Start 31: secp256k1_noverify_tests::ellswift
4816/33 Test [#29](/bitcoin-core-secp256k1/29/): secp256k1_noverify_tests::schnorrsig ...   Passed    0.03 sec
49      Start 32: secp256k1_noverify_tests::utils
5017/33 Test [#32](/bitcoin-core-secp256k1/32/): secp256k1_noverify_tests::utils ........   Passed    0.00 sec
51      Start 33: secp256k1_exhaustive_tests
5218/33 Test  [#9](/bitcoin-core-secp256k1/9/): secp256k1_tests::ecdh ..................   Passed    0.13 sec
5319/33 Test [#30](/bitcoin-core-secp256k1/30/): secp256k1_noverify_tests::musig ........   Passed    0.11 sec
5420/33 Test [#14](/bitcoin-core-secp256k1/14/): secp256k1_tests::musig .................   Passed    0.29 sec
5521/33 Test [#26](/bitcoin-core-secp256k1/26/): secp256k1_noverify_tests::ecdsa ........   Passed    0.98 sec
5622/33 Test  [#5](/bitcoin-core-secp256k1/5/): secp256k1_tests::field .................   Passed    1.24 sec
5723/33 Test [#22](/bitcoin-core-secp256k1/22/): secp256k1_noverify_tests::group ........   Passed    1.71 sec
5824/33 Test [#10](/bitcoin-core-secp256k1/10/): secp256k1_tests::ecdsa .................   Passed    1.95 sec
5925/33 Test [#21](/bitcoin-core-secp256k1/21/): secp256k1_noverify_tests::field ........   Passed    1.94 sec
6026/33 Test  [#2](/bitcoin-core-secp256k1/2/): secp256k1_tests::integer ...............   Passed    2.47 sec
6127/33 Test [#18](/bitcoin-core-secp256k1/18/): secp256k1_noverify_tests::integer ......   Passed    2.47 sec
6228/33 Test  [#6](/bitcoin-core-secp256k1/6/): secp256k1_tests::group .................   Passed    4.24 sec
6329/33 Test [#31](/bitcoin-core-secp256k1/31/): secp256k1_noverify_tests::ellswift .....   Passed    4.32 sec
6430/33 Test [#33](/bitcoin-core-secp256k1/33/): secp256k1_exhaustive_tests .............   Passed    7.87 sec
6531/33 Test [#23](/bitcoin-core-secp256k1/23/): secp256k1_noverify_tests::ecmult .......   Passed    8.77 sec
6632/33 Test [#15](/bitcoin-core-secp256k1/15/): secp256k1_tests::ellswift ..............   Passed   13.11 sec
6733/33 Test  [#7](/bitcoin-core-secp256k1/7/): secp256k1_tests::ecmult ................   Passed   17.27 sec
68
69100% tests passed, 0 tests failed out of 33
70
71Total Test time (real) =  17.31 sec
72
73The following tests did not run:
74	 11 - secp256k1_tests::recovery (Skipped)
75	 27 - secp256k1_noverify_tests::recovery (Skipped)

UPD. Even after splitting up the ecmult tests, the ellswift tests are still the longest.

furszy commented at 4:34 pm on September 10, 2025: member

This branch with splitting by modules

Nice thanks, so this doesn’t introduce any overhead compared to #1211 when run by modules (same functionality in both places). That’s good to know.

Now, here you can go faster by parallelizing by test cases instead of modules. Can run ./tests -print_tests to see all available individual cases.

UPD. Even after splitting up the ecmult tests, the ellswift tests are still the longest.

I left a todo comment about that. We can subdivide the ellswift tests (there is a single function atm) and they will be automatically parallelizable here too.

hebasto commented at 5:04 pm on September 10, 2025: member

Now, here you can go faster by parallelizing by test cases instead of modules. Can run ./tests -print_tests to see all available individual cases.

Yes, but that output can be quite noisy, especially when running in downstream projects such as Bitcoin Core.

That said, combining both approaches should work.

hebasto commented at 5:28 pm on September 10, 2025: member

Now, here you can go faster by parallelizing by test cases instead of modules. Can run ./tests -print_tests to see all available individual cases.

Yes, but that output can be quite noisy, especially when running in downstream projects such as Bitcoin Core.

That said, combining both approaches should work.

For example, for this branch:

 0$ ctest --test-dir build -j 16 --output-on-failure
 1Test project /home/hebasto/dev/secp256k1/secp256k1/build
 2      Start  1: secp256k1_tests::ecmult_constants
 3      Start  2: secp256k1_tests::ecmult
 4      Start  3: secp256k1_tests::general
 5      Start  4: secp256k1_tests::integer
 6      Start  5: secp256k1_tests::hash
 7      Start  6: secp256k1_tests::scalar
 8      Start  7: secp256k1_tests::field
 9      Start  8: secp256k1_tests::group
10      Start  9: secp256k1_tests::ec
11      Start 10: secp256k1_tests::ecdh
12      Start 11: secp256k1_tests::ecdsa
13      Start 12: secp256k1_tests::recovery
14      Start 13: secp256k1_tests::extrakeys
15      Start 14: secp256k1_tests::schnorrsig
16      Start 15: secp256k1_tests::musig
17      Start 16: secp256k1_tests::ellswift
18 1/35 Test  [#3](/bitcoin-core-secp256k1/3/): secp256k1_tests::general .....................   Passed    0.01 sec
19 2/35 Test [#12](/bitcoin-core-secp256k1/12/): secp256k1_tests::recovery ....................***Skipped   0.00 sec
20      Start 17: secp256k1_tests::utils
21      Start 18: secp256k1_noverify_tests::ecmult_constants
22 3/35 Test [#17](/bitcoin-core-secp256k1/17/): secp256k1_tests::utils .......................   Passed    0.00 sec
23      Start 19: secp256k1_noverify_tests::ecmult
24 4/35 Test  [#5](/bitcoin-core-secp256k1/5/): secp256k1_tests::hash ........................   Passed    0.02 sec
25      Start 20: secp256k1_noverify_tests::general
26 5/35 Test [#20](/bitcoin-core-secp256k1/20/): secp256k1_noverify_tests::general ............   Passed    0.00 sec
27      Start 21: secp256k1_noverify_tests::integer
28 6/35 Test [#13](/bitcoin-core-secp256k1/13/): secp256k1_tests::extrakeys ...................   Passed    0.03 sec
29      Start 22: secp256k1_noverify_tests::hash
30 7/35 Test  [#9](/bitcoin-core-secp256k1/9/): secp256k1_tests::ec ..........................   Passed    0.03 sec
31      Start 23: secp256k1_noverify_tests::scalar
32 8/35 Test  [#6](/bitcoin-core-secp256k1/6/): secp256k1_tests::scalar ......................   Passed    0.05 sec
33      Start 24: secp256k1_noverify_tests::field
34 9/35 Test [#22](/bitcoin-core-secp256k1/22/): secp256k1_noverify_tests::hash ...............   Passed    0.02 sec
35      Start 25: secp256k1_noverify_tests::group
3610/35 Test [#23](/bitcoin-core-secp256k1/23/): secp256k1_noverify_tests::scalar .............   Passed    0.03 sec
37      Start 26: secp256k1_noverify_tests::ec
3811/35 Test [#26](/bitcoin-core-secp256k1/26/): secp256k1_noverify_tests::ec .................   Passed    0.01 sec
39      Start 27: secp256k1_noverify_tests::ecdh
4012/35 Test [#14](/bitcoin-core-secp256k1/14/): secp256k1_tests::schnorrsig ..................   Passed    0.07 sec
41      Start 28: secp256k1_noverify_tests::ecdsa
4213/35 Test [#27](/bitcoin-core-secp256k1/27/): secp256k1_noverify_tests::ecdh ...............   Passed    0.05 sec
43      Start 29: secp256k1_noverify_tests::recovery
4414/35 Test [#29](/bitcoin-core-secp256k1/29/): secp256k1_noverify_tests::recovery ...........***Skipped   0.00 sec
45      Start 30: secp256k1_noverify_tests::extrakeys
4615/35 Test [#30](/bitcoin-core-secp256k1/30/): secp256k1_noverify_tests::extrakeys ..........   Passed    0.01 sec
47      Start 31: secp256k1_noverify_tests::schnorrsig
4816/35 Test [#10](/bitcoin-core-secp256k1/10/): secp256k1_tests::ecdh ........................   Passed    0.14 sec
49      Start 32: secp256k1_noverify_tests::musig
5017/35 Test [#31](/bitcoin-core-secp256k1/31/): secp256k1_noverify_tests::schnorrsig .........   Passed    0.03 sec
51      Start 33: secp256k1_noverify_tests::ellswift
5218/35 Test [#32](/bitcoin-core-secp256k1/32/): secp256k1_noverify_tests::musig ..............   Passed    0.12 sec
53      Start 34: secp256k1_noverify_tests::utils
5419/35 Test [#34](/bitcoin-core-secp256k1/34/): secp256k1_noverify_tests::utils ..............   Passed    0.00 sec
55      Start 35: secp256k1_exhaustive_tests
5620/35 Test [#15](/bitcoin-core-secp256k1/15/): secp256k1_tests::musig .......................   Passed    0.29 sec
5721/35 Test [#28](/bitcoin-core-secp256k1/28/): secp256k1_noverify_tests::ecdsa ..............   Passed    0.97 sec
5822/35 Test  [#7](/bitcoin-core-secp256k1/7/): secp256k1_tests::field .......................   Passed    1.62 sec
5923/35 Test [#25](/bitcoin-core-secp256k1/25/): secp256k1_noverify_tests::group ..............   Passed    1.75 sec
6024/35 Test [#24](/bitcoin-core-secp256k1/24/): secp256k1_noverify_tests::field ..............   Passed    1.97 sec
6125/35 Test [#11](/bitcoin-core-secp256k1/11/): secp256k1_tests::ecdsa .......................   Passed    2.80 sec
6226/35 Test [#21](/bitcoin-core-secp256k1/21/): secp256k1_noverify_tests::integer ............   Passed    3.79 sec
6327/35 Test  [#4](/bitcoin-core-secp256k1/4/): secp256k1_tests::integer .....................   Passed    3.99 sec
6428/35 Test [#33](/bitcoin-core-secp256k1/33/): secp256k1_noverify_tests::ellswift ...........   Passed    4.26 sec
6529/35 Test  [#8](/bitcoin-core-secp256k1/8/): secp256k1_tests::group .......................   Passed    4.75 sec
6630/35 Test [#18](/bitcoin-core-secp256k1/18/): secp256k1_noverify_tests::ecmult_constants ...   Passed    5.71 sec
6731/35 Test [#19](/bitcoin-core-secp256k1/19/): secp256k1_noverify_tests::ecmult .............   Passed    8.26 sec
6832/35 Test [#35](/bitcoin-core-secp256k1/35/): secp256k1_exhaustive_tests ...................   Passed    8.61 sec
6933/35 Test  [#1](/bitcoin-core-secp256k1/1/): secp256k1_tests::ecmult_constants ............   Passed   10.30 sec
7034/35 Test  [#2](/bitcoin-core-secp256k1/2/): secp256k1_tests::ecmult ......................   Passed   10.98 sec
7135/35 Test [#16](/bitcoin-core-secp256k1/16/): secp256k1_tests::ellswift ....................   Passed   12.77 sec
72
73100% tests passed, 0 tests failed out of 35
74
75Total Test time (real) =  12.78 sec
76
77The following tests did not run:
78	 12 - secp256k1_tests::recovery (Skipped)
79	 29 - secp256k1_noverify_tests::recovery (Skipped)

hebasto commented at 5:28 pm on September 10, 2025: member

Concept ACK.

furszy commented at 5:42 pm on September 10, 2025: member

Now, here you can go faster by parallelizing by test cases instead of modules. Can run ./tests -print_tests to see all available individual cases.

Yes, but that output can be quite noisy, especially when running in downstream projects such as Bitcoin Core. That said, combining both approaches should work.

For example, for this branch:
0$ ctest --test-dir build -j 16 --output-on-failure
1...
2
3100% tests passed, 0 tests failed out of 35
4
5Total Test time (real) =  12.78 sec

Cool, thanks for testing! That’s a good speedup. And we can do even better, because your branch only specifies a few single test cases, not all, to run in isolation; the rest are still running as a single process group (the library currently has 88 single test cases, and your output shows that only 35 were run - so you must have grouped most of them).

Now, here you can go faster by parallelizing by test cases instead of modules. Can run ./tests -print_tests to see all available individual cases.

Yes, but that output can be quite noisy, especially when running in downstream projects such as Bitcoin Core.

It may worth to try using the binary -j (or -jobs) command-line arg from ctest in some way if excessive logging is a concern on the CI. Because we can control it internally; the output will be minimal (unless you specify -log=1).

hebasto commented at 5:46 pm on September 10, 2025: member

And we can do even better, because your branch only specifies a few single test cases, not all, to run in isolation; the rest are still running as a single process group (the library currently has 88 single test cases, and your output shows that only 35 were run - so you must have grouped most of them).

I doubt this would improve performance, since the total runtime is now constrained by the longest-running test.

furszy commented at 7:15 pm on September 10, 2025: member

I doubt this would improve performance, since the total runtime is now constrained by the longest-running test.

Ah, I see. We could split the test in a follow-up then. At first glance, it seems internal cases are well defined.

furszy force-pushed on Sep 10, 2025

in src/tests_common.h:24 in 785c34ea86 outdated

17+
18+#if (defined(_MSC_VER) && _MSC_VER >= 1900)
19+#  include <time.h>
20+#else
21+#  include <sys/time.h>
22+#endif

hebasto commented at 10:58 am on September 11, 2025:

a759cf5b9408ca630654e04faea56ff4f6944d45:

Include <stdint.h> for int64_t?

furszy commented at 6:40 pm on September 12, 2025:

updated, thanks

in src/tests.c:29 in 785c34ea86 outdated

24@@ -25,6 +25,8 @@
25 #include "checkmem.h"
26 #include "testutil.h"
27 #include "util.h"
28+#include "unit_test.h"
29+#include "unit_test.c"

hebasto commented at 11:43 am on September 11, 2025:

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.

furszy commented at 2:20 pm on September 11, 2025:

Just left you a comment there: https://github.com/hebasto/secp256k1/commit/082b928b7d16f83d8749a695f2a4b4e293e322a4#r165610965

hebasto commented at 3:26 pm on September 11, 2025:

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.

hebasto commented at 4:50 pm on September 11, 2025:

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).

in src/tests.c:7689 in 785c34ea86 outdated

7709-    /* run test RNG tests (must run before we really initialize the test RNG) */
7710-    run_xoshiro256pp_tests();
7711+/* --- Context Independent --- */
7712+static struct TestEntry tests_no_ctx[] = {
7713+    CASE(xoshiro256pp_tests),
7714+    {NULL, NULL}

hebasto commented at 3:41 pm on September 11, 2025:

e262ba546a9d7b1e7a5977613efe36ff2c67b313:

Why is this necessary?

furszy commented at 4:10 pm on September 11, 2025:

The sentinel avoids specifying the array size for this special case, which is not included in the general modules list. I could include the size to match the structure of the others as well. This was just a small shortcut to avoid doing so.

theStack commented at 3:01 pm on September 12, 2025:

Was a bit confused as well to see these values only in a single TestEntry array, would suggest to either use sentinels for all of the arrays or for none of them for consistency.

furszy commented at 6:42 pm on September 12, 2025:

Done. Updated to use the size-based approach everywhere, mainly because the compiler may be able to optimize it.

in src/tests_common.h:6 in a759cf5b94 outdated

0@@ -0,0 +1,40 @@
1+/***********************************************************************
2+ * Distributed under the MIT software license, see the accompanying    *
3+ * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
4+ ***********************************************************************/
5+
6+#ifndef LIBSECP256K1_TEST_COMMON_H

theStack commented at 2:14 pm on September 12, 2025:

nit, for consistency with other headers:

0#ifndef SECP256K1_TEST_COMMON_H

furszy commented at 6:40 pm on September 12, 2025:

done, thanks.

in src/unit_test.c:8 in e262ba546a outdated

0@@ -0,0 +1,293 @@
1+/***********************************************************************
2+ * Copyright (c) 2025  Matias Furszyfer (furszy)                       *
3+ * Distributed under the MIT software license, see the accompanying    *
4+ * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
5+ ***********************************************************************/
6+
7+#ifndef LIBSECP256K1_UNIT_TEST_C
8+#define LIBSECP256K1_UNIT_TEST_C

theStack commented at 2:20 pm on September 12, 2025:

seems unusual to have include guards for .c files and we currently don’t follow that practice (looking at e.g. secp256k1.c), I presume we prefer a compile error if someone ever tried to include a .c file (which itself is already very unusual as well :p) more than once per translation unit?

furszy commented at 6:44 pm on September 12, 2025:

yeah sure. Extra guards removed. Thanks

theStack commented at 3:09 pm on September 12, 2025: contributor

Concept ACK

Tested this quickly on an arm64 machine and observed a nice >2x speedup (~24.4s j=1 vs. ~11.1s j=6), also played around running only tests of certain modules, which worked as expected. Left just a few first-look nits.

furszy force-pushed on Sep 12, 2025

furszy commented at 6:46 pm on September 12, 2025: member

Updated per feedback, thanks theStack and hebasto!

in src/tests_common.h:6 in 5a501061ea outdated

0@@ -0,0 +1,42 @@
1+/***********************************************************************
2+ * Distributed under the MIT software license, see the accompanying    *
3+ * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
4+ ***********************************************************************/
5+
6+#ifndef SECP256K1_TEST_COMMON_H

hebasto commented at 11:57 am on September 13, 2025:

91eac9c9411bf27607ffc1e114fec039a7fca00a: a typo:

0#ifndef SECP256K1_TESTS_COMMON_H

furszy commented at 1:39 pm on September 14, 2025:

Done, thanks.

in src/unit_test.c:22 in 5a501061ea outdated

17+#if !defined(_PID_T) && !defined(pid_t)
18+#   include <sys/types.h>
19+#endif
20+#    define SUPPORTS_CONCURRENCY 1
21+#  endif
22+#endif

hebasto commented at 12:16 pm on September 13, 2025:

I’m not sure about this implementation:

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

furszy commented at 3:10 pm on September 13, 2025:

I’m not sure about this implementation:

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

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).

furszy commented at 1:39 pm on September 14, 2025:

Done. Let me know if the build system changes can be shrunk even more. I tried to keep them minimal.

in src/unit_test.c:232 in 5a501061ea outdated

236+            perror("Error during pipe setup");
237+            return EXIT_FAILURE;
238+        }
239+
240+        pid = fork();
241+        if (pid < 0) {

hebasto commented at 12:48 pm on September 13, 2025:

318460c39aec04aa3580df5eadc7a935355968de

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

furszy commented at 1:32 pm on September 14, 2025:

I’m not sure these two functions are directly comparable. They have different return value ranges: pipe() just indicates success or failure (0 or -1), while fork() has three cases: -1 for an error, 0 in the child process, and any value greater than 0 is the child’s PID in the parent. So it seemed slightly more accurate to me to describe the entire range for fork().

in src/unit_test.h:26 in 5a501061ea outdated

21+/* --------------------------------------------------------- */
22+/* Test Framework Registry Macros                            */
23+/* --------------------------------------------------------- */
24+
25+#define CASE(name) { #name, run_##name }
26+#define CASE1(name) { #name, name }

hebasto commented at 1:13 pm on September 13, 2025:

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.

furszy commented at 3:41 pm on September 13, 2025:

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.

hebasto commented at 1:21 pm on September 13, 2025: member

Approach ACK 5a501061ea082d66e193234023843738a328e4a4.

I’ve completed my first round of reviewing. The code looks good.

refactor: move 'gettime_i64()' to tests_common.h

Relocate the clock time getter to tests_common.h to
make it easily reusable across test programs. This
will be useful for the upcoming unit test framework.

Context - why not placing it inside testutil.h?:
The bench program links against the production-compiled library,
not its own compiled version. Therefore, `gettime_i64()` cannot
be moved to testutil.h, because testutil.h calls
`secp256k1_pubkey_save()`, which exists only in the internal
secp256k1.c and not in the public API.

9cce703863

furszy force-pushed on Sep 14, 2025

furszy commented at 1:38 pm on September 14, 2025: member

Updated per feedback, thanks Hebasto!
Moved the headers existence check to the build systems (both).

in src/tests.c:7694 in 4e7bdc135a outdated

7717+};
7718+static struct TestModule registry_modules_no_ctx = MAKE_TEST_MODULE(no_ctx);
7719+
7720+/* --- Context-dependent tests start here --- */
7721+static struct TestEntry tests_general[] = {
7722+    CASE(selftest_tests),

stratospher commented at 4:17 pm on September 15, 2025:

0423532: nit: selftest_tests doesn’t require a context though unlike other tests inside tests_general.

furszy commented at 2:39 pm on September 16, 2025:

related to #1734 (review).

in configure.ac:450 in 726e70b899 outdated

442@@ -443,6 +443,16 @@ if test x"$enable_experimental" = x"no"; then
443   fi
444 fi
445 
446+# Check for concurrency support (tests only)
447+if test "x$enable_tests" != x"no"; then
448+  AC_CHECK_HEADERS([sys/types.h sys/wait.h unistd.h])
449+  SUPPORTS_CONCURRENCY=0
450+  if test "x$HAVE_SYS_TYPES_H" = xyes -a "x$HAVE_SYS_WAIT_H" = xyes -a "x$HAVE_UNISTD_H" = xyes; then

john-moffett commented at 5:28 pm on September 15, 2025:

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

furszy commented at 7:27 pm on September 15, 2025:

That’s interesting. Thanks!. Updated per feedback plus also modified it to use the AS_IF macro because that seems to be the autoconf-idiomatic way to write conditionals (which might avoid portability issues with the test … -a … option). Let me know how it goes on your end now.

john-moffett commented at 1:06 pm on September 16, 2025:

Successful 👍

in src/unit_test.c:140 in 726e70b899 outdated

135+    return 0;
136+}
137+
138+static int parse_target(const char* value, struct TestFramework* tf) {
139+    TestRef i = {/*idx_module=*/0, /*idx_test=*/0};
140+    if (tf->args.targets.size >= MAX_ARGS) {

john-moffett commented at 5:38 pm on September 15, 2025:

I think this should be checked in the inner loop, too. Passing a bunch of modules , eg,

./tests -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa -t=ecdsa

and it will write past slots[149] and clobbers adjacent struct fields. On my machine, it doesn’t even crash but just corrupts the size and logging fields.

furszy commented at 7:29 pm on September 15, 2025:

Yeah, good catch. Fixed. I should have moved the check when introduced support for module selection.

john-moffett commented at 5:40 pm on September 15, 2025: contributor

I like this a lot. Approach ACK 726e70b

furszy force-pushed on Sep 15, 2025

furszy commented at 7:33 pm on September 15, 2025: member

Updated per feedback. Thanks john-moffett! Adapted the autoconf code for better portability and fixed a misplaced targets size check.

in src/unit_test.h:7 in 0423532ea9 outdated

0@@ -0,0 +1,111 @@
1+/***********************************************************************
2+ * Copyright (c) 2025  Matias Furszyfer (furszy)                       *
3+ * Distributed under the MIT software license, see the accompanying    *
4+ * file COPYING or https://www.opensource.org/licenses/mit-license.php.*
5+ ***********************************************************************/
6+
7+#ifndef LIBSECP256K1_UNIT_TEST_H

stratospher commented at 4:48 am on September 16, 2025:

0423532: SECP256K1_UNIT_TEST_H here as well

furszy commented at 2:37 pm on September 16, 2025:

Done

in src/unit_test.c:77 in 0423532ea9 outdated

72+static int parse_iterations(const char* arg) {
73+    /* Find iteration count */
74+    if (arg) {
75+        COUNT = (int) strtol(arg, NULL, 0);
76+    } else {
77+        const char* env = getenv("SECP256K1_TEST_ITERS");

stratospher commented at 7:16 am on September 16, 2025:

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

furszy commented at 2:28 pm on September 16, 2025:

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.

in src/unit_test.c:136 in 0423532ea9 outdated

81+    }
82+    if (COUNT <= 0) {
83+        fputs("An iteration count of 0 or less is not allowed.\n", stderr);
84+        return -1;
85+    }
86+    printf("Iterations count = %i\n", COUNT);

stratospher commented at 7:24 am on September 16, 2025:

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 ?

furszy commented at 2:37 pm on September 16, 2025:

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.

in src/unit_test.h:75 in 0423532ea9 outdated

70+    /* Test modules registry */
71+    struct TestModule* registry_modules;
72+    /* Num of modules */
73+    int num_modules;
74+    /* Registry for tests that require no context setup */
75+    struct TestModule* registry_no_ctx;

stratospher commented at 12:52 pm on September 16, 2025:

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.

furszy commented at 2:02 pm on September 16, 2025:

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.

kmk142789 approved

stratospher commented at 1:15 pm on September 16, 2025: contributor

nice! useful to be able to run a subset of tests.

in src/unit_test.c:149 in aaacb77019 outdated

142+        struct TestModule* module = &tf->registry_modules[i.group];
143+        int add_all = strcmp(value, module->name) == 0; /* select all from module */
144+        for (i.idx = 0; i.idx < module->size; i.idx++) {
145+            if (add_all || strcmp(value, module->data[i.idx].name) == 0) {
146+                tf->args.targets.slots[tf->args.targets.size] = i;
147+                if (++tf->args.targets.size >= MAX_ARGS) {

john-moffett commented at 1:49 pm on September 16, 2025:

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.

furszy commented at 3:32 pm on September 16, 2025:

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.

john-moffett commented at 3:55 pm on September 16, 2025:

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;

furszy commented at 5:50 pm on September 16, 2025:

Ah ok, now we are sync. Updated per feedback. Thanks!

in src/unit_test.c:380 in aaacb77019 outdated

375+    /* Initialize test RNG and library contexts */
376+    testrand_init(tf->args.custom_seed);
377+    if (tf->fn_setup && tf->fn_setup() != 0) return EXIT_FAILURE;
378+
379+    /* Check whether to process tests sequentially or concurrently */
380+    if (tf->args.num_processes == 0) {

john-moffett commented at 2:16 pm on September 16, 2025:

Nit: should it be sequential for 1 as well?

furszy commented at 3:36 pm on September 16, 2025:

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.

in src/unit_test.c:264 in aaacb77019 outdated

259+    }
260+
261+    /* Close all pipes to signal workers to exit */
262+    for (it = 0; it < tf->args.num_processes; it++) close(pipes[it][1]);
263+    /* Wait for all workers */
264+    for (it = 0; it < tf->args.num_processes; it++) waitpid(workers[it], &status, 0);

john-moffett commented at 2:27 pm on September 16, 2025:

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?

furszy commented at 4:03 pm on September 16, 2025:

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!

john-moffett commented at 2:34 pm on September 16, 2025: contributor

Two more nits and one observation. Otherwise all looks good.

furszy force-pushed on Sep 16, 2025

test: introduce (mini) unit test framework

Lightweight unit testing framework, providing a structured way to define,
execute, and report tests. It includes a central test registry, a flexible
command-line argument parser of the form "-key=value" (facilitating future
framework extensions), ability to run tests in parallel and accumulated
test time logging reports.

So far the supported command-line args are:
- "-jobs=<num>" to specify the number of parallel workers.
- "-seed=<hex>" to specify the context seed (random if not set).
- "-iterations=<value>" to specify the number of iterations.

Compatibility Note:
To stay compatible with previous versions, the framework also supports
the two original positional arguments: the iterations count and the
RNG seed (in that order).

588f2cf993

test: adapt modules to the new test infrastructure

This not only provides a structural improvement but also
allows us to (1) specify individual tests to run and (2)
execute each of them concurrently.

f9b26f7a65

test: add -help for command-line options

Add a help message for the test suite, documenting available options,
defaults, and backward-compatible positional arguments.

e64ac7b60c

test: support running specific tests/modules targets

Add support for specifying single tests or modules to run via the
"-target" or "-t" command-line option. Multiple targets can be
provided; only the specified tests or all tests in the specified
module/s will run instead of the full suite.

Examples:
-t=<test name> runs an specific test.
-t=<module name> runs all tests within the specified module.

Both options can be provided multiple times.

601572a389

test: Add option to display all available tests

Useful option to avoid opening the large tests.c file just to find
the test case you want to run.

6afa0c3103

test: add -log option to display tests execution

When enabled (-log=1), shows test start, completion, and execution time.

7b51e53607

in src/unit_test.c:364 in 906b45a73f outdated

359+        int slot = 0;
360+        for (ref.group = 0; ref.group < tf->num_modules; ref.group++) {
361+            struct TestModule* group = &tf->registry_modules[ref.group];
362+            for (ref.idx = 0; ref.idx < group->size; ref.idx++) {
363+                tf->args.targets.slots[slot++] = ref;
364+                if (slot >= MAX_ARGS) {

john-moffett commented at 4:36 pm on September 16, 2025:

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

furszy commented at 5:51 pm on September 16, 2025:

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.

furszy force-pushed on Sep 16, 2025

john-moffett commented at 2:35 pm on September 17, 2025: contributor

ACK 7b51e53

Introduce (mini) unit test framework #1734