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

tests/drivers: move all driver tests into own folder #19435

Merged
merged 1 commit into from
May 4, 2023

Conversation

Teufelchen1
Copy link
Contributor

Hi! 🦅

Contribution description

This moves all tests/driver_*/ to tests/drivers/<driver name>/.
Reason for change is purely esthetic 🐮

Testing procedure

A passing CI should be enough - it should be (manually) checked that the new locations are still covered by it.
./dist/tools/compile_test/compile_like_murdock.py -b native -a tests/drivers/* can be helpful as well.

@github-actions github-actions bot added Area: build system Area: Build system Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework labels Mar 30, 2023
@@ -12,9 +12,6 @@ ifneq (,$(wildcard $(CURDIR)/tests*/.))
endif
endif

ifneq (,$(filter tests_driver_%,$(APPLICATION)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is no longer needed to check if the given tests is a driver test. This is now done in the tests/drivers/Makefile.drivers_common

@@ -0,0 +1,3 @@
RIOTBASE ?= $(CURDIR)/../../..
BOARD ?= samr21-xpro
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the old tests/Makefile.tests_common; All driver tests are run on this board for some reason.

@aabadie
Copy link
Contributor

aabadie commented Mar 30, 2023

Maybe have a look at #15358 for the build system integration. Otherwise, I'm 👍 for this PR.

@aabadie
Copy link
Contributor

aabadie commented Apr 17, 2023

This needs a rebase

@aabadie
Copy link
Contributor

aabadie commented Apr 17, 2023

Looks good but I don't know what the problem is with the header guards check...

@aabadie
Copy link
Contributor

aabadie commented Apr 23, 2023

Sorry, this needs another rebase...

@aabadie
Copy link
Contributor

aabadie commented Apr 23, 2023

The problem with the header guard check comes from missing updates of symlinks with mrf24j40 and kw2xrf drivers. The following diff fixes the problem locally:

diff --git a/tests/drivers/kw2xrf/common.h b/tests/drivers/kw2xrf/common.h
index e95bd2d8c6..618a21b4fc 120000
--- a/tests/drivers/kw2xrf/common.h
+++ b/tests/drivers/kw2xrf/common.h
@@ -1 +1 @@
-../ieee802154_hal/common.h
\ No newline at end of file
+../../ieee802154_hal/common.h
\ No newline at end of file
diff --git a/tests/drivers/kw2xrf/init_dev.c b/tests/drivers/kw2xrf/init_dev.c
index ddf5f5ff33..f4a4ab83e2 120000
--- a/tests/drivers/kw2xrf/init_dev.c
+++ b/tests/drivers/kw2xrf/init_dev.c
@@ -1 +1 @@
-../ieee802154_hal/init_devs.c
\ No newline at end of file
+../../ieee802154_hal/init_devs.c
\ No newline at end of file
diff --git a/tests/drivers/mrf24j40/common.h b/tests/drivers/mrf24j40/common.h
index e95bd2d8c6..618a21b4fc 120000
--- a/tests/drivers/mrf24j40/common.h
+++ b/tests/drivers/mrf24j40/common.h
@@ -1 +1 @@
-../ieee802154_hal/common.h
\ No newline at end of file
+../../ieee802154_hal/common.h
\ No newline at end of file
diff --git a/tests/drivers/mrf24j40/init_dev.c b/tests/drivers/mrf24j40/init_dev.c
index ddf5f5ff33..f4a4ab83e2 120000
--- a/tests/drivers/mrf24j40/init_dev.c
+++ b/tests/drivers/mrf24j40/init_dev.c
@@ -1 +1 @@
-../ieee802154_hal/init_devs.c
\ No newline at end of file
+../../ieee802154_hal/init_devs.c
\ No newline at end of file

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 25, 2023
@riot-ci
Copy link

riot-ci commented Apr 25, 2023

Murdock results

✔️ PASSED

bb76f13 tests/drivers: move all driver tests into own folder

Success Failures Total Runtime
6931 0 6931 10m:22s

Artifacts

@aabadie
Copy link
Contributor

aabadie commented Apr 25, 2023

Not sure what's wrong on Murdock. The error message is error: comsys-minion-0: get_supported_boards failed in tests/ieee802154_security

@Teufelchen1
Copy link
Contributor Author

Ah! It's probably failing because I overlooked tests/ieee802154_security/Makefile.

[...]
include ../driver_netdev_common/Makefile.netdev.mk

Since drivers/netdev_common is the correct location now, this fails.

I'm no sure how to fix it. Simply correcting the location does not work as the Makefiles in tests/driver/ expect $(curdir) to be there and not in tests/ieee82154_security/.
One option would be to move this test into the drivers folder but that feels wrong, as it is testing a module and not a driver (for a piece of hardware).
Alternatively, netdev_common could be moved back, out of the drivers folder, into tests/. But then it would be used by two* drivers and two** module test. Feels unclean as well.
Lastly, I could just bodge these two and make it "work". But I would rather not.

* atwinc15x0, xbee
** netstats_neighbor, ieee82154_security

@aabadie
Copy link
Contributor

aabadie commented Apr 25, 2023

Alternatively, netdev_common could be moved back, out of the drivers folder, into tests/. But then it would be used by two* drivers and two** module test. Feels unclean as well.

That option is the least worst IMHO. I'd do that.

@Teufelchen1
Copy link
Contributor Author

I think it turned out cleaner than anticipated. For ease of review, it's in separate commit. I will squash after review. :)

@aabadie
Copy link
Contributor

aabadie commented Apr 25, 2023

Murdock is still complaining: error: mobi3: get_supported_boards failed in tests/netdev_common. You might have to keep the "include ../Makefile.tests_common" in its Makefile for some reason.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Murdock is green and changes are good IMHO. Please squash!

ACK

@aabadie
Copy link
Contributor

aabadie commented Apr 28, 2023

bors merge

bors bot added a commit that referenced this pull request Apr 28, 2023
19435: tests/drivers: move all driver tests into own folder r=aabadie a=Teufelchen1

Hi! 🦅
### Contribution description

This moves all `tests/driver_*/` to `tests/drivers/<driver name>/`.
Reason for change is purely esthetic 🐮

### Testing procedure
A passing CI should be enough - it should be (manually) checked that the new locations are still covered by it. 
`./dist/tools/compile_test/compile_like_murdock.py -b native -a tests/drivers/*` can be helpful as well. 



Co-authored-by: Teufelchen1 <bennet.blischke@haw-hamburg.de>
@bors
Copy link
Contributor

bors bot commented Apr 28, 2023

Build failed:

@aabadie
Copy link
Contributor

aabadie commented May 3, 2023

Murdock failed and now this needs another rebase...

@aabadie
Copy link
Contributor

aabadie commented May 4, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented May 4, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 77bd7d0 into RIOT-OS:master May 4, 2023
@gschorcht
Copy link
Contributor

Why are there still some driver_xxx test applications in tests and have not been moved to tests/drivers?

$~> ls -1d tests/driver_*
tests/driver_htu21d 
tests/driver_sdcard_spi 
tests/driver_vl53l1x
tests/driver_vl53l1x_st_api
tests/driver_ws281x

@aabadie
Copy link
Contributor

aabadie commented May 5, 2023

Why are there still some driver_xxx test applications in tests and have not been moved to tests/drivers?

They were simply missed I think, even by the reviewer :)

@gschorcht
Copy link
Contributor

These are pretty old test applications. Therefore, I was wondering whether there was a special reason for not moving them to tests/drivers.

@aabadie
Copy link
Contributor

aabadie commented May 5, 2023

I'll open a PR in a moment to fix the problem

@gschorcht
Copy link
Contributor

BTW, it is a very nice cleanup that makes the navigation in test applications much easier. I wished we had such a change for tests/pkg_*, tests/gnrc_*, tests/periph_*, tests/zimer_* ...

@aabadie
Copy link
Contributor

aabadie commented May 5, 2023

Actually, all driver applications in tests were moved. It's just that the directories remains if they contain artifacts. Use git clean -fxx tests/driver_* and you'll see that all driver applications are now in tests/drivers/

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: doc Area: Documentation Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants