-
Notifications
You must be signed in to change notification settings - Fork 2k
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: use test_interactive_test_util when possible #12461
tests: use test_interactive_test_util when possible #12461
Conversation
With this PR test failed goes down from to 16 for ``z1` PR:ERROR:z1:Tests failed: 76
PR:ERROR:z1:Tests failed: 16
Te compilation issue in tests/gnrc_ndp is because of fw size. |
I'm going to trigger murdock to see which applications don't compile because of size anymore. |
@@ -22,10 +22,12 @@ | |||
TIMEOUT = int(os.environ.get('RIOT_TEST_TIMEOUT') or 10) | |||
|
|||
|
|||
def run(testfunc, timeout=TIMEOUT, echo=True, traceback=False): | |||
def run(testfunc, timeout=TIMEOUT, echo=True, traceback=False, sync=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered to make sync default to True? Wouldn't that reduce the number of files you have to change for this PR?
There are 168 line with "run(testfunc" and in the PR 154 with sync=True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I didn't initially because I wasn't sure if the PR would need to be split and introduce changes slowly, in which case a default false
was better.
I can remove them at the end, for now I rather keep it help me see easily which tests I have changed and which not :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove them at the end, for now I rather keep it help me see easily which tests I have changed and which not :)
By the end I mean after squashing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is time now, if all tests are not regressing, please squash and update the with default sync=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll test the squash version then push.
fee1d9e
to
3245eaa
Compare
rebased |
3245eaa
to
ddce923
Compare
rebased |
I can make this a priority if you have time to work on it (I think we need to move fast due to moving target rebasing). My only concern with this is that it changes the behaviour of the tests when you just want to term in and read values. if there is a way to include everything only when Also I think it would be good to evaluate this on the arduino-mega2560. I have had some problems with getting stuck in the bootloader mode when trying to poll if the terminal is ready (I believe you need a 2.6 second timeout after opening a serial connection otherwise it could take up to 10 seconds or so to go out of bootloader mode, this 10 seconds resets whenever you try to talk to it). I don't see this problem on a cheap offbrand china version but on the standard ones this is how the bootloader seems to behave. |
I was on holiday but I'm back and can get back to working on this.
I will test and see what happens.
I'll look into this |
rebased |
Failing tests:
The first two and the 4th are fixed by removing the |
9979fb5
to
cf95904
Compare
@fjmolinas, please add |
c4127ed
to
513d4c5
Compare
I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Once the CI is green, this can be merged.
Automatically call the test_utils_interactive_sync synchronization if it is used.
- Define test_utils_interactive_sync as DEFAULT_MODULE in Makefile.tests_common - For tests disabling autoinit, add test_utils_interactive_sync to main - Add DISABLE_MODULE += test_utils_interactive_sync for tests requiring sudo, `tests/shell`, `tests/minimal` and `tests/stdin` - Add shell_commands to tests/periph_wdt and tests/struct_tm_utility to pull `r` and `s` commands - Remove includes and usage in `tests/main.c` for tests that where already using test_utils_interactive_sync
- When using test_interactive_sync_utils, stdin and many more prints/puts are included. These all go into .bss/.data which quickly fills up RAM.
513d4c5
to
175c48f
Compare
All green! |
@keestux @aabadie Thanks for the review! @MrKevinWeiss Thanks for testing! |
With RIOT-OS#12941 and RIOT-OS#13613 some of the blacklisting introduced in RIOT-OS#12461 are no longer needed, since `test_interactive_test_util` is lighter or adds no extra code.
Contribution description
In order to be able to run tests on as many boards as possible use
test_interactive_test_util
in all possible cases.Many tests depend on resetting the board after the terminal has been opened to make sure the captured output is the start of the tests.
When
BOARD
's can't provide this feature usingtest_interactive_test_util
allows the application to wait for async
from the test script to actually start.Changes introduced:
sync
optionauto-init
. I did't split the commit since introducing it before the test changes would make most tests failed.Can be splited from the PR:
tests/isr_yield_higher
TODO in follow-up:
sync=True
:Testing procedure
native
samr21-xpro
iotlab-m3
Issues/PRs references
Part of #12448