Skip to content

Conversation

jonasnick
Copy link
Contributor

This PR introduces the ability for users to select specific tests to run via command line arguments. The aim is to eliminate the need to comment out parts of the tests.c file in order to speed up testing. The implementation uses similar command line flags as the benchmarks, for example:

./tests ecmult schnorrsig

The approach taken in this PR may not be the most straightforward, but it ensures backwards compatibility. The shell script located at https://gist.github.com/jonasnick/5e37248e3fa5911cd41e1da2f5f4e395 can be used to test the changes introduced in this PR. I'd be happy to add more flags if needed.

@real-or-random
Copy link
Contributor

Concept ACK

I still think it's an interesting idea for the future to look into existing test frameworks, which typically support running a subset of tests. (But that's no reason not to merge this, of course.)

@@ -28,7 +29,8 @@

#define CONDITIONAL_TEST(cnt, nam) if (COUNT < (cnt)) { printf("Skipping %s (iteration count too low)\n", nam); } else

static int COUNT = 64;
static const int DEFAULT_COUNT = 64;
static int COUNT = DEFAULT_COUNT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC doesn't like that line apparently because DEFAULT_COUNT is not a constant, which may be true by a strict reading of the standard...

Maybe try #defining DEFAULT_COUNT instead, or set COUNT = DEFAULT_COUNT; only at the beginning of main().

(By the way, this shows that CI on MSVC will timeout after 60 min in case of failure instead of just aborting. This will be fixed by e433034)

@sipa
Copy link
Contributor

sipa commented Feb 10, 2023

Concept ACK

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the commit 81f791e "tests: add "help" command line flag and output", shouldn't #include "cli_util.h" be added to all src/modules/*/bench_impl.h where the have_flag function is actually used?

@furszy
Copy link
Member

furszy commented Sep 5, 2025

A bit late to comment here, but I ended up implementing this feature differently in #1734 (built on top of the new unit test framework introduced there).

Originally, I just wanted to parallelize test execution because the suite was taking too long on my end. Since I couldn’t find a good C89-compatible unit test framework without dynamic memory allocations, I built one from scratch; supporting parallelization from the start and adding an easy to expand named-args command-line interface. Then, once the framework was in place, adding new options was easy, so I also added support for running specific targets or groups of targets (along with a few other useful options). That’s how I ended up approaching this problem from another angle. I hope I didn’t step on your toes with that.
The main idea behind the new framework is to make the testing infrastructure scalable and development smoother.

@jonasnick
Copy link
Contributor Author

I hope I didn’t step on your toes with that.

Definitely not. The approaches in both PRs seem to be very different. The goal of this PR is to allow running specific tests with minimal changes to the testing code.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While reviewing #1734, I decided to re-check this PR from a different angle.

This branch is essentially the PR branch rebased on the latest master branch @ 03fb60a, with an additional commit introducing greater parallelism when running ctest.

Below are typical runs on my laptop:

  • master branch:
$ ctest --test-dir build_master/ -j 16
Test project /home/hebasto/dev/secp256k1/secp256k1/build_master
    Start 2: secp256k1_tests
    Start 1: secp256k1_noverify_tests
    Start 3: secp256k1_exhaustive_tests
1/3 Test #3: secp256k1_exhaustive_tests .......   Passed    5.91 sec
2/3 Test #1: secp256k1_noverify_tests .........   Passed   14.57 sec
3/3 Test #2: secp256k1_tests ..................   Passed   32.50 sec

100% tests passed, 0 tests failed out of 3

Total Test time (real) =  32.51 sec
$ ctest --test-dir build_pr/ -j 16
Test project /home/hebasto/dev/secp256k1/secp256k1/build_pr
      Start 17: secp256k1_tests::ecmult
      Start 22: secp256k1_tests::ellswift
      Start  6: secp256k1_noverify_tests::ecmult
      Start 23: secp256k1_exhaustive_tests
      Start 11: secp256k1_noverify_tests::ellswift
      Start 16: secp256k1_tests::group
      Start  1: secp256k1_noverify_tests::integer
      Start 12: secp256k1_tests::integer
      Start  4: secp256k1_noverify_tests::field
      Start 15: secp256k1_tests::field
      Start  5: secp256k1_noverify_tests::group
      Start 21: secp256k1_tests::musig
      Start 18: secp256k1_tests::ecdh
      Start 10: secp256k1_noverify_tests::musig
      Start 14: secp256k1_tests::scalar
      Start 20: secp256k1_tests::schnorrsig
 1/23 Test #14: secp256k1_tests::scalar ................   Passed    0.08 sec
      Start  7: secp256k1_noverify_tests::ecdh
 2/23 Test #20: secp256k1_tests::schnorrsig ............   Passed    0.11 sec
      Start 19: secp256k1_tests::extrakeys
 3/23 Test #10: secp256k1_noverify_tests::musig ........   Passed    0.12 sec
      Start 13: secp256k1_tests::hash
 4/23 Test  #7: secp256k1_noverify_tests::ecdh .........   Passed    0.07 sec
      Start  9: secp256k1_noverify_tests::schnorrsig
 5/23 Test #19: secp256k1_tests::extrakeys .............   Passed    0.07 sec
      Start  3: secp256k1_noverify_tests::scalar
 6/23 Test #18: secp256k1_tests::ecdh ..................   Passed    0.18 sec
      Start  2: secp256k1_noverify_tests::hash
 7/23 Test #13: secp256k1_tests::hash ..................   Passed    0.06 sec
      Start  8: secp256k1_noverify_tests::extrakeys
 8/23 Test  #9: secp256k1_noverify_tests::schnorrsig ...   Passed    0.05 sec
 9/23 Test  #8: secp256k1_noverify_tests::extrakeys ....   Passed    0.03 sec
10/23 Test  #2: secp256k1_noverify_tests::hash .........   Passed    0.04 sec
11/23 Test  #3: secp256k1_noverify_tests::scalar .......   Passed    0.04 sec
12/23 Test #21: secp256k1_tests::musig .................   Passed    0.33 sec
13/23 Test  #5: secp256k1_noverify_tests::group ........   Passed    1.76 sec
14/23 Test #15: secp256k1_tests::field .................   Passed    2.14 sec
15/23 Test  #1: secp256k1_noverify_tests::integer ......   Passed    2.16 sec
16/23 Test  #4: secp256k1_noverify_tests::field ........   Passed    2.18 sec
17/23 Test #12: secp256k1_tests::integer ...............   Passed    2.41 sec
18/23 Test #16: secp256k1_tests::group .................   Passed    3.41 sec
19/23 Test #11: secp256k1_noverify_tests::ellswift .....   Passed    4.29 sec
20/23 Test #23: secp256k1_exhaustive_tests .............   Passed    6.86 sec
21/23 Test  #6: secp256k1_noverify_tests::ecmult .......   Passed    8.78 sec
22/23 Test #22: secp256k1_tests::ellswift ..............   Passed   13.06 sec
23/23 Test #17: secp256k1_tests::ecmult ................   Passed   17.17 sec

100% tests passed, 0 tests failed out of 23

Total Test time (real) =  17.17 sec

@hebasto
Copy link
Member

hebasto commented Sep 10, 2025

Concept ACK, with the hope that build system changes will be picked up (perhaps in a follow-up).

Comment on lines +7478 to +7479
fprintf(stderr, "Use ./configure --enable-module-%s.\n\n", module);
return 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fprintf(stderr, "Use ./configure --enable-module-%s.\n\n", module);
return 1;
fprintf(stderr, "Use ./configure --enable-module-%s.\n\n", module);
fprintf(stderr, "%s tests skipped.\n\n", module);
return 1;

This change allows CTest to skip tests for disabled modules gracefully, rather than merely ignoring them:

...
27/27 Test #19: secp256k1_tests::ecmult ................   Passed   19.21 sec

100% tests passed, 0 tests failed out of 27

Total Test time (real) =  19.24 sec

The following tests did not run:
	  9 - secp256k1_noverify_tests::recovery (Skipped)
	 22 - secp256k1_tests::recovery (Skipped)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running a single test file
5 participants