tests/relic: move from unittests to regular test by smlng · Pull Request #10189 · RIOT-OS/RIOT

smlng · 2018-10-18T07:23:09Z

Contribution description

This moves tests for the relic package from unittests to a regular
test, which should help to decrease binary size of unittests.

Testing procedure

run the tests, i.e. BOARD=<your-favourite> make -C tests/pkg_relic flash test

Issues/PRs references

issue tests/unittests: Move large unittests to individual tests #10187
similar PRs: tests/libcose: Move from unittests to regular test #10183, tests/tweetnacl: Move from unittests to regular test #10184, tests/qdsa: Move from unittests to regular tests #10185, tests/cn-cbor: move from unittests to regular test #10188

cladmi · 2018-10-21T17:38:38Z

Question, the DISABLE_TEST_FOR_ARM_CORTEX_M := tests-relic and others has not been propagated here. What is the reason for this ?

cladmi · 2018-10-21T17:40:20Z

tests/pkg_relic/Makefile

+                                nucleo-l053r8 \
+                                #
+
+TEST_BOARDS_LLVM_COMPILE := native \


This should be done in the relic package to blacklist llvm if it is not compatible using TOOLCHAIN_BLACKLIST += llvm with the correct conditionals.

ah thanks, that line didn't work anyway

cladmi · 2018-10-21T17:41:10Z

tests/pkg_relic/Makefile

+TEST_BOARDS_LLVM_COMPILE := native \
+                            #
+
+CFLAGS += -DTHREAD_STACKSIZE_MAIN=\(4*THREAD_STACKSIZE_DEFAULT\)


This was not set for this test in unittests why now ?

First this test was blacklisted for all platforms other than native in the unittests. I tried to run it on different boards, and I got a hard fault + reboot with increased stack size that is fixed but the test gets stuck anyway.

cladmi

Some big changes from what was in unittests that are not currently justified.

If the update is correct, it should be specified in the migration commit message.

tests/pkg_relic/Makefile

cladmi · 2018-10-21T17:58:42Z

The test currently fails on iotlab-m3 and samr21-xpro.

With a stack size of -DTHREAD_STACKSIZE_MAIN=$5*THREAD_STACKSIZE_DEFAULT$ test successfully executes on both boards.

cladmi · 2018-10-24T08:24:58Z

tests/pkg_relic/Makefile

                                #

-export TEST_BOARDS_LLVM_COMPILE := native
+TOOLCHAIN_BLACKLIST += llvm


This should be in the relic package as it is a package issue, not from the application (like others packages using it). git grep TOOLCHAINS_BLACKLIST
And sorry it was TOOLCHAINS_BLACKLIST I was missing the S. Btw, the documentation uses the wrong name… will update it.

And currently, building in native works for ubuntu 18.04

/home/harter/work/git/RIOT/tests/pkg_relic/bin/native/tests_pkg_relic.elf RIOT native interrupts/signals initialized. LED_RED_OFF LED_GREEN_ON RIOT native board initialized. RIOT native hardware initialization complete. main(): This is RIOT! (Version: 2018.10-devel-1204-g19e1b-inetm02-pr/riot/10189/move_from_unittests_to_regular_test) . OK (1 tests)

so should only disable for arm like in

RIOT/pkg/jerryscript/Makefile.include

Lines 4 to 9 in f5379c0

ifneq (,$(filter cortex-m%,$(CPU_ARCH)))

# jerryscript package package is not using system includes right now, so

# many newlib hearders (not even stdio.h) is found

# Fixed in #9821 (so remove when merged)

TOOLCHAINS_BLACKLIST += llvm

endif

I suspect the issue with llvm to be the same as jerryscript by the way #9821

Indeed it is the same issue, so you can reuse the same comment as jerryscript.

It compiles with llvm with this fix, I can do a PR for the fix:

diff --git a/pkg/relic/Makefile b/pkg/relic/Makefile index 7e2466c9d..d73aecbee 100644 --- a/pkg/relic/Makefile +++ b/pkg/relic/Makefile @@ -18,7 +18,7 @@ all: $(PKG_BUILDDIR)/Makefile $(PKG_BUILDDIR)/Makefile: $(TOOLCHAIN_FILE) cd $(PKG_BUILDDIR) && \ - COMP="$(filter-out -Werror -Werror=old-style-definition -Werror=strict-prototypes -std=gnu99, $(CFLAGS) ) " \ + COMP="$(INCLUDES) $(filter-out -Werror -Werror=old-style-definition -Werror=strict-prototypes -std=gnu99, $(CFLAGS) ) " \ cmake -DCMAKE_TOOLCHAIN_FILE=$(TOOLCHAIN_FILE) \ -DCHECK=off -DTESTS=0 -DBENCH=0 -DSHLIB=off -Wno-dev $(RELIC_CONFIG_FLAGS) .

so just to clarify: its either blacklisting llvm for the relic package or applying your fix for jerryscript to make it work with llvm? Or is both somehow needed?

I would say it is now blacklisting because it does not work for llvm and cortex-m, it is a fact, and it is an immediate solution.

Making it work for llvm is an improvement that needs work, so a new feature somehow with specific review. So can come in a second step.

I do not delay this PR because of something that was broken for years.
I was just adding details so the blacklisting comment would be accurate.

cladmi · 2018-10-24T09:03:45Z

pkg/relic/Makefile.include

 INCLUDES += -I$(PKGDIRBASE)/relic/include
+
+ifneq (,$(filter cortex-m%,$(CPU_ARCH)))
+  # jerryscript package package is not using system includes right now, so


Same but with relic instead of jerryscript. :p

yeah fixed the wording

cladmi · 2018-11-27T17:32:40Z

I added the run tests label as it uses CI_WHITELIST and re-triggered murdock.

cladmi · 2018-11-27T17:33:40Z

I will finish reviewing after murdock.

cladmi

I successfully tested with IoT-LAB nodes with the following board and command:

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_relic clean all flash test || break; done

Additional compile tests that I ran:

It does not compile on wsn430-v1_3b due to missing time.h.
With the latest update of riotdocker image (ubuntu bionic), it does not fail for arduino-mega2560 anymore as time.h is available, but does not have enough memory to link.
It failed In the xenial image it fails because of missing time.h.

So ok to keep them blacklisted.

Python flake8 error

travis is complaining because the regexp for the test is not a python regex string so r'match_string'. By rebasing to riot/master it would be using the same environment as murdock, not sure it it would still be detected.

Change for the python test described inlined.

After this it would be ok to go

cladmi · 2018-11-27T19:34:51Z

tests/pkg_relic/tests/01-run.py

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


flake8 is correctly complaining about this one, this should be a regex string

Suggested change

child.expect('OK $\d+ tests$', timeout=120)

child.expect(r'OK $\d+ tests$', timeout=120)

When rebasing to riot/master it will not be detected anymore as it will be using riotdocker image that uses the repository flake8 version but the error is still valid.

This moves tests for the relic package from unittests to a regular test, which should help to decrease binary size of unittests.

cladmi · 2018-11-27T19:50:48Z

@smlng Please tell when you force push, github does not send any notification about it

cladmi

Diff is the same as before the rebase/squash except the regex fix.

Tested the rebase again with the same boards as before.

Also a recent version of flake8 does not complain anymore about the test script.

cladmi · 2018-11-27T19:53:20Z

Just waiting for murdock and it will be good to merge.

smlng · 2018-11-27T19:54:49Z

@cladmi sorry about the force-push. On another note: the regex is wrong in some other tests in master too.

cladmi · 2018-11-27T19:57:37Z

I know, it was already mentioned in another test migration out of tests/unittests, but static-tests are mainly only done on changed files.

cladmi · 2018-11-28T15:48:08Z

Thank you for doing the migration, I find it magic all the issues there can be with simple tasks like this.

smlng added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 18, 2018

smlng requested a review from jnohlgard October 18, 2018 07:23

smlng mentioned this pull request Oct 18, 2018

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

Open

16 tasks

smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 18, 2018

This was referenced Oct 18, 2018

tests/libcose: Move from unittests to regular test #10183

Merged

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

Merged

cladmi reviewed Oct 21, 2018

View reviewed changes

cladmi suggested changes Oct 21, 2018

View reviewed changes

cladmi reviewed Oct 21, 2018

View reviewed changes

tests/pkg_relic/Makefile Show resolved Hide resolved

smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 23, 2018

cladmi reviewed Oct 24, 2018

View reviewed changes

smlng force-pushed the pr/tests/pkg_relic branch from d9bbda4 to 569f542 Compare October 24, 2018 09:41

cladmi suggested changes Nov 27, 2018

View reviewed changes

tests/relic: move from unittests to regular test

618fd16

This moves tests for the relic package from unittests to a regular test, which should help to decrease binary size of unittests.

smlng force-pushed the pr/tests/pkg_relic branch from 569f542 to 618fd16 Compare November 27, 2018 19:41

cladmi approved these changes Nov 27, 2018

View reviewed changes

miri64 merged commit c5ff183 into RIOT-OS:master Nov 27, 2018

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

smlng deleted the pr/tests/pkg_relic branch June 25, 2019 08:41

aabadie mentioned this pull request Sep 29, 2019

pkg: tests: RELIC unittests fail on iotlab-m3 #4512

Closed

	ifneq (,$(filter cortex-m%,$(CPU_ARCH)))
	# jerryscript package package is not using system includes right now, so
	# many newlib hearders (not even stdio.h) is found
	# Fixed in #9821 (so remove when merged)
	TOOLCHAINS_BLACKLIST += llvm
	endif



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

	child.expect('OK \(\d+ tests\)', timeout=120)
	child.expect(r'OK \(\d+ tests\)', timeout=120)

Conversation

smlng commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

cladmi 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!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cladmi 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!

cladmi commented Nov 27, 2018

Uh oh!

cladmi commented Nov 27, 2018

Uh oh!

cladmi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Python flake8 error

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Nov 27, 2018

Uh oh!

cladmi left a comment

Choose a reason for hiding this comment

Uh oh!

cladmi commented Nov 27, 2018

Uh oh!

smlng commented Nov 27, 2018

Uh oh!

cladmi commented Nov 27, 2018

Uh oh!

cladmi commented Nov 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smlng commented Oct 18, 2018 •

edited

Loading

cladmi left a comment •

edited

Loading