tests/unittests: Move large unittests to individual tests

bergzand commented 5 years ago

I propose breaking out a number of large tests from the unittests to their own test. This to decrease the flash size of the full unittest binary a bit.

Tests:

Flash sizes as mentioned below are based on the size of the required flash of the package itself, or when it is not a package, the flash required for the test itself. All flash sizes are measured for the nrf52840dk board.

[x] tweetnacl (9KB) #10184
[x] qDSA (13KB) #10185
[x] libcose #10183
[x] hacl (10KB) #10209
[x] spiffs (16KB) #10210
[ ] gnrc_ipv6_nib (38KB)
[x] cn-cbor (1.9KB) #10188
[x] relic #10189
[x] heatshrink #10202
[x] littlefs #10212

Optionally:

[ ] hashes (19KB)
[ ] Bloom (15KB)
[ ] tests-cppctors to remove `DISABLE` tests variables.

Final task

[x] Remove LARGE_STACK_TESTS from unittests #11601
[ ] Update BOARD_INSUFFICIENT_MEMORY
- [x] Updated after all packages moved outside https://github.com/RIOT-OS/RIOT/pull/11602

~Packages that would still remain if all these tests are moved outside are cn-cbor at almost 2KB and heatshrink at 1.5KB~ removed

smlng commented 5 years ago

good idea, but I would rephrase to: move out all unittests that require external packages.

smlng commented 5 years ago

move package tests from unittests, which are

cd tests/unittests && git grep USEPKG
tests-cn_cbor/Makefile.include:USEPKG += cn-cbor
tests-hacl/Makefile.include:USEPKG += hacl
tests-heatshrink/Makefile.include:USEPKG += heatshrink
tests-libcose/Makefile.include:USEPKG += libcose
tests-qDSA/Makefile.include:USEPKG += qDSA
tests-relic/Makefile.include:USEPKG += relic
tests-tweetnacl/Makefile.include:USEPKG += tweetnacl

[x] cn_cbor #10188
[x] hacl
[x] heatshrink #10202
[x] libcose #10183
[x] qDSA #10185
[x] relic #10189
[x] tweetnacl #10184

bergzand commented 5 years ago

My goal here is shrink the size of the unittest binary a bit. Removing all packages from the unittests helps a lot and I agree that they should not be in the unittests. However there are a few tests in there that are also large enough that they in my opinion qualify for having a separate test.

bergzand commented 5 years ago

at #10183 we reached the conclusion that the crypto tests only get to run on native as not to increase the load on the hardware CI units too much. Those tests take around 1-2mins per crypto tests on a samr21-xpro and could significantly impact the runtime of the CI.

smlng commented 5 years ago

My goal here is shrink the size of the unittest binary a bit. Removing all packages from the unittests helps a lot and I agree that they should not be in the unittests. However there are a few tests in there that are also large enough that they in my opinion qualify for having a separate test.

yep, maybe we should discuss (again) a general split of the (unit)tests by main-folder, i.e., core, drivers, net, pkg, and sys - which apart from drivers would apply to unittests as well.

bergzand commented 5 years ago

The current set of open PRs shaves approximately 70KB of the unittest binary

cladmi commented 5 years ago

I paused reviewing the tests splits since the feature freeze. I have less time and I think it would be easier to merge them after the release in case we need to provide bug fix and backports.

jnohlgard commented 5 years ago

tests-hashes overflows the stack on frdm-kw41z when running the full unit tests suite. Backtrace:

Thread 5 received signal SIGTRAP, Trace/breakpoint trap.
0x0002ea1c in be32enc_vect (dst_=0x1fff934c <idle_stack+212>, src_=0x1fff94d4 <main_stack+348>, len=<optimized out>)
    at /home/jgn/work/src/riot/sys/hashes/sha256.c:73
73              dst[i] = __builtin_bswap32(src[i]);
(gdb) bt
#0  0x0002ea1c in be32enc_vect (dst_=0x1fff934c <idle_stack+212>, src_=0x1fff94d4 <main_stack+348>, 
    len=<optimized out>) at /home/jgn/work/src/riot/sys/hashes/sha256.c:73
#1  0x0002ea5a in sha256_transform (state=state@entry=0x1fff94ac <main_stack+308>, 
    block=block@entry=0x1fff94d4 <main_stack+348> "My cool secret seed, you'll never guess it ;P 123456!\200")
    at /home/jgn/work/src/riot/sys/hashes/sha256.c:135
#2  0x0002ec4c in sha256_update (ctx=ctx@entry=0x1fff94ac <main_stack+308>, 
    data=data@entry=0x1fff948c <main_stack+276>, len=len@entry=8) at /home/jgn/work/src/riot/sys/hashes/sha256.c:234
#3  0x0002ecae in sha256_pad (ctx=<optimized out>) at /home/jgn/work/src/riot/sys/hashes/sha256.c:187
#4  sha256_final (ctx=0x1fff94ac <main_stack+308>, dst=0x1fff95f4 <main_stack+636>)
    at /home/jgn/work/src/riot/sys/hashes/sha256.c:256
#5  0x0002ed3c in sha256 (data=data@entry=0x1fff9884 <main_stack+1292>, len=len@entry=53, 
    digest=digest@entry=0x1fff95f4 <main_stack+636>) at /home/jgn/work/src/riot/sys/hashes/sha256.c:276
#6  0x0002eef2 in sha256_chain_with_waypoints (seed=seed@entry=0x1fff9884 <main_stack+1292>, seed_length=53, 
    elements=17, tail_element=tail_element@entry=0x1fffa9d4 <tail_hash_chain_element>, 
    waypoints=0x1fff95f0 <main_stack+632>, waypoints@entry=0x69bd65e1, 
    waypoints_length=waypoints_length@entry=0x1fff9860 <main_stack+1256>)
    at /home/jgn/work/src/riot/sys/hashes/sha256.c:413
#7  0x0001295a in test_sha256_hash_chain_store_whole ()
    at /home/jgn/work/src/riot/tests/unittests/tests-hashes/tests-hashes-sha256-chain.c:125
#8  0x0002a2b2 in TestCase_run (self=0x1fff9900 <main_stack+1416>, result=0x1fff99b0 <result_>)
    at /home/jgn/work/src/riot/sys/embunit/TestCase.c:58
#9  0x0002a268 in TestCaller_run (self=0x55450 <hashes_sha256_tests>, result=0x1fff99b0 <result_>)
    at /home/jgn/work/src/riot/sys/embunit/TestCaller.c:54
#10 0x00002736 in TestRunner_runTest (test=<optimized out>) at /home/jgn/work/src/riot/sys/embunit/TestRunner.c:99
#11 0x000125a2 in tests_hashes () at /home/jgn/work/src/riot/tests/unittests/tests-hashes/tests-hashes.c:30
#12 0x0000253e in main () at /home/jgn/work/src/riot/tests/unittests/main.c:35

bergzand commented 5 years ago

@gebart Thanks for reporting. I think we split of most (probably all) of the tests that require and configured increased stack size. So these tests that do require increased stack size, but never configured it trigger a few errors.

jnohlgard commented 5 years ago

I did another test with increased stack size and measured the stack usage after all tests had run. On frdm-kw41z, the full unittests suite required 1652 bytes at most. I did not investigate where this is used, but I assume that it is in tests-hashes because of the stack overflow crash I logged earlier.

cladmi commented 5 years ago

Note, we will need to update the BOARD_INSUFFICIENT_MEMORY list at some point, but we can keep it for a last step as it would be a pain to maintain with the multiple open branches.

cladmi commented 5 years ago

I noticed it would be also nice to move tests-cpp_ctors outside to remove all the DISABLE_ tests variables.

cladmi commented 5 years ago

I am listing the boards that can now be re-enabled.

cladmi commented 5 years ago

@miri64 I was about to move tests/unittests/tests-gnrc_ipv6_nib to tests/gnrc_ipv6_nib and a directory already exists. What is the difference between both ?

Also as we were talking about it last time, the static const in the functions are not put in common.

git grep '    static' tests-gnrc_ipv6_nib/   | grep 'next_hop' 
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop1 = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop2 = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop1 = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop2 = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },
tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-internal.c:    static const ipv6_addr_t next_hop = { .u64 = { { .u8 = LINK_LOCAL_PREFIX },

arm-none-eabi-readelf --symbols bin/iotlab-m3/tests_unittests.elf  | grep next_hop 
   649: 0800ad7c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9303
   650: 0800ad8c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9337
   651: 0800ad9c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9369
   652: 0800adac    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9403
   653: 0800adbc    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9437
   654: 0800adcc    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9473
   655: 0800addc    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9507
   656: 0800adec    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9563
   657: 0800adfc    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9579
   658: 0800ae0c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9598
   659: 0800ae1c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9612
   660: 0800ae2c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9631
   661: 0800ae3c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9649
   662: 0800ae4c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9665
   663: 0800ae5c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9691
   664: 0800ae6c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9711
   665: 0800ae7c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9730
   666: 0800ae8c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9770
   667: 0800ae9c    16 OBJECT  LOCAL  DEFAULT    1 next_hop.9789
   886: 0800d7a4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8572
   887: 0800d7b4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8583
   888: 0800d7c4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8599
   889: 0800d7d4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8665
   890: 0800d7e4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8863
   891: 0800d7f4    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8877
   892: 0800d804    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8895
   893: 0800d814    16 OBJECT  LOCAL  DEFAULT    1 next_hop.8934
   894: 0800d824    16 OBJECT  LOCAL  DEFAULT    1 next_hop1.8615
   895: 0800d834    16 OBJECT  LOCAL  DEFAULT    1 next_hop1.8635
   896: 0800d844    16 OBJECT  LOCAL  DEFAULT    1 next_hop2.8616
   897: 0800d854    16 OBJECT  LOCAL  DEFAULT    1 next_hop2.8636

miri64 commented 5 years ago

@miri64 I was about to move tests/unittests/tests-gnrc_ipv6_nib to tests/gnrc_ipv6_nib and a directory already exists. What is the difference between both ?

I have to look at it again to be 100% sure, but I think the tests/unittests one do pure unittests (so they try to avoid side effects and just test the data structures themselves), while the tests/gnrc_ipv6_nib once do tests with the context of a running stack (configured as IPv6 host; in contrast to tests/gnrc_ipv6_nib_6ln which does this for a 6Lo host), like injecting packets into the stack that manipulate the NIB, causing neighbor probes by sending to unknown neighbors, etc.

TL;DR: tests/gnrc_ipv6_nib requires the network stack to run, tests/unittests/tests-gnrc_ipv6_nib don't.

Also as we were talking about it last time, the static const in the functions are not put in common.

Then I guess most memory (also in other tests) can be saved by making those constants public.

cladmi commented 5 years ago

I just noticed a funny thing.

All the unittests tests, except for armv7 boards, are linked with g++ instead of gcc. Sounds great.

cladmi commented 5 years ago

I am splitting tests-crypto to its own test as part of https://github.com/RIOT-OS/RIOT/pull/12038 and I moved cpp_ctors out of unittests in https://github.com/RIOT-OS/RIOT/pull/12040

miri64 commented 5 years ago

Then I guess most memory (also in other tests) can be saved by making those constants public.

Mhhh... I did that, but somehow it grew instead of shrinking:

On current master (741d5437a7ce60b7833c938776d72faddaea5be7):

$ BOARD=nrf52840dk RIOT_CI_BUILD=1 make -C tests/unittests/ tests-gnrc_ipv6_nib clean all -j16
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

make: Nothing to be done for 'clean'.
   text    data     bss     dec     hex filename
  57112     116   12716   69944   11138 /home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests/bin/nrf52840dk/tests_unittests.elf
make: Nothing to be done for 'all'.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'

On my branch:

$ BOARD=nrf52840dk RIOT_CI_BUILD=1 make -C tests/unittests/ tests-gnrc_ipv6_nib clean all -j16
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

make: Nothing to be done for 'clean'.
   text    data     bss     dec     hex filename
  57772     116   12716   70604   113cc /home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests/bin/nrf52840dk/tests_unittests.elf
make: Nothing to be done for 'all'.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'

cladmi commented 5 years ago

Then I guess most memory (also in other tests) can be saved by making those constants public.

Mhhh... I did that, but somehow it grew instead of shrinking:

On current master (741d543):

$ BOARD=nrf52840dk RIOT_CI_BUILD=1 make -C tests/unittests/ tests-gnrc_ipv6_nib clean all -j16
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

make: Nothing to be done for 'clean'.
   text      data     bss     dec     hex filename
  57112       116   12716   69944   11138 /home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests/bin/nrf52840dk/tests_unittests.elf
make: Nothing to be done for 'all'.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'

On my branch:

$ BOARD=nrf52840dk RIOT_CI_BUILD=1 make -C tests/unittests/ tests-gnrc_ipv6_nib clean all -j16
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'
Building application "tests_unittests" for "nrf52840dk" with MCU "nrf52".

make: Nothing to be done for 'clean'.
   text      data     bss     dec     hex filename
  57772       116   12716   70604   113cc /home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests/bin/nrf52840dk/tests_unittests.elf
make: Nothing to be done for 'all'.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/tests/unittests'

I tried it too and when I changed it in one file, it shrinked it, but the other made it grow too :/ It may be some type of long operand to get the structure.

Splitting it would solve the issue for unittests at least.

miri64 commented 5 years ago

Splitting it would solve the issue for unittests at least.

Ok, but those should be distinct from tests/gnrc_ipv6_nib. Maybe tests/gnrc_ipv6_nib_adt?

cladmi commented 5 years ago

No idea about what adt means, but a different name indeed as it is unittests only and not as the other one more "integration" tests iirc what you told me :)

miri64 commented 5 years ago

ADT = Abstract Data Type ;-)

RIOT-OS / RIOT