Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests/unittests: Move large unittests to individual tests #10187

Open
11 of 16 tasks
bergzand opened this issue Oct 17, 2018 · 22 comments
Open
11 of 16 tasks

tests/unittests: Move large unittests to individual tests #10187

bergzand opened this issue Oct 17, 2018 · 22 comments
Assignees
Labels
Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: tracking The issue tracks and organizes the sub-tasks of a larger effort

Comments

@bergzand
Copy link
Member

bergzand commented Oct 17, 2018

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.

Optionally:

  • hashes (19KB)
  • Bloom (15KB)
  • tests-cpp_ctors to remove DISABLE_ tests variables.

Final task

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

@bergzand bergzand added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: tracking The issue tracks and organizes the sub-tasks of a larger effort labels Oct 17, 2018
@bergzand bergzand self-assigned this Oct 17, 2018
@smlng
Copy link
Member

smlng commented Oct 18, 2018

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

@smlng
Copy link
Member

smlng commented Oct 18, 2018

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

@bergzand
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

smlng commented Oct 18, 2018

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
Copy link
Member Author

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

@cladmi
Copy link
Contributor

cladmi commented Oct 31, 2018

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
Copy link
Member

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
Copy link
Member Author

bergzand commented Nov 8, 2018

@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
Copy link
Member

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
Copy link
Contributor

cladmi commented Nov 27, 2018

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
Copy link
Contributor

cladmi commented May 28, 2019

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

@cladmi
Copy link
Contributor

cladmi commented May 28, 2019

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

@cladmi
Copy link
Contributor

cladmi commented Jun 3, 2019

@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
Copy link
Member

miri64 commented Jun 3, 2019

@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
Copy link
Contributor

cladmi commented Aug 20, 2019

I just noticed a funny thing.

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

@cladmi
Copy link
Contributor

cladmi commented Aug 20, 2019

I am splitting tests-crypto to its own test as part of #12038
and I moved cpp_ctors out of unittests in #12040

@miri64
Copy link
Member

miri64 commented Sep 10, 2019

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'

@cladmi
Copy link
Contributor

cladmi commented Sep 10, 2019

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
Copy link
Member

miri64 commented Sep 10, 2019

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
Copy link
Contributor

cladmi commented Sep 10, 2019

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
Copy link
Member

miri64 commented Sep 10, 2019

ADT = Abstract Data Type ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Type: tracking The issue tracks and organizes the sub-tasks of a larger effort
Projects
None yet
Development

No branches or pull requests

6 participants