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

[RFC] tests: add 'test-manual' targets #11954

Closed
wants to merge 4 commits into from

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented Aug 2, 2019

Contribution description

Add new 'test-manual' targets for tests that should be run manually and
not through automated testing.

Rationale

The goal is to separate test failures due to real test error from tests failure due to not working when run through 'make test'.
It could also be used for tests that currently need to be run as root, and for driver tests that require plugging external hardware and giving the port mapping.

Testing procedure

There is now normal test is pkg_fatfs_fvs

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

But there is test-manual.
For native the test is automated, but the documentation still mentions manual operations that can be run.

BOARD=native make --no-print-directory -C tests/pkg_fatfs_vfs/ test-manual/available flash test-manual
�[1;32mBuilding application "pkg_fatfs_vfs" for "native" with MCU "native".�[0m

if [ 34f371c7735fc6fc8e714a74a7a73a61d3ed5633 != 34f371c7735fc6fc8e714a74a7a73a61d3ed5633 ] ; then \
	git -C /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs clean -xdff ; \
	git -C /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs fetch "https://github.com/RIOT-OS/FatFS" "34f371c7735fc6fc8e714a74a7a73a61d3ed5633" ; \
	git -C /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs checkout -f 34f371c7735fc6fc8e714a74a7a73a61d3ed5633 ; \
	touch /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs/.git-downloaded ; \
fi
"make" -C /home/harter/work/git/RIOT/pkg/fatfs
"make" -C /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs
"make" -C /home/harter/work/git/RIOT/boards/native
"make" -C /home/harter/work/git/RIOT/boards/native/drivers
"make" -C /home/harter/work/git/RIOT/core
"make" -C /home/harter/work/git/RIOT/cpu/native
"make" -C /home/harter/work/git/RIOT/cpu/native/mtd
"make" -C /home/harter/work/git/RIOT/cpu/native/periph
"make" -C /home/harter/work/git/RIOT/cpu/native/vfs
"make" -C /home/harter/work/git/RIOT/drivers
"make" -C /home/harter/work/git/RIOT/drivers/mtd
"make" -C /home/harter/work/git/RIOT/drivers/periph_common
"make" -C /home/harter/work/git/RIOT/pkg/fatfs/fatfs_diskio/mtd
"make" -C /home/harter/work/git/RIOT/pkg/fatfs/fatfs_vfs
"make" -C /home/harter/work/git/RIOT/sys
"make" -C /home/harter/work/git/RIOT/sys/auto_init
"make" -C /home/harter/work/git/RIOT/sys/auto_init/storage
"make" -C /home/harter/work/git/RIOT/sys/vfs
"make" -C /home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/pkg/native/fatfs
   text	   data	    bss	    dec	    hex	filename
  64538	    764	  49844	 115146	  1c1ca	/home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/native/pkg_fatfs_vfs.elf
true 
/home/harter/work/git/RIOT/tests/pkg_fatfs_vfs/bin/native/pkg_fatfs_vfs.elf  
RIOT native interrupts/signals initialized.
Native RTC initialized.
LED_RED_OFF
LED_GREEN_ON
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2019.10-devel-212-gb5bf-pr/tests/manual)
Tests for FatFs over VFS - test results will be printed in the format test_name:result
test_mount__mount:[OK]
test_mount__umount:[OK]
test_open__mount:[OK]
test_open__open:[OK]
test_open__open_ro:[OK]
test_open__close_ro:[OK]
test_open__open_wo:[OK]
test_open__close_wo:[OK]
test_open__open_rw:[OK]
test_open__close_rw:[OK]
test_open__umount:[OK]
test_rw__mount:[OK]
test_rw__open_ro:[OK]
test_rw__read_ro:[OK]
test_rw__write_ro:[OK]
test_rw__close_ro:[OK]
test_rw__open_wo:[OK]
test_rw__read_wo:[OK]
test_rw__close_wo:[OK]
test_rw__open_rw:[OK]
test_rw__read_rw:[OK]
test_rw__write_rw:[OK]
test_rw__lseek:[OK]
test_rw__read_rw:[OK]
test_rw__close_rw:[OK]
test_rw__open_rwc:[OK]
test_rw__write_rwc:[OK]
test_rw__lseek_rwc:[OK]
test_rw__read_rwc:[OK]
test_rw__close_rwc:[OK]
test_rw__umount:[OK]
test_dir__mount:[OK]
test_dir__opendir:[OK]
test_dir__readdir1:[OK]
test_dir__readdir2:[OK]
test_dir__readdir_name:[OK]
test_dir__readdir3:[OK]
test_dir__closedir:[OK]
test_dir__umount:[OK]
test_rename__mount:[OK]
test_rename__rename:[OK]
test_rename__opendir:[OK]
test_rename__readdir1:[OK]
test_rename__readdir2:[OK]
test_rename__check_name:[OK]
test_rename__readdir3:[OK]
test_rename__closedir:[OK]
test_rename__umount:[OK]
test_unlink__mount:[OK]
test_unlink__unlink1:[OK]
test_unlink__unlink2:[OK]
test_unlink__opendir:[OK]
test_unlink__readdir:[OK]
test_unlink__closedir:[OK]
test_unlink__umount:[OK]
test_mkrmdir__mount:[OK]
test_mkrmdir__mkdir:[OK]
test_mkrmdir__opendir1:[OK]
test_mkrmdir__closedir:[OK]
test_mkrmdir__rmdir:[OK]
test_mkrmdir__opendir2:[OK]
test_mkrmdir__umount:[OK]
test_create__mount:[OK]
test_create__open_wo:[OK]
test_create__write_wo:[OK]
test_create__close_wo:[OK]
test_create__umount:[OK]
Test end.

TODO

  • Get feedback
  • Update the test documentation
  • Should it still depend on TEST_DEPS ?
  • Follow-ups for changing other tests
  • update release specs for mentioning these tests

Issues/PRs references

All automated testing on boards using dist/tools/compile_and_test_for_board/compile_and_test_for_board.py and release tests outputs that have these tests as failure RIOT-OS/Release-Specs#128 (comment)

@fjmolinas fjmolinas added Area: CI Area: Continuous Integration of RIOT components Area: tests Area: tests and testing framework Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 2, 2019
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Regarding semantics, what falls in to a manual tests? Are tests requiring specific hardware/drivers manual tests? Are tests requiring sudo manual tests? What happens to tests requiring a specific driver?

Would it make sense adding an tests-integration concept as well? I'm thinking of tests like the gnrc* ones requiring sudo, riotboot/flashwrite, upcoming suit tests, etc. I'm mentioning this because I feel there are more tests that should be excluded from our general make tests target, and where the manual qualifier doesn't apply very well.

Would it make sense adding a make test-all that would run all available tests? Or keep our current behavior and instead add a make test-auto that only runs on automatic tests and use make test to run all tests.

Although I like the idea behind this, I'm not sure about the semantics, the manual tag would apply to few of our tests. There is also the problem about tests that require hardware access, which seems to be the case in the test your provided as an example, it seems to be more about that than the "manual" side. The test could probably be adapted to load the image into the sd-card during the test-setup (I know we talked about this in IRL).

@cladmi
Copy link
Contributor Author

cladmi commented Aug 12, 2019

I can replace the manual by something else, I did not have any other more precise name idea. I was thinking as somehow supervised but may be too weird.
I first chose manual as in hardware/software integration context when there are test procedures which need to click buttons and do configuration to run a test, the term manual tests is used. Like for hardware equipment manufacturers. And automated tests can be run without anybody being there and are more the norm in software development, so kept them as test.

Here I would also put the test that need sudo as for running them you need to do something different than calling make flash test.
Also currently, as there is no way to know if a board has an external peripheral connected, I would also include the drivers_* tests, when there is no board whitelist as there is a need to manually connect and configure the pin mapping (a test could be manual for all boards and automated for few).
This would also include tests where you may need 5 commands but still make sense to add the test script in the repository.

The problem with going to test-auto and the others in make test and test-all is that these will mostly not be run with make test anyway but make flash; sudo make test, connect hardware; edit the periph_conf.h; make flash test. And cannot be 100% automated if you do not have a dedicated board and environment for that application. And if you have a board with the hardware connected and the Makefile.local for it, then you are also at a point where you can call make manual-test in your script.

For the automated tests that have an environment dependency == not a setup phase, like IPv6 support (murdock builders do not have), existing tap (or way to create them automatically without entering the sudo password), or anything, I am thinking somehow about test environment features.
This test needs "created tap interfaces" for native, which are provided in murdock so the test can be run.
So you could run the automated "compatible tests" with your setup.
This would also go in some kind of make build idea that would do a flash_and_test_if_available_and_supported without you thinking about is it supported, does it have a test, can it be run on my machine, whithout flashing/compiling it for nothing. But would be more for a phase where this creates an output file with the result directly instead of being created by dist/tools/compile_and_test_for_board (xml output results too :)). As if you skip the test due to env reasons, you want to keep a trace that you skipped it.

Create a file for setting tests targets and variables.
It is a refactoring before adding new commands.
Add new 'test-manual' targets for tests that should be run manually and
not through automated testing.
…al test

For running the test on boards, it needs a manual setup.
This however declares the "native" test as manual too.
… as manual test

No need to be blacklisted as "manual test" is a manual test.
@fjmolinas
Copy link
Contributor

fjmolinas commented Sep 27, 2019

The problem with going to test-auto and the others in make test and test-all is that these will mostly not be run with make test anyway but make flash; sudo make test, connect hardware; edit the periph_conf.h; make flash test. And cannot be 100% automated if you do not have a dedicated board and environment for that application. And if you have a board with the hardware connected and the Makefile.local for it, then you are also at a point where you can call make manual-test in your script.

The only auto for you would be native then. I think making the distinction because of board setup makes no sense in this case. We are at a point where we only make a distinction between unit-tests and all other tests. native is still a board and although there is no "flashing" the compiled code is still bootstrapped and executed, the same thing is happening in hardware. Also the opposite of "manual" is "automatic", so having the duality makes a lot of sense to me ;) .

I would actually like having:

  • test: all tests
  • test-manual:test minus test-manual
  • test-auto: all tests that do not require configuration except plugin in a board

The special case for native you are mentioning I don't know how to handle, or in general cases where for some boards the test ins manual and for others automatic.. Maybe native should be an exception where all tests are run no matter what?

@aabadie
Copy link
Contributor

aabadie commented Oct 1, 2019

I would actually like having:

test: all tests
test-manual: all tests that do not require configuration except plugin in a board
test-auto: test minus test-manual

I agree with this classification. Maybe use test-with-config or test-with-setup instead of test-manual ? This would exhibit the fact that tests run by this target require a specific configuration.

@cladmi
Copy link
Contributor Author

cladmi commented Oct 1, 2019

I also meant auto as, plugging a board as you want to test on it, even if it was not clear in my explanation. It was the "connect additional hardware on the board" that I was referring to as not going in auto.

I have no feeling for the current name at all, I just put a name to do this RFC. So whatever feels more accurate.

From the classification I think in your description you ended up mixing test-auto and test-manual. Because test-manual would be the one that require configuration more than plugin a board (so the opposite).

By thinking about it more right now, by still keeping test as "everything" it does not change the user interface so is easy to put in place.
Even if no guarantee when running tests it can rely on users judgement if the test fail to check if they need to do manual setup.
It would only be the test running scripts that need updating to use the more accurate names.

Further steps/scaling

One idea I had about going further than this, is adding a similar things as "FEATURES" but for testing.
This test needs to be able to run "sudo" without a password, or "start ethos" automatically, or "have this extra hardware on the board".

Which could allow special handling on a dev machine/murdock/iotlab to run ethos for example with specific safe arguments only.

But I will not be able to push or follow such a change.

@fjmolinas
Copy link
Contributor

By thinking about it more right now, by still keeping test as "everything" it does not change the user interface so is easy to put in place.
Even if no guarantee when running tests it can rely on users judgement if the test fail to check if they need to do manual setup.
It would only be the test running scripts that need updating to use the more accurate names.

Sounds good

I agree with this classification. Maybe use test-with-config or test-with-setup instead of test-manual ? This would exhibit the fact that tests run by this target require a specific configuration.

+1 test-with-config

@MrKevinWeiss
Copy link
Contributor

And cannot be 100% automated if you do not have a dedicated board and environment for that application.

I don't know if I agree with separating these things as automation isn't always a 1 or 0 but a gradient.
Say you want to test the ifconfig command, if you have a samr21-xpro you can test it natively but you would need an additional module with a nucelo board. This can also be automated if you have the environment setup as you mentioned.

Isn't the FEATURES_REQUIRED a solution to this.
Maybe it is better to just improve the pkg_fatfs_fvs test so it can be automated easier?

@aabadie
Copy link
Contributor

aabadie commented Oct 1, 2019

Say you want to test the ifconfig command, if you have a samr21-xpro you can test it natively but you would need an additional module with a nucelo board

This would be fixed by #11676 because applications/tests requiring radio won't be built/run on a Nucleo that doesn't provide an on-board communication interface.

@cladmi
Copy link
Contributor Author

cladmi commented Oct 1, 2019

Say you want to test the ifconfig command, if you have a samr21-xpro you can test it natively but you would need an additional module with a nucelo board. This can also be automated if you have the environment setup as you mentioned.

A nucleo with a communication shield, is a different "board" in the RIOT sense.
It may be defined from command line without creating a new directory but it still represent different hardware.
And yeah, the requirement on netif should prevent testing if there is no radio.

The pkg_fasfs_vfs is just an example, fixing this specific test does not solve that the functionality is needed. Tests that currently require root to run with scapy are also an example. All the drivers that may require looking at printed values, or to an oscilloscope.

@MrKevinWeiss
Copy link
Contributor

To me it seems like a way to just include metadata about the tests. I don't know if this is the best way to do it though. I still see a problem of some boards being able to be run automatically in the manual tests and some boards not. Where should the tests go in this case? If you put them in manual tests don't you still need some logic to determine if they can be tested automatically depending on the board? If so doesn't that defeat the point?

@stale
Copy link

stale bot commented Apr 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 4, 2020
@fjmolinas fjmolinas removed the State: stale State: The issue / PR has no activity for >185 days label Apr 4, 2020
@fjmolinas
Copy link
Contributor

I would like to revive this PR, currently when running all tests these test always fail, and its normal because they either require specific drivers or they require sudo. For me those fit perfectly in what this PR is trying to achieve

- [examples/lorawan](pba-d-01-kw2x/examples/lorawan/test.failed)
- [examples/suit_update](pba-d-01-kw2x/examples/suit_update/test.failed)
- [tests/driver_grove_ledbar](nucleo-l152re/tests/driver_grove_ledbar/test.failed)
- [tests/driver_my9221](nucleo-l152re/tests/driver_my9221/test.failed)
- [tests/emcute](pba-d-01-kw2x/tests/emcute/test.failed)
- [tests/gnrc_dhcpv6_client](pba-d-01-kw2x/tests/gnrc_dhcpv6_client/test.failed)
- [tests/gnrc_dhcpv6_client_6lbr](pba-d-01-kw2x/tests/gnrc_dhcpv6_client_6lbr/test.failed)
- [tests/gnrc_ipv6_ext](pba-d-01-kw2x/tests/gnrc_ipv6_ext/test.failed)
- [tests/gnrc_ipv6_ext_frag](pba-d-01-kw2x/tests/gnrc_ipv6_ext_frag/test.failed)
- [tests/gnrc_ipv6_ext_opt](pba-d-01-kw2x/tests/gnrc_ipv6_ext_opt/test.failed)
- [tests/gnrc_ipv6_nib_dns](pba-d-01-kw2x/tests/gnrc_ipv6_nib_dns/test.failed)
- [tests/gnrc_rpl_srh](pba-d-01-kw2x/tests/gnrc_rpl_srh/test.failed)
- [tests/gnrc_sock_dns](pba-d-01-kw2x/tests/gnrc_sock_dns/test.failed)
- [tests/gnrc_tcp](pba-d-01-kw2x/tests/gnrc_tcp/test.failed)
- [tests/pkg_fatfs_vfs](pba-d-01-kw2x/tests/pkg_fatfs_vfs/test.failed)
- [tests/pkg_semtech-loramac](pba-d-01-kw2x/tests/pkg_semtech-loramac/test.failed)

The ones requiring a driver can be addressed in different ways, I had a proposal in #12952 that got no attention but that goes exactly in the direction you mention @MrKevinWeiss and there is also @aabadie approach in #11676. But tests requiring tap interfaces or sudo for some reason can't be run automatically. The 2 lora tests require a gateway as well as a driver so are also manual or requiring extra setup.

It would be better to have REQUIRED_DRIVERS or driver provided FEATURES, but this doesn't exist so far, and it would only affect about 3-5 tests, most of the cases exposed are perfectly represented by the tests-manual or test-with-config target.

@fjmolinas
Copy link
Contributor

@miri64 might have thoughts on this as well

@fjmolinas fjmolinas requested review from miri64 and aabadie July 17, 2020 12:54
@miri64
Copy link
Member

miri64 commented Jul 17, 2020

@miri64 might have thoughts on this as well

I think manual tests and tests requiring root permissions for automation are also two distinct classes of tests. All of the sudo tests actually don't need manual intervention (maybe a started tap interface, but as we saw with the SUIT tests even that can be ensured). So if I understood you correctly there are actually three classes that may or may not be exposed via the FEATURES feature: test-manual, test-with-config, and test-root-perm.

@fjmolinas
Copy link
Contributor

@miri64 might have thoughts on this as well

I think manual tests and tests requiring root permissions for automation are also two distinct classes of tests. All of the sudo tests actually don't need manual intervention (maybe a started tap interface, but as we saw with the SUIT tests even that can be ensured). So if I understood you correctly there are actually three classes that may or may not be exposed via the FEATURES feature: test-manual, test-with-config, and test-root-perm.

IMO if they require sudo its usually for a script that setups the tap interface right? And suit requires setting up a script with sudo, even you scapyunroot requires starting a daemon process, so in a way they all require some manual setup or config. At least they all fall in a category of tests where its not enough to simply plug in a board and run the test, and somehow I feel that my suggestion in #11954 (comment) would still be a good first step for this, while we work towards a better way to specify test-metadata.

@cladmi did some previous work where he added some pytest wrappers for our tests, maybe there is also a way to integrate that and via markers get the same functionality we want here. It might avoid us from re-inventing the wheel (also thinking about the comment made by @cladmi #11954 (comment)), maybe using markers?

@miri64
Copy link
Member

miri64 commented Jul 17, 2020

IMO if they require sudo its usually for a script that setups the tap interface right?

Depends. For those tests that just use ethos, yes, but for those that use scapy they also need root permissions because RAW sockets might be used. That is what scapy_unroot circumvents.

And suit requires setting up a script with sudo, even you scapyunroot requires starting a daemon process, so in a way they all require some manual setup or config. At least they all fall in a category of tests where its not enough to simply plug in a board and run the test, and somehow I feel that my suggestion in #11954 (comment) would still be a good first step for this, while we work towards a better way to specify test-metadata

Once we have scapy_unroot deployed somewhere, its more test-with-config, true. Two environment configurations need to be assumed:

  1. a tap exists and is configured properly
  2. scapy_unroot is running for that tap

Ideally, the tests are then ported so that they automatically detect if the scapy_unroot daemon is running and re-configured as such. Until the latter is not the case, they are however test-root-perm, not test-with-config (but in no case test-manual).

Just to clarify: as a manual test I see a test that requires some kind of user interaction during the test (e.g. check thermometer in room to check if temperature sensor is working correctly).

@aabadie
Copy link
Contributor

aabadie commented Sep 25, 2020

I'd like to revamp the work that was done there. The main reason is because I'd like to be able to skip some tests in the test-on-iotlab GH action.

they are however test-root-perm, not test-with-config

I agree that we can distinguish between tests that requires root permission and test that requires configuration. I would rather rename test-root-perm to test-as-root but that's minor.

Regarding the scapy-unroot question, my opinion is that once this can be used, affected tests are likely to fall in the test-with-config because then these tests will need a local configuration (start the scapy_unroot daemon as root, so needs an extra step).

Regarding this PR, it's trivial to split the test-manual logic in 2 test-as-root and test-with-config. If we can agree on this, that would be very useful.

@aabadie
Copy link
Contributor

aabadie commented Feb 26, 2021

#15771 was merged, so closing this one.

@aabadie aabadie closed this Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Continuous Integration of RIOT components Area: tests Area: tests and testing framework Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants