murdock: initial hardware-in-the-loop support by kaspar030 · Pull Request #8801 · RIOT-OS/RIOT

kaspar030 · 2018-03-19T13:55:16Z

Contribution description

This PR adds:

the make target "test-murdock", which will run "make test" for the given board on murdock. (needs authentication to be set up)
a per-application variable "TEST_WHITELIST", which is supposed to contain a list of boards on which CI should execute "make test".
some code in .murdock that executes "make test" after successful compilation, on nightly builds or if the label "CI: run tests" is set.

Don't merge (yet), this needs a worker for every board in "TEST_WHITELIST", otherwise CI builds stall forever.

kYc0o · 2018-03-21T14:21:24Z

tests/unittests/Makefile


+info-unittests:
+	@echo $(UNIT_TESTS)
+


It won't be useful to give info for any test? Why only for unittests (at least in the examples you're giving).

Only unittests are split into tests-*. This info target outputs that information. I want to use that to split up the unittests into seperate binaries for platforms where all tests don't fit.

I want to use that to split up the unittests into seperate binaries for platforms where all tests don't fit.

finally (:

kaspar030 · 2018-03-21T14:56:32Z

This is ready for review.

kaspar030 · 2018-03-21T16:55:22Z

I'll proactively squash now that I'm happy ;)

kYc0o · 2018-03-21T17:20:08Z

I added more tests. Let's see what murdock says.

aabadie · 2018-03-21T17:24:02Z

makefiles/murdock.inc.mk

+
+# don't whitelist tests if there's no binary
+ifeq (1,$(RIOTNOLINK))
+  TEST_WHITELIST:=


Maybe call this variable TEST_BOARD_WHITELIST to be more explicit ?

+1
With the TEST_WHITELIST = all lines I was wondering if all was a make target.

how about TEST_ON_CI_WHITELIST?

CI_BOARDS_WHITELIST ?

are boards in CI_BOARDS_WHITELIST compile-tested or not?

are boards in CI_BOARDS_WHITELIST compile-tested or not?

Good point. Since you need to build before running, there's no need for it indeed. The HIL test will also check the build.

So TEST_ON_CI_WHITELIST is better than TEST_WHITELIST? @aabadie @cladmi?

I even missed it was a variable only for CI and not for tests in general. So yes having it explicit is good.

cladmi

I agree with the global idea and implementation.
Its good to separate building machine from testing machine.

I just added some remarks inline.

cladmi · 2018-03-21T17:12:50Z

.murdock

        make -C${appdir} clean all -j${JOBS:-4}
    RES=$?

+    # run tests


The compile function compiles and runs tests, maybe it could be renamed and split in subfunctions.
This would allow removing the 5 levels of if.

Tests are only actually run on native (see below).

On other boards, the compile function runs "make test-murdock", which in turn creates a "run_test" job. thus the test jobs only created by compile().
Without massive refactoring, it has to be done like this.

No problem to run everything at once.
I was just talking about renaming the function, which could then in bash, call two sub functions to split the function in two in the script. But its cosmetic and only used by murdock. So do as you wish with it.

the murdock scripts use the actual function name to sort the results. Thus if "compile" gets changed into e.g., "dispatch_compile_and_test", murdock would call that job "dispatch_compile_and_test//" and also put the job's output into that folder + /output.txt. Thus I'm reluctant to change too much for now.

cladmi · 2018-03-21T18:31:49Z

makefiles/murdock.inc.mk

+
+# don't whitelist tests if there's no binary
+ifeq (1,$(RIOTNOLINK))
+  TEST_WHITELIST:=


+1
With the TEST_WHITELIST = all lines I was wondering if all was a make target.

cladmi · 2018-03-21T18:52:03Z

makefiles/murdock.inc.mk

+endif
+
+# create $(BINDIR)/.test file only if BOARD is in $(TEST_WHITELIST)
+link: $(BINDIR)/.test


Using the .test file here is only for knowing if there is a test because without running make another time right? which is a good reason, just asking.
A make runavailabletest that does nothing if there is no tests could be nice when run from command line.
But it would be another PR and both would be necessary.

Makefile remark:
Right now the rm -f $@ does nothing. Because if the file is there, even if TEST_WHITELIST is changed, it will not be deleted. The target could be set .PHONY to be re-run everytime.

Right now this file is created even when not in murdock, so you could remove the link: $(BINDIR)/.test dependency line and just make murdock script generate it make clean all "${BINDIR}/.test"

Makefile remark:

will fix

The target could be set .PHONY to be re-run everytime.

I chose that option so if the Makefile changes, this removes a potentially existing .test.

The target could be set .PHONY to be re-run everytime.

I chose that option so if the Makefile changes, this removes a potentially existing .test after Makefile changes.

cladmi · 2018-03-21T18:54:38Z

.murdock

+                    BOARD=$board make -C${appdir} test
+                    RES=$?
+                else
+                    if is_in_list "$board" "$TEST_BOARDS_AVAILABLE"; then


Could be replaced by elif

cladmi · 2018-03-21T19:05:11Z

.murdock

+        if [ $RUN_TESTS -eq 1 -o "$board" = "native" ]; then
+            if [ -f "${BINDIR}/.test" ]; then
+                if [ "$board" = "native" ]; then
+                    BOARD=$board make -C${appdir} test


Why not run the native tests also with test-murdock this would allow separating their output in murdock result ?

This is an optimization. Justs running the test saves the CI to create a job (with the attached .elf), copy job to the murdock master (over potentially slow network), then dispatch to a worker. That's an unnecessary network round trip.

The end result is that failed native tests end up as failed compile job, which is IMO cosmetic and (for now) worth the benefits of saving the network bandwidth.

cladmi · 2018-03-21T19:41:47Z

makefiles/murdock.inc.mk

+#
+
+# (HACK) get actual flash binary from FFLAGS.
+FLASHFILE:=$(filter $(HEXFILE) $(ELFFILE:.elf=.bin) $(ELFFILE),$(FFLAGS))


It will not work for boards using jlink.sh as flasher as it does not get the filename from command line but from an environment variable… so there is nothing in FFLAGS.

Also having a FLASHFILE variable is something I am interested to fix in general.
Move the FLASHFILE outside of FFLAGS and be called as $(FLASHER) $(FFLAGS) $(FLASHFILE).
We currently mix FLASHFILE and HEXFILE/ELFFILE/BINFILE names right now, and some HEXFILE are .bin.

Also, for nucleo boards we generate only an hex file when a .bin file is needed to flash over the mounted volume. And we will need to be able to generate both outputs to handle different flashing.

Yeah. I started fixing that for this PR, then decided that I'd prefer to support boards working with this FLASHFILE hack first and solve the problem afterwards. Ideally, once solved elsewhere, only the "FLASHFILE" variable has to be removed,and until then, we can run tests on all boards where the hack works.

Yep, should be fixed in its proper PR.

kaspar030 · 2018-03-22T10:27:11Z

I added more tests. Let's see what murdock says.

@kYc0o please open another PR for extending the enabled tests, so we split infrastructure and its application. Otherwise, this PR is unnecessarily blocked until all tests complete successfully.

kYc0o · 2018-03-22T10:34:12Z

@kYc0o please open another PR for extending the enabled tests, so we split infrastructure and its application. Otherwise, this PR is unnecessarily blocked until all tests complete successfully.

OK, I pushed here since it was already adding some tests, and I wasn't sure it would work also for a separated PR. I'll open another one then.

kaspar030 · 2018-03-23T08:24:53Z

changed "TEST_WHITELIST" to "TEST_ON_CI_WHITELIST".

IMO whe should get this in. @cladmi care to ACK?

kaspar030 · 2018-03-26T20:05:40Z

@smlng @cgundogan maybe you could take a look?

kaspar030 · 2018-03-28T16:02:40Z

@cladmi ping ACK now and I'll bring a random berlin bottled craft beer to the next pizza day!

cladmi

Looks good now.
Sorry I was on a "lets finish this quickly before doing reviews" streak and yeah takes longer than expected…

(which beer ? :p)

cladmi · 2018-03-28T16:13:53Z

Please rebase

cladmi · 2018-03-28T16:40:16Z

Its good for me, I let you merge when you want if you want more feedback.

kaspar030 · 2018-03-28T16:43:29Z

Its good for me, I let you merge when you want if you want more feedback.

Let's see how the nightly looks like.

kaspar030 · 2018-03-28T16:45:49Z

Ahja, and let's not tell everyone that on-hardware test can be triggered using the "CI: run tests" label, and test this a bit with selected PR's first. 😏

kaspar030 · 2018-03-28T16:46:07Z

(which beer ? :p)

Surprise!

kaspar030 requested a review from smlng March 19, 2018 13:55

kaspar030 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 Mar 19, 2018

kaspar030 force-pushed the add_some_hil branch from d7a2906 to e207821 Compare March 19, 2018 20:33

kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 19, 2018

kaspar030 force-pushed the add_some_hil branch 2 times, most recently from 4d1acce to a8667a3 Compare March 19, 2018 21:58

kaspar030 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 Mar 19, 2018

kaspar030 force-pushed the add_some_hil branch from 6729812 to 46a4408 Compare March 21, 2018 11:23

kaspar030 added Area: tests Area: tests and testing framework Area: CI Area: Continuous Integration of RIOT components and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Mar 21, 2018

kaspar030 force-pushed the add_some_hil branch from 78c6fea to 11c50cd Compare March 21, 2018 14:06

kYc0o reviewed Mar 21, 2018

View reviewed changes

kaspar030 force-pushed the add_some_hil branch from f920180 to 76147d7 Compare March 21, 2018 16:56

aabadie reviewed Mar 21, 2018

View reviewed changes

cladmi suggested changes Mar 21, 2018

View reviewed changes

cladmi reviewed Mar 21, 2018

View reviewed changes

kaspar030 force-pushed the add_some_hil branch from b0bb16a to 0374508 Compare March 22, 2018 10:29

kYc0o mentioned this pull request Mar 23, 2018

pkg: cncbor: misc fixes #8791

Merged

cladmi mentioned this pull request Mar 27, 2018

Makefiles: add support to generate both .hex and .bin file and add FLASHFILE variable #8838

Merged

49 tasks

miri64 mentioned this pull request Mar 27, 2018

tools: improve testrunner #7258

Closed

5 tasks

cladmi approved these changes Mar 28, 2018

View reviewed changes

kaspar030 added 4 commits March 28, 2018 18:28

make: add test-murdock target

35c73c8

murdock: add support for running tests on hardware

58a0fd8

tests: enable CI testing for some tests

a5ed5e2

tests/unittests: support CI on-hardware testing

0a55340

kaspar030 force-pushed the add_some_hil branch from 818f796 to 0a55340 Compare March 28, 2018 16:29

kaspar030 merged commit c3ac794 into RIOT-OS:master Mar 28, 2018

kaspar030 deleted the add_some_hil branch March 28, 2018 16:43

kYc0o mentioned this pull request Apr 3, 2018

tests: add on-hardware execution for selected tests #8869

Merged

kYc0o mentioned this pull request Apr 23, 2018

tests: add more hardware in the loop tests #9007

Merged

cladmi mentioned this pull request Jun 27, 2019

makefiles/murdock.inc.mk: remove flashfile hack #11755

Merged

Conversation

kaspar030 commented Mar 19, 2018

Contribution description

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!

kaspar030 commented Mar 21, 2018

Uh oh!

kaspar030 commented Mar 21, 2018

Uh oh!

kYc0o commented Mar 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!

aabadie Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

kaspar030 commented Mar 22, 2018

Uh oh!

kYc0o commented Mar 22, 2018

Uh oh!

kaspar030 commented Mar 23, 2018

Uh oh!

kaspar030 commented Mar 26, 2018

aabadie Mar 22, 2018 •

edited

Loading