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.

Sign up for GitHub

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

Jump to bottom

tests: move all pkg applications to their own pkgs/ folder #19551

Merged
merged 7 commits into from
May 6, 2023
Merged

tests: move all pkg applications to their own pkgs/ folder #19551

merged 7 commits into from
May 6, 2023

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented May 5, 2023

Contribution description

This PR is similar as #19435 but applied to pkg test applications.

Testing procedure

Green CI

Issues/PRs references

Follow-up of #19435

@aabadie aabadie added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 5, 2023
@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 May 5, 2023
@riot-ci
Copy link

riot-ci commented May 5, 2023 •
edited
Loading

Murdock results

✔️ PASSED

07d2e1c treewide: replace remaining occurrences of tests/pkg_*

Success Failures Total Runtime
6930 0 6931 09m:28s

Artifacts

@aabadie aabadie mentioned this pull request May 5, 2023
Merged
@aabadie aabadie requested a review from gschorcht May 5, 2023 08:33
@gschorcht
Copy link
Contributor

Looks good and works as expected. One nitpicking comment, the tests for $RIOTBASE/drivers are in $RIOTBASE/tests/drivers, the tests for $RIOTBASE/pkg are now in $RIOTBASE/tests/pkgs with s. Maybe it makes sense to use same naming scheme as the source directories. I would like to have a second opinion.

@aabadie
Copy link
Contributor Author

aabadie commented May 5, 2023

the tests for $RIOTBASE/drivers are in $RIOTBASE/tests/drivers, the tests for $RIOTBASE/pkg are now in $RIOTBASE/tests/pkgs with s. Maybe it makes sense to use same naming scheme as the source directories

I agree, will change that

@aabadie aabadie force-pushed the pr/tests/pkg_move branch from 640afa3 to ead0ce8 Compare May 5, 2023 11:04
@aabadie
Copy link
Contributor Author

aabadie commented May 5, 2023 •
edited
Loading

the tests for $RIOTBASE/drivers are in $RIOTBASE/tests/drivers, the tests for $RIOTBASE/pkg are now in $RIOTBASE/tests/pkgs with s

Actually, that was a bug. The path defined in app_dirs.inc.mk without the "s"... and no pkg applications were built in the Murdock job

@aabadie aabadie force-pushed the pr/tests/pkg_move branch 2 times, most recently from 3e5f88f to 49ebd1d Compare May 5, 2023 14:15
@aabadie
Copy link
Contributor Author

aabadie commented May 5, 2023

RIOT tests are full of symlinks. But quick Murdock is finally green :)

gschorcht
gschorcht approved these changes May 5, 2023
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

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

Looks good and works as epxected.

@aabadie aabadie force-pushed the pr/tests/pkg_move branch from 49ebd1d to 04caf1b Compare May 5, 2023 15:59
@aabadie
Copy link
Contributor Author

aabadie commented May 5, 2023

For some reason the static-tests are stuck with this PR...

@aabadie aabadie mentioned this pull request May 5, 2023
Merged
@miri64
Copy link
Member

miri64 commented May 5, 2023

For some reason the static-tests are stuck with this PR...

Maybe the build_system_sanity_check? It might expect the tests (or rather their makefiles) to be in a certain path.

@miri64
Copy link
Member

miri64 commented May 5, 2023

We could cancel and restart it to see where exactly it is stuck.

@miri64
Copy link
Member

miri64 commented May 5, 2023

Ah, just canceling was sufficient: Seems like vera++ is the check that is stuck:

image

vera++ is the check running after externc, which succeeded:

RIOT/dist/tools/ci/static_tests.sh

Lines 118 to 122 in 04caf1b

run ./dist/tools/externc/check.sh
# broken configuration produces many false positives
# TODO: fix config and re-enable
# run ./dist/tools/cppcheck/check.sh
run ./dist/tools/vera++/check.sh

@aabadie aabadie requested a review from kaspar030 as a code owner May 5, 2023 20:07
@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: pkg Area: External package ports labels May 5, 2023
@aabadie
Copy link
Contributor Author

aabadie commented May 5, 2023

git grep "tests/pkg\_*" reveals other places where path to pkg test applications have to be updated, mainly in doc. I'll update the PR later.
(I also noticed that there are a few similar things missed in #19435, for Kconfig in .murdock)

@github-actions github-actions bot added Area: boards Area: Board ports Area: examples Area: Example Applications Area: network Area: Networking labels May 6, 2023
@aabadie aabadie force-pushed the pr/tests/pkg_move branch from 58d1f6b to 07d2e1c Compare May 6, 2023 05:55
@aabadie
Copy link
Contributor Author

aabadie commented May 6, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented May 6, 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 9fc5786 into RIOT-OS:master May 6, 2023
@aabadie aabadie deleted the pr/tests/pkg_move branch May 6, 2023 13:40
This was referenced May 10, 2023
Merged
Merged
Merged
@aabadie
Copy link
Contributor Author

aabadie commented May 11, 2023

nimble and lwip are also packages and have their own test application. Any opinion on whether to move their test applications to tests/pkg or not? I'm personally in favor of moving them there but maybe some person aren't (and if that's the case what could be an alternative?)

@gschorcht
Copy link
Contributor

Hm 🤔 IMHO, if they are test apps for packages, they should reside in tests/pkg.

@aabadie
Copy link
Contributor Author

aabadie commented May 12, 2023

IMHO, if they are test apps for packages, they should reside in tests/pkg.

Let's do this then!

@aabadie aabadie mentioned this pull request May 12, 2023
Merged
@aabadie
Copy link
Contributor Author

aabadie commented May 12, 2023

@gschorcht, see #19582

This was referenced May 12, 2023
Merged
Merged
@benpicco benpicco added this to the Release 2023.07 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gschorcht gschorcht gschorcht approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@smlng smlng Awaiting requested review from smlng

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

Assignees
No one assigned
Labels
Area: boards Area: Board ports Area: build system Area: Build system Area: CI Area: Continuous Integration of RIOT components Area: doc Area: Documentation Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: pkg Area: External package ports Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2023.07
Development

Successfully merging this pull request may close these issues.
5 participants
@aabadie @riot-ci @gschorcht @miri64 @benpicco