JOSS: unit tests

[ludopulles]

While reviewing for JOSS (openjournals/joss-reviews#5272), I encountered the following errors in the unit tests:

• There was a segmentation fault caused by https://github.com/AngryMaciek/hypercomplex/blob/master/.test/unit/test.cpp#L1553
• There seem to be some integer overflows happening in the testcase "Hypercomplex: Polynomial lib test" (https://github.com/AngryMaciek/hypercomplex/blob/master/.test/unit/test.cpp#L1628), which I suspect was not intentional.

After changing L1553 to Polynomial<9>, I got this output:

[ludo@unit 0]# g++ -g -fsanitize=address,undefined test.cpp -lmpfr -I../../
[ludo@unit 0]# ./a.out "*Polynomial lib test*"
Filters: *Polynomial lib test*
../../hypercomplex/./Polynomial.hpp:221:23: runtime error: signed integer overflow: 2350762724241362075 + 7026887158483631575 cannot be represented in type 'long int'
../../hypercomplex/./Polynomial.hpp:223:68: runtime error: signed integer overflow: 4687921697387726500 + 7020006581695868425 cannot be represented in type 'long int'
../../hypercomplex/./Polynomial.hpp:184:65: runtime error: signed integer overflow: 4741992405962908075 + 4741992405962908075 cannot be represented in type 'long int'
../../hypercomplex/./Polynomial.hpp:200:65: runtime error: signed integer overflow: 8658947902678907891 - -5880580300698688189 cannot be represented in type 'long int'
../../hypercomplex/./Polynomial.hpp:221:31: runtime error: signed integer overflow: -3907215870331955536 * -635402420 cannot be represented in type 'long int'
1,0,2,0,2
2,1,0,0,2
3,0,2,2,2
0,3,1,1,3
===============================================================================
All tests passed (199 assertions in 1 test case)

[ludopulles]

During compilation I also ran into this problem: catchorg/Catch2#2421.

[vissarion]

I have a similar compilation error (and a few warnings) with clang-14

mkdir ../.test/unit/hypercomplex; \
cp Hypercomplex.hpp ../.test/unit/hypercomplex/Hypercomplex.hpp; \
cp Polynomial.hpp ../.test/unit/hypercomplex/Polynomial.hpp; \
cd ../.test/unit; \
clang++ -O0 -Wall --std=c++17 -o test test.cpp -lmpfr -lgmp; \
./test -d yes -w NoAssertions --use-colour yes --benchmark-samples 100 --benchmark-resamples 100000; \
rm -rf hypercomplex test
In file included from test.cpp:16:
./catch.hpp:10818:34: error: constexpr variable 'sigStackSize' must be initialized by a constant expression
    static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
                                 ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./catch.hpp:10818:58: note: non-constexpr function 'sysconf' cannot be used in a constant expression
    static constexpr std::size_t sigStackSize = 32768 >= MINSIGSTKSZ ? 32768 : MINSIGSTKSZ;
                                                         ^
/usr/include/x86_64-linux-gnu/bits/sigstksz.h:32:22: note: expanded from macro 'MINSIGSTKSZ'
# define MINSIGSTKSZ SIGSTKSZ
                     ^
/usr/include/x86_64-linux-gnu/bits/sigstksz.h:28:19: note: expanded from macro 'SIGSTKSZ'
# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
                  ^
/usr/include/unistd.h:640:17: note: declared here
extern long int sysconf (int __name) __THROW;
                ^
In file included from test.cpp:16:
./catch.hpp:10877:33: error: variable length array declaration not allowed at file scope
    char FatalConditionHandler::altStackMem[sigStackSize] = {};
                                ^           ~~~~~~~~~~~~
test.cpp:244:12: warning: explicitly assigning value of variable of type 'Hypercomplex<TestType, dim4>' to itself [-Wself-assign-overloaded]
        h1 = h1;
        ~~ ^ ~~
test.cpp:937:16: warning: explicitly assigning value of variable of type 'Hypercomplex<mpfr_t, dim4>' (aka 'Hypercomplex<__mpfr_struct[1], dim4>') to itself [-Wself-assign-overloaded]
            h1 = h1;
            ~~ ^ ~~
test.cpp:1457:12: warning: explicitly assigning value of variable of type 'Polynomial<deg>' to itself [-Wself-assign-overloaded]
        P1 = P1;
        ~~ ^ ~~
test.cpp:1886:16: warning: explicitly assigning value of variable of type 'Hypercomplex<Polynomial<MaxDeg>, dim>' to itself [-Wself-assign-overloaded]
            hA = hA;
            ~~ ^ ~~
4 warnings and 2 errors generated.

[AngryMaciek]

@ludopulles

1. typo: template parameter should be $n+1$ for an array of size $n$
2. My bad, the test code:

            short int si = 3;
            unsigned short int usi = 3;
            int i = 3;
            unsigned int ui = 3;
            long int li = 3;
            unsigned long int uli = 3;
            long long int lli = 3;
            unsigned long long int ulli = 3;
            REQUIRE_NOTHROW(hA ^= si);
            REQUIRE_NOTHROW(hA ^= usi);
            REQUIRE_NOTHROW(hA ^= i);
            REQUIRE_NOTHROW(hA ^= ui);
            REQUIRE_NOTHROW(hA ^= li);
            REQUIRE_NOTHROW(hA ^= uli);
            REQUIRE_NOTHROW(hA ^= lli);
            REQUIRE_NOTHROW(hA ^= ulli);

is a series of power-assignments, ending up with hA ^ 6561; obviously there are limits for long long arithmetic.

Will be fixed in a new branch 🆗

[AngryMaciek]

Alright, I have added #define CATCH_CONFIG_NO_POSIX_SIGNALS as they suggest in their issue; on my machine and in the ci everything is OK; hopefully this fixes it for you too.
Warnings are OK; I am testing self-assignment operator in these exact lines.

[AngryMaciek]

Fixed on branch revision : could you please confirm?

[ludopulles]

Works 👍

[vissarion]

The compile time Catch2 error is now gone (in revision branch) but now I am getting a runtime Segfault in TEST_CASE( "CD[256] | N = 257", "[local]" ). The same if I comment out TEST_CASE( "CD[256] | N = 257", "[local]" ) and uncomment TEST_CASE( "CD[1024] | N = 1031", "[local]" ). This is with both g++-12 and clang++-14 with -O0 and without.

[vissarion]

For your reference this is the report from sanitizer

AddressSanitizer:DEADLYSIGNAL
=================================================================
==23354==ERROR: AddressSanitizer: stack-overflow on address 0x7ffd4db8d2f8 (pc 0x00000086a6f6 bp 0x7ffd4dd15c00 sp 0x7ffd4db8d2e0 T0)
    #0 0x86a6f6 in Hypercomplex<Polynomial<257u>, 256u> RingInverse<257u, 256u>(Hypercomplex<Polynomial<257u>, 256u> const&, long const&) hypercomplex/Hypercomplex.hpp:1532
    #1 0x7ee1bd in Hypercomplex<Polynomial<257u>, 256u> PUBLICKEY<257u, 256u>(Hypercomplex<Polynomial<257u>, 256u> const&, Hypercomplex<Polynomial<257u>, 256u> const&, long const&) hypercomplex/Hypercomplex.hpp:1574
    #2 0x618732 in ____C_A_T_C_H____T_E_S_T____152 /home/vissarion/workspace/hypercomplex/.test/unit/test.cpp:3176
    #3 0x4919a5 in Catch::TestInvokerAsFunction::invoke() const /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:14213
    #4 0x48c937 in Catch::TestCase::invoke() const /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:14056
    #5 0x46ceb7 in Catch::RunContext::invokeActiveTestCase() /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:12915
    #6 0x46b763 in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:12888
    #7 0x46106f in Catch::RunContext::runTest(Catch::TestCase const&) /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:12649
    #8 0x476ff4 in execute /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:13243
    #9 0x47ea91 in Catch::Session::runInternal() /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:13449
    #10 0x47d2e4 in Catch::Session::run() /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:13405
    #11 0x7ef9a5 in int Catch::Session::run<char>(int, char const* const*) /home/vissarion/workspace/hypercomplex/.test/unit/catch.hpp:13127
    #12 0x619b66 in main /home/vissarion/workspace/hypercomplex/.test/unit/test.cpp:3264
    #13 0x7f9d7f829d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #14 0x7f9d7f829e3f in __libc_start_main_impl ../csu/libc-start.c:392
    #15 0x40d4a4 in _start (/home/vissarion/workspace/hypercomplex/.test/unit/test+0x40d4a4)

SUMMARY: AddressSanitizer: stack-overflow hypercomplex/Hypercomplex.hpp:1532 in Hypercomplex<Polynomial<257u>, 256u> RingInverse<257u, 256u>(Hypercomplex<Polynomial<257u>, 256u> const&, long const&)
==23354==ABORTING

[AngryMaciek]

We are not getting any errors, neither in CI nor at the Gitpod test I yesterday added.
Could you please post the exact command you run and your steps (if they are different then those from gitpodtest.sh)?

EDIT:

Do you get any errors when commenting out TEST_CASE( "CD[256] | N = 257", "[local]" ) too?

[vissarion]

I was running the commands as posted in the comment above #35 (comment)
There is a ~[local] argument missing, which when added results in all tests passing. So, this is fixed also on my side.
You can close the issue.
I think it would be better not to execute the "local" tests if the local argument is not given to somehow avoid segmentation faults if some user accidentally misses some argument. However, this is not a blocking issue.

[vissarion]

Also it would be helpful to add in the documentation detailed instructions on how to build and run the tests locally.

[AngryMaciek]

With the following information:

1. Test script passed without any issues while excluding just the last test (i.e. bigger input matrix)
2. The error msg: SUMMARY: AddressSanitizer: stack-overflow...

I am going to conclude that this is not a problem with the library itself, rather the stack size limit - exceeded by recursive calls of multiplications of big hypercomplex numbers. Seems other ppl experienced this issue too. Strangely enough valgrind did not report any errors. Moreover, this stack overflow does not crash the program: encryption-decryption is still successful for all my test cases. I think this is enough information to consider this issue resolved.

Documentation updated: I added one more line how to run the unit test script to test the installation.

It is much easier to exclude test cases in Catch2 than to specify "to include all others by default". I have made sure that in all places I run unit test script I have ~[local], just to be sure no one gets confused how I call commands. The last test is commented out because it takes very long to run (~2h). However I tested it once on my machine, it passed.

Given both reviewers approved on the changes in the comments above I consider this thread finished. The issue will be closed on PR merge. For further revision please create new issues.