tests/qdsa: Move from unittests to regular tests by bergzand · Pull Request #10185 · RIOT-OS/RIOT

bergzand · 2018-10-17T20:18:06Z

And another!

Contribution description

similar to #10183, this is another large package that only increases the size of the unittests.

Testing procedure

Run the tests/pkg_qdsa test.

Issues/PRs references

Part of #10187

kaspar030 · 2018-10-17T20:36:55Z

LGTM.

kaspar030

ACK.

bergzand · 2018-10-17T20:47:42Z

Adjusted blacklist and insufficient memory list based on Murdock results.

bergzand · 2018-10-17T20:52:09Z

Test takes around 66 seconds on a samr21-xpro:

2018-10-17 22:49:44,610 - INFO # main(): This is RIOT! (Version: 2018.10-devel-1213-g386c9-dystopia-pr/tests/libcose_move)
2018-10-17 22:50:50,990 - INFO # ...
2018-10-17 22:50:50,991 - INFO # OK (3 tests)

Also here the question whether we want to have this run on the CI?

cladmi

Test takes around 66 seconds on a samr21-xpro:
2018-10-17 22:49:44,610 - INFO # main(): This is RIOT! (Version: 2018.10-devel-1213-g386c9-dystopia-pr/tests/libcose_move)
2018-10-17 22:50:50,990 - INFO # ...
2018-10-17 22:50:50,991 - INFO # OK (3 tests)
Also here the question whether we want to have this run on the CI?

This one takes only 7.6 seconds on samr21-xpro I think you mixed with the libcose test according to the output :)
So it could still be run on CI.

Other than this the test worked on iotlab-m3 and samr21-xpro.

bergzand · 2018-10-21T09:47:16Z

This one takes only 7.6 seconds on samr21-xpro I think you mixed with the libcose test according to the output :)

You're right, mixed up a few numbers. As the PR is now, the test is run on all.

cladmi · 2018-10-21T13:27:22Z

tests/pkg_qdsa/tests/01-run.py

+
+
+def testfunc(child):
+    child.expect('OK \(\d+ tests\)', timeout=120)


The timeout commit can be removed as it should not be necessary, I see 5 seconds execution for an iotlab-m3 and 3 seconds for samr21-xpro.

To be more precise, the 7.6 seconds I mentioned in the main review is the total time counted by time.
But the time actually spend in the test is ~3 seconds.

Ack, sloppy copy-paste code after moving too many package tests :)

cladmi · 2018-10-21T14:03:02Z

tests/pkg_qdsa/Makefile

+
+BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-uno nucleo-f031k6
+
+CFLAGS += -DTHREAD_STACKSIZE_MAIN=\(4*THREAD_STACKSIZE_DEFAULT\)


As the issue also happened in test/tweetnacl I think you should keep the increased stack size for avrboards

AVR_BOARDS := arduino-duemilanove \ arduino-mega2560 \ arduino-uno \ waspmote-pro \ #

CFLAGS += -DTHREAD_STACKSIZE_MAIN=\(4*THREAD_STACKSIZE_DEFAULT+THREAD_EXTRA_STACKSIZE_PRINTF\)

This way, we could merge this without re-checking them for now.

Is it also okay if I increase the THREAD_STACKSIZE_DEFAULT multiplier by 1? It doesn't really make sense to increase the stack size by THREAD_EXTRA_STACKSIZE_PRINTF if there are no printf statements in the code.

Then it should be re-tested on avr which I cannot do today.
By only moving them without changing the "absurd" size, it is easy to merge.
Fixing the stack size can be another task.

There is only one platform where increasing it to 5*THREAD_STACKSIZE_DEFAULT would result in a lower total stack size compared to 4*THREAD_STACKSIZE_DEFAULT+THREAD_EXTRA_STACKSIZE_PRINTF: the lpc2387. For all other platforms, the THREAD_STACKSIZE_DEFAULT is larger than the THREAD_EXTRA_STACKSIZE_PRINTF.

I don't mind if you want to retest on the avr platforms anyway, I'm not in that much of a hurry with this.

Oh, I did not check the exact values for this. Thank you for researching it.
If 5* is bigger no need to retest for them.

So I like to keep the 5 * here as you updated and please explain it in the commit message (also that the lpc2387 is now smaller and not re-tested).
Putting your last comment directly is good for me.

This concern and solving justification also applies for the other LARGE_STACK_TESTS tests you migrated.

cladmi · 2018-10-21T16:15:24Z

Agreed with the timeout, only need to update to 5*THREAD_STACKSIZE_DEFAULT and put it in the commit message.
You can directly squash when changing these.

I tested with iotlab-m3 and samr21-xpro and the timeout is good for them. No corrupt stack.

bergzand · 2018-10-21T18:06:17Z

@cladmi squashed and reworded the commit message. Please check whether it contains al the requested information. Thanks for the thorough testing of this PR (and all the related PRs)

cladmi · 2018-10-23T14:19:23Z

The commit says 5* but the change is still 4*THREAD_STACKSIZE_DEFAULT.

However I now tested with arduino-mega2560 and it works without warning message. A ps gives me this for main thread:

Stack ( used)
   1024 (  734)

From the other avr boards, waspmote-pro has the same THREAD_STACKSIZE_DEFAULT and the two other ones are blacklisted.

So with this 4 can be enough to keep for this test.

For information, for samr21-xpro I get 4096 ( 988) and iotlab-m3 I get 4096 ( 1220).

bergzand · 2018-10-23T14:37:40Z

@cladmi Sorry for forgetting to change the actual value.

So now change the commit message to reflect the current situation?

cladmi · 2018-10-23T15:03:43Z

By keeping it to 5 for the moment, it may keep other boards we did not re-test still working.
It is the "do not change without reason" solution :)

If you prefer putting 4, just put my stack usage infos as reference.

As long as it is consistent with the change and the justification I am ok.

cladmi · 2018-10-23T16:36:58Z

I "merged" from the web interface to check murdock output.

bergzand · 2018-10-23T17:23:55Z

Okay...

cladmi · 2018-10-24T14:03:46Z

But then murdock cannot rebase it anymore anyway D:

cladmi · 2018-11-27T17:29:58Z

I am finally back after the release, can you please rebase, I think it was good to go, I will quickly re-test after rebase.

Stack size is changed from 4 times the default + printf to 5 times the default stack size. Only on the lpc2387 this reduces the resulting stack space. the test is not rerun for the lpc2387 and is untested.

bergzand · 2018-11-28T13:38:27Z

rebased!

bergzand · 2018-11-28T13:41:30Z

I might have screwed something up here with my rebase. I suspect I lost a commit here :(

miri64 · 2018-11-28T13:45:54Z

git reflog and search ;-). Good luck!

cladmi · 2018-11-28T18:48:41Z

The rebase looks ok for me, by comparing both commits against your rebase base I get this "merge-diff":

git show $(git commit-tree 'f9eebce43^{tree}' -p 4631180f1 -p 4eaf1d813 -m commit-tree)

commit 94dca98d9e0be29e3a17edf55093622097ca35eb
Merge: 4631180f1 4eaf1d813
Author: Gaëtan Harter <gaetan.harter@fu-berlin.de>
Date:   Wed Nov 28 19:45:16 2018 +0100

    commit-tree

diff --cc tests/unittests/Makefile
index dfea5bdbf,f24eb63bd..c8dae1960
--- a/tests/unittests/Makefile
+++ b/tests/unittests/Makefile
@@@ -231,8 -231,10 +231,6 @@@ ifneq (,$(filter tests-cpp_%, $(UNIT_TE
    export CPPMIX := 1
  endif
  
 -ifneq (, $(filter $(AVR_BOARDS), $(BOARD)))
 -  LARGE_STACK_TESTS += tests-qDSA
 -endif
--
- LARGE_STACK_TESTS += tests-tweetnacl
  ifneq (,$(filter $(LARGE_STACK_TESTS), $(UNIT_TESTS)))
    CFLAGS += -DTHREAD_STACKSIZE_MAIN=\(4*THREAD_STACKSIZE_DEFAULT+THREAD_EXTRA_STACKSIZE_PRINTF\)
  endif

Which corresponds to the rebased version on top of the tests-tweetnacl PR.
So for me the rebase is good.

We also note that after this PR, the LARGE_STACK_TESTS handling can be removed from unittests.

cladmi

ACK

I successfully tested on IoT-LAB for the following boards:

BOARDS="samr21-xpro pba-d-01-kw2x b-l475e-iot01a nrf52dk arduino-zero nrf52840dk iotlab-m3 microbit frdm-kw41z"
for board in ${BOARDS}; do BOARD=${board} RIOT_CI_BUILD=1 IOTLAB_NODE=auto-ssh BUILD_IN_DOCKER=1 DOCKER="sudo docker" make --no-print-directory -C tests/pkg_qdsa clean all flash test || break; done

I also re-tested locally on arduino-mega2560

Removing LARGE_STACK_TESTS will be done separately.

bergzand requested review from cladmi and kaspar030 October 17, 2018 20:19

kaspar030 approved these changes Oct 17, 2018

View reviewed changes

bergzand force-pushed the pr/tests/qdsa_move branch from e009579 to d5fb436 Compare October 17, 2018 20:47

bergzand mentioned this pull request Oct 17, 2018

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

Open

16 tasks

This was referenced Oct 18, 2018

tests/cn-cbor: move from unittests to regular test #10188

Merged

tests/relic: move from unittests to regular test #10189

Merged

tests/heatshrink: move from unittests to regular test #10202

Merged

cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 21, 2018

cladmi suggested changes Oct 21, 2018

View reviewed changes

cladmi added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Oct 21, 2018

cladmi reviewed Oct 21, 2018

View reviewed changes

cladmi mentioned this pull request Oct 21, 2018

tests/tweetnacl: Move from unittests to regular test #10184

Merged

bergzand force-pushed the pr/tests/qdsa_move branch from 143867d to bcff0f0 Compare October 21, 2018 18:05

cladmi mentioned this pull request Nov 19, 2018

Add xsltproc to build container RIOT-OS/riotdocker#43

Merged

tests/qdsa: Move from unittests to regular tests

f9eebce

Stack size is changed from 4 times the default + printf to 5 times the default stack size. Only on the lpc2387 this reduces the resulting stack space. the test is not rerun for the lpc2387 and is untested.

bergzand force-pushed the pr/tests/qdsa_move branch from 4631180 to f9eebce Compare November 28, 2018 13:38

cladmi approved these changes Nov 28, 2018

View reviewed changes

cladmi merged commit fdde802 into RIOT-OS:master Nov 28, 2018

aabadie added this to the Release 2019.01 milestone Dec 2, 2018



		def testfunc(child):
		child.expect('OK \(\d+ tests\)', timeout=120)


		BOARD_INSUFFICIENT_MEMORY := arduino-duemilanove arduino-uno nucleo-f031k6

		CFLAGS += -DTHREAD_STACKSIZE_MAIN=\(4*THREAD_STACKSIZE_DEFAULT\)

Conversation

bergzand commented Oct 17, 2018 • edited by cladmi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Oct 17, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Oct 17, 2018

Uh oh!

bergzand commented Oct 17, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Oct 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Oct 21, 2018

Uh oh!

bergzand commented Oct 21, 2018

Uh oh!

cladmi commented Oct 23, 2018

Uh oh!

bergzand commented Oct 23, 2018

Uh oh!

cladmi commented Oct 23, 2018

Uh oh!

cladmi commented Oct 23, 2018

Uh oh!

bergzand commented Oct 23, 2018

Uh oh!

cladmi commented Oct 24, 2018

Uh oh!

cladmi commented Nov 27, 2018

Uh oh!

bergzand commented Nov 28, 2018

Uh oh!

bergzand commented Nov 28, 2018

Uh oh!

miri64 commented Nov 28, 2018

Uh oh!

cladmi commented Nov 28, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergzand commented Oct 17, 2018 •

edited by cladmi

Loading