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

CI: can_fast_ci_build.py unittests fix #18996

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Nov 30, 2022

Contribution description

This PR should fix #18987.

I first started with some sophisticated regex based generic replacement / mapping functionality, then decided to go simple...

The issue is that can_fast_ci_run.py correctly classifies any change to tests/unittests/tests-$foo to be a change to that module, but unittests are special in our build system - they can be built as one huge binary (tests/unittests), or individually (tests/unittests/tests-*). The latter are not built by CI and are not listed when doing make info-applications.

Testing procedure

Issues/PRs references

Fixes #18987.

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2022
@github-actions github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools labels Nov 30, 2022
@kaspar030 kaspar030 changed the title ci: can_fast_ci_build.py unittests fix CI: can_fast_ci_build.py unittests fix Nov 30, 2022
@riot-ci
Copy link

riot-ci commented Nov 30, 2022

Murdock results

✔️ PASSED

34cfdac ci/can_fast_ci_run.py: add workaround for #18987

Success Failures Total Runtime
1 0 1 54s

Artifacts

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Please squash and remove the extra commits then I think it is good. ACK.

@github-actions github-actions bot removed Area: tests Area: tests and testing framework Area: CI Area: Continuous Integration of RIOT components labels Nov 30, 2022
@kaspar030 kaspar030 merged commit 0d30def into RIOT-OS:master Nov 30, 2022
@kaspar030 kaspar030 deleted the fast_build_unittests_fix branch November 30, 2022 09:14
@miri64
Copy link
Member

miri64 commented Nov 30, 2022

Ah more general approach could be to find the parent directory that has a Makefile [edit]with an APPLICATION defined[/edit].

@maribu
Copy link
Member

maribu commented Nov 30, 2022

The tests don't define an application name, but rather include a Makefile that sets it based on the folder name.

One could try to go up the tree further and not find the closed related parent dir having a Makefile (as done now), but rather the most distant related parent folder.

But I think that unit tests a really just a special case. I think it is fine to habe a special case for it in the code.

And more generally: I wonder if it wouldn't have made more sense to just have multiple applications for the unit tests. Right now, adding more test cases always runs into the risk of reducing the number of boards with enough memory further. If we do this, we no longer need a special case here as well.

@kaspar030
Copy link
Contributor Author

I wonder if it wouldn't have made more sense to just have multiple applications for the unit tests.

Every tests-foo is already it's own application. We just don't use that in CI (and they're not exposed via "make info-applications".

@MrKevinWeiss
Copy link
Contributor

And more generally: I wonder if it wouldn't have made more sense to just have multiple applications for the unit tests. Right now, adding more test cases always runs into the risk of reducing the number of boards with enough memory further. If we do this, we no longer need a special case here as well.

Then there would be some tradeoff between flash cycles and size, ideally we do some chunking that would cram as many unittest cases in a binary as possible... But that would require some amount of smarts to implement.

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: can_fast_ci_run.py misclassifying sub unittests
5 participants