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

makefiles/murdock.inc.mk: change policy to run tests by default #11680

Merged
merged 9 commits into from
Aug 21, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Jun 12, 2019

Contribution description

Change the policy to run the tests by default on murdock.

Introduce TEST_ON_CI_BLACKLIST for describing tests that must not be
run in CI.

By default if a test is available and not blacklisted it will be run by
murdock.

This will move from a whitelisting everything that works, to
blacklisting what fails. This way, failing tests will be easier to
track instead of being silently just not run.

It could even allow introducing failing tests before a fix is available.

TODO

  • This pull request will require blacklisting all what fails in a commit before changing the policy.
  • Update the remaining TEST_ON_CI_WHITELIST with the reason.
    git grep 'TEST_ON_CI_WHITELIST' '*' ':!makefiles/murdock.inc.mk'
    tests/libfixmath_unittests/Makefile:TEST_ON_CI_WHITELIST += native
    tests/pkg_libcose/Makefile:TEST_ON_CI_WHITELIST += native
    tests/pkg_tweetnacl/Makefile:TEST_ON_CI_WHITELIST += native
    tests/rng/Makefile:TEST_ON_CI_WHITELIST += native
    
  • An followup will be to update dist/tools/compile_an_test_for_board.py to maybe not handle a failure as FAILED but more expected failure if the test was not enabled.
  • Followup with a command to list all disabled tests in RIOT

Testing procedure

New behavior

If there is a test and nothing is set the board is enabled

BOARD=samr21-xpro make --no-print-directory -C tests/pkg_fatfs_vfs/ test/available test-on-ci-enabled 
Blacklisting

If all is blacklisted, no test will be run

BOARD=native make --no-print-directory -C tests/pkg_fatfs_vfs/ test/available test-on-ci-enabled 
/home/harter/work/git/RIOT/makefiles/murdock.inc.mk:47: recipe for target 'test-on-ci-enabled' failed
make: *** [test-on-ci-enabled] Error 1
BOARD=samr21-xpro make --no-print-directory -C tests/pkg_fatfs_vfs/ test/available test-on-ci-enabled 
/home/harter/work/git/RIOT/makefiles/murdock.inc.mk:47: recipe for target 'test-on-ci-enabled' failed
make: *** [test-on-ci-enabled] Error 1
diff --git a/tests/pkg_fatfs_vfs/Makefile b/tests/pkg_fatfs_vfs/Makefile
index edbf14bf4..babd1db3c 100644
--- a/tests/pkg_fatfs_vfs/Makefile
+++ b/tests/pkg_fatfs_vfs/Makefile
@@ -50,6 +50,8 @@ BOARD_WHITELIST := airfy-beacon arduino-due arduino-duemilanove \
 
 TEST_DEPS += image
 
+TEST_ON_CI_BLACKLIST += all
+
 include $(RIOTBASE)/Makefile.include
 
 image:

If one board is blacklisted only this one will not run

BOARD=native make --no-print-directory -C tests/pkg_fatfs_vfs/ test/available test-on-ci-enabled 
BOARD=samr21-xpro make --no-print-directory -C tests/pkg_fatfs_vfs/ test/available test-on-ci-enabled 
/home/harter/work/git/RIOT/makefiles/murdock.inc.mk:47: recipe for target 'test-on-ci-enabled' failed
make: *** [test-on-ci-enabled] Error 1

with this diff

diff --git a/tests/pkg_fatfs_vfs/Makefile b/tests/pkg_fatfs_vfs/Makefile
index edbf14bf4..d44376293 100644
--- a/tests/pkg_fatfs_vfs/Makefile
+++ b/tests/pkg_fatfs_vfs/Makefile
@@ -50,6 +50,8 @@ BOARD_WHITELIST := airfy-beacon arduino-due arduino-duemilanove \
 
 TEST_DEPS += image
 
+TEST_ON_CI_BLACKLIST += samr21-xpro
+
 include $(RIOTBASE)/Makefile.include
 
 image:

Kept behavior when TEST_ON_CI_WHITELIST is set

The previous behavior when WHITELIST is set did not change.

Enabled tests are still enabled:

# Only native enabled
BOARD=native make --no-print-directory -C tests/libfixmath_unittests/ test-on-ci-enabled
# all enabled
BOARD=samr21-xpro make --no-print-directory -C tests/bloom_bytes/ test-on-ci-enabled
BOARD=iotlab-m3 make --no-print-directory -C tests/bloom_bytes/ test-on-ci-enabled 

Not enabled tests are still not enabled

BOARD=samr21-xpro make --no-print-directory -C tests/libfixmath_unittests/ test-on-ci-enabled 
/home/harter/work/git/RIOT/makefiles/murdock.inc.mk:47: recipe for target 'test-on-ci-enabled' failed
make: *** [test-on-ci-enabled] Error 1

If a board does not have enough memory, it is still disabled

RIOT_CI_BUILD=1 BOARD=samr21-xpro make --no-print-directory -C tests/unittests/ test-on-ci-enabled 
CI-build: skipping link step
/home/harter/work/git/RIOT/makefiles/murdock.inc.mk:47: recipe for target 'test-on-ci-enabled' failed
make: *** [test-on-ci-enabled] Error 1

Issues/PRs references

#11128 (comment)
Some tests always fail on some boards, even some supported for testing in CI RIOT-OS/Release-Specs#98 (comment)

@cladmi cladmi added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tests Area: tests and testing framework Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before labels Jun 12, 2019
@cladmi cladmi added this to the Release 2019.07 milestone Jun 12, 2019
@cladmi cladmi removed the CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before label Jun 12, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Jun 12, 2019

Currently

  • tests/lwip crashes, but it also fails on my machine.
  • tests/socket_zep works on my machine but not on murdock
  • tests/lua_loader, tests/pkg_u8g3, tests/pthread_rwlock work on my machine but not on murdock. Are the worker running with an ascii console ?

@cladmi cladmi added CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jun 13, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Jun 13, 2019

With "cache enabled" the test report is not included for the previous ones. So must re-run it.

@cladmi cladmi 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 Jun 13, 2019
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from b280758 to bae6014 Compare June 25, 2019 16:55
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch 3 times, most recently from aa10a5e to 3af3849 Compare July 12, 2019 16:49
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from fe47d84 to 866d26f Compare August 12, 2019 09:50
@cladmi
Copy link
Contributor Author

cladmi commented Aug 12, 2019

Rebased and removed the dependency to the fix in #11688 by currently just blacklisting the test. Fixing is a different task.

@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from 605f461 to 46ec352 Compare August 12, 2019 10:04
@jcarrano jcarrano added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 20, 2019
Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the way to go with tests. This needs squashing.

@cladmi
Copy link
Contributor Author

cladmi commented Aug 20, 2019

This one is touching similar files as #11688 Should I rebase the other one on top of this one ? At least the conflicting commit.

@kaspar030
Copy link
Contributor

This one is touching similar files as #11688 Should I rebase the other one on top of this one ? At least the conflicting commit.

IMO this one is larger and should get in first. Please squash!

cladmi added 3 commits August 21, 2019 12:40
HACK, the test currently fails in CI for `native` due to `utf-8` handling.
HACK, the test implementation can fail due being de-scheduled during printf.
HACK, blacklist native as it fails to create a tun interface in murdock.
HACK, the test currently fails in CI for 'native' due to binding issue
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from 46ec352 to 75e6f7f Compare August 21, 2019 10:43
cladmi added 5 commits August 21, 2019 12:46
Disable running tests that need a setup and run with root as they cannot
currently be run in CI.
The test need a manual setup that cannot currently be done in CI.
The test fails on both murdock and on my machine due to the process
exiting directly.

pexpect.exceptions.EOF: End Of File (EOF). Exception style platform.
Only set `TEST_ON_CI_ENABLED` if the application has tests.
Currently `TEST_ON_CI_WHITELIST` was only set if there were tests.

This is a pre-step to have `TEST_ON_CI_WHITELIST` being `all` by
default.
By default if a test is available and not blacklisted it will be run by
murdock.

This will move from a whitelisting everything that works, to
blacklisting what fails. This way, failing tests will be easier to
track instead of being silently just not run.

It could even allow introducing failing tests before a fix is available.
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from 7bf0659 to d6dedd9 Compare August 21, 2019 10:51
This is now the default so not required anymore.
@cladmi cladmi force-pushed the pr/ci/change_testing_policy branch from d6dedd9 to 8e5422f Compare August 21, 2019 10:51
@cladmi
Copy link
Contributor Author

cladmi commented Aug 21, 2019

Squashed with test blacklisting first.

When enabling all tests by default, some additional tests will be run (if compatible) as I did not add TEST_ON_CI_WHITELIST += all in all directory before doing it.

Lets check no failing tests were introduced in between.

@jcarrano jcarrano added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 21, 2019
@kaspar030
Copy link
Contributor

ACK. This ran 511 tests (compared to 447 last nightly). &go.

@kaspar030 kaspar030 merged commit 8f7009e into RIOT-OS:master Aug 21, 2019
@cladmi
Copy link
Contributor Author

cladmi commented Aug 21, 2019

Thank you for the review.

Now I can do the TODO further work.

@cladmi cladmi deleted the pr/ci/change_testing_policy branch August 21, 2019 13:37
@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: tests Area: tests and testing framework CI: disable test cache If set, CI will always run all tests regardless of whether they have been run successfully before CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants