Productionize testing + add CI #33

sipa · 2021-04-16T18:28:09Z

Various improvements to testing:

Add Cirrus CI
exercise the C++ wrapper interface
operate on all field implementations simultaneously, and compare the results
make tests not run forever (but controlled by command-line complexity argument)
make tests single threaded
generic improvements to the tests
rename the test binary from test-exhaust to test
drop analysis/debug output

Also fix two bugs discovered while writing these improvements:

when requesting a clmul 64-bit field, a sketch for 63 bits was returned.
an internal sanity check in the root finding function could fail incorrectly on 32-bit platforms

src/test.cpp

src/test-exhaust.cpp

gmaxwell · 2021-04-17T23:30:16Z

ACK 707aaad

I didn't review any of the CI stuff more than a glance, and didn't test on 32-bit hosts yet (I assume CI is doing it!)

jonatack

Tested approach ACK 707aaad with a few minor comments. Each commit builds cleanly. Did not review or test the CI. Tested only on Debian 5.10.28-1 (2021-04-09) x86_64 GNU/Linux.

src/test.cpp

src/fields/clmul_8bytes.cpp

src/test-exhaust.cpp

Various changes: * Simplify logic to determine capacity of test * Also test for cases where capacity > 2*bits-1

jonatack · 2021-04-25T20:24:52Z

Smoke test at e05ed05

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test 99999999999999999999
Invalid complexity specified: `99999999999999999999'

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test 0
Invalid complexity specified: `0'

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test -1
Invalid complexity specified: `-1'

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test a
Invalid complexity specified: `a'

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test 1
Running libminisketch tests with complexity=1
All tests succesfull.

~/projects/bitcoin/minisketch ((HEAD detached at origin/pr/33))$ ./test
Running libminisketch tests with complexity=4
All tests succesfull.

s/succesfull/successful/

* Rename function from TestAll to TestExhaustive * Use C++ wrapper interface * Test all implementations simultaneously, and compare them * No need to report counts back to caller

* Rename from TestRand to TestRandomized * Use C++ wrapper interface * Add a lot more sanity checks * Compare more results between implementations * Test cases where the number of elements exceeds the capacity, but not when cancelling out duplicates.

sipa · 2021-04-26T00:45:53Z

Ok, this is ready I think.

gmaxwell · 2021-04-26T02:58:41Z

ACK 84b87d5

jonatack · 2021-04-26T08:59:54Z

ACK 84b87d5 per git range-diff 7408bde 707aaad 84b87d5, only skimmed the CI changes

jnewbery

Just did a very cursory review and ran the tests. Looks good.

jnewbery · 2021-05-06T08:41:17Z

src/test.cpp

+            long long complexity = std::stoll(arg, &len);
+            if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {


You're using complexity as a uint64_t later. Why not:

Suggested change

long long complexity = std::stoll(arg, &len);

if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {

auto complexity = static_cast<uint64_t>(std::stoll(arg, &len));

if (complexity >= 1 && len == arg.size() && (complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {

jnewbery · 2021-05-06T08:41:56Z

src/test.cpp

+        try {
+            test_complexity = 0;
+            long long complexity = std::stoll(arg, &len);
+            if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {


Is there something special about std::numeric_limits<uint64_t>::max() >> 10 ? Perhaps make a variable named max_complexity and comment what's special about that value.

There is a test_complexity << 10; this tests prevents that from overflowing.

jnewbery · 2021-05-06T09:28:52Z

src/test.cpp

+            if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {
+                test_complexity = complexity;
+            }
+        } catch (const std::logic_error&) {}


What logic error is this potentially catching? The only exceptions that stoll() can throw are std::invalid_argument and std::out_of_range.

Both std::invalid_argument and std::out_of_range are subclasses of std::logic_error, so this way we can avoid 2 catch statement.

jnewbery · 2021-05-06T09:30:53Z

src/test.cpp

+void TestComputeFunctions() {
+    for (uint32_t bits = 0; bits <= 256; ++bits) {
+        for (uint32_t fpbits = 0; fpbits <= 512; ++fpbits) {
+            std::vector<size_t> table_max_elements(1025);


std::array ?

jnewbery · 2021-05-06T09:36:18Z

src/test.cpp

+                if (capacity > 0) CHECK(table_max_elements[capacity] == 0 || table_max_elements[capacity] > table_max_elements[capacity - 1]);
+            }
+
+            std::vector<size_t> table_capacity(513);


std::array ?

sipa · 2021-05-07T04:14:49Z

@jnewbery Feel free to PR your comments here.

sipa force-pushed the 202104_test_improvements branch 5 times, most recently from 2801604 to 6c7e46e Compare April 17, 2021 00:39

sipa changed the title ~~Productionize unit tests~~ Productionize testing + add CI Apr 17, 2021

sipa force-pushed the 202104_test_improvements branch 5 times, most recently from bf514d4 to 707aaad Compare April 17, 2021 17:43

gmaxwell reviewed Apr 17, 2021

View reviewed changes

src/test.cpp Outdated Show resolved Hide resolved

gmaxwell reviewed Apr 17, 2021

View reviewed changes

src/test.cpp Outdated Show resolved Hide resolved

gmaxwell reviewed Apr 17, 2021

View reviewed changes

src/test-exhaust.cpp Outdated Show resolved Hide resolved

sipa mentioned this pull request Apr 18, 2021

Add mingw32-w64/wine CI build bitcoin-core/secp256k1#922

Merged

jonatack reviewed Apr 21, 2021

View reviewed changes

sipa force-pushed the 202104_test_improvements branch from 707aaad to e433b94 Compare April 24, 2021 00:30

sipa added 7 commits April 24, 2021 19:00

Bugfix: construct 64-bit clmul field when requested

fbdadf8

Bugfix: RecFindRoots sanity check fails on 32-bit

7adad14

Remove multi-threadedness of test-exhaust

7657fd4

Rename count -> capacity in tests

1564917

Simplify exhaustive test

c5b22c6

Various changes: * Simplify logic to determine capacity of test * Also test for cases where capacity > 2*bits-1

Remove exhaustive test analysis output

a430875

Clean up test code a bit

4c01bf7

sipa force-pushed the 202104_test_improvements branch 2 times, most recently from f149995 to e05ed05 Compare April 25, 2021 03:10

sipa added 2 commits April 25, 2021 16:47

Make test complexity configurable on cmdline

a92954f

Improve exhaustive tests

6a82ae2

* Rename function from TestAll to TestExhaustive * Use C++ wrapper interface * Test all implementations simultaneously, and compare them * No need to report counts back to caller

sipa added 4 commits April 25, 2021 16:47

Improve randomized tests

0499d6b

* Rename from TestRand to TestRandomized * Use C++ wrapper interface * Add a lot more sanity checks * Compare more results between implementations * Test cases where the number of elements exceeds the capacity, but not when cancelling out duplicates.

Rename test_exhaust to test

37f2a41

Report configuration in ./configure

de1ea97

Add Cirrus CI

84b87d5

sipa force-pushed the 202104_test_improvements branch from e05ed05 to 84b87d5 Compare April 25, 2021 23:49

sipa merged commit 901bbd3 into master Apr 26, 2021

jnewbery reviewed May 6, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Productionize testing + add CI #33

Productionize testing + add CI #33

sipa commented Apr 16, 2021 •

edited

gmaxwell commented Apr 17, 2021

jonatack left a comment •

edited

jonatack commented Apr 25, 2021

sipa commented Apr 26, 2021

gmaxwell commented Apr 26, 2021

jonatack commented Apr 26, 2021

jnewbery left a comment

jnewbery May 6, 2021

jnewbery May 6, 2021

sipa May 7, 2021

jnewbery May 6, 2021

sipa May 7, 2021 •

edited

jnewbery May 6, 2021

jnewbery May 6, 2021

sipa commented May 7, 2021

		long long complexity = std::stoll(arg, &len);
		if (complexity >= 1 && len == arg.size() && ((uint64_t)complexity <= std::numeric_limits<uint64_t>::max() >> 10)) {

Productionize testing + add CI #33

Productionize testing + add CI #33

Conversation

sipa commented Apr 16, 2021 • edited

gmaxwell commented Apr 17, 2021

jonatack left a comment • edited

Choose a reason for hiding this comment

jonatack commented Apr 25, 2021

sipa commented Apr 26, 2021

gmaxwell commented Apr 26, 2021

jonatack commented Apr 26, 2021

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery May 6, 2021

Choose a reason for hiding this comment

jnewbery May 6, 2021

Choose a reason for hiding this comment

sipa May 7, 2021

Choose a reason for hiding this comment

jnewbery May 6, 2021

Choose a reason for hiding this comment

sipa May 7, 2021 • edited

Choose a reason for hiding this comment

jnewbery May 6, 2021

Choose a reason for hiding this comment

jnewbery May 6, 2021

Choose a reason for hiding this comment

sipa commented May 7, 2021

sipa commented Apr 16, 2021 •

edited

jonatack left a comment •

edited

sipa May 7, 2021 •

edited