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: posix + libc/thrd: one tier0 integration platform per arch, improved libc coverage #76377

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Jul 27, 2024

The intent of this change is to mitigate the possibility of regressions in POSIX and related areas due to lack of test coverage between multiple architectures and multiple C libraries. There is unfortunately a compromise to make between fast CI and sufficient test coverage.

If it makes sense to reduce the scope in certain areas, please feel free to make suggestions.

Add one tier0 integration platform per supported architecture, and increase libc coverage.

Note

Depending on other suite constraints, only certain tier0 platforms may be used for certain suites (e.g. netif dependency)

@cfriedt cfriedt requested a review from ycsin July 27, 2024 15:11
@cfriedt cfriedt force-pushed the tests-posix-common-integration-platforms branch 4 times, most recently from 9fbc0da to fa87c8a Compare July 28, 2024 13:09
@cfriedt cfriedt changed the title tests: posix: one tier0 platform per arch for integration tests tests: posix + libc/thrd: one tier0 integration platform per arch and better libc coverage Jul 28, 2024
@cfriedt cfriedt changed the title tests: posix + libc/thrd: one tier0 integration platform per arch and better libc coverage tests: posix + libc/thrd: one tier0 integration platform per arch, improved libc coverage Jul 28, 2024
@cfriedt cfriedt requested a review from nashif July 29, 2024 10:18
@cfriedt cfriedt marked this pull request as ready for review July 29, 2024 10:18
@zephyrbot zephyrbot added area: POSIX POSIX API Library area: C Library C Standard Library labels Jul 29, 2024
@cfriedt cfriedt assigned nashif and unassigned stephanosio Jul 29, 2024
tests:
portability.posix.eventfd: {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that picolibc is usually the default, it looks like this configuration will test against picolibc twice and never test against minimal libc?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's been a while, but IIRC the reason why the base case exists and is empty is to allow people to override things easier on the command line. So, for example, if a completely external libc was used, people could still run the suite without having to n-select anything in addition to y-selecting things.

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add an explicit case for the minimal libc though. Good catch 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, that makes good sense, it's nice to provide a way for people to test things outside of the "known" C libraries. I was mostly wondering if we want to test minimal libc at all, because aiui, it's not happening with the proposed change (I don't know about the pre-patch status).

Copy link
Member Author

Choose a reason for hiding this comment

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

Went through all of them and made sure there was a .minimal case as well. So far so good on my desk, so I'll push.

INFO    - Added initial list of jobs to queue
INFO    - Total complete: 1822/2488  73%  skipped: 1341, failed:    0, error:    0

@cfriedt
Copy link
Member Author

cfriedt commented Jul 29, 2024

If it makes sense to reduce coverage in certain areas, please make suggestions.

There is unfortunately a bit of a compromise to make between fast CI and sufficient coverage to ensure things are regression-free, while simultaneously testing most / all architectures and most / all libc's.

@cfriedt cfriedt force-pushed the tests-posix-common-integration-platforms branch from fa87c8a to dd4e5fc Compare July 30, 2024 00:07
@cfriedt cfriedt requested a review from keith-packard July 30, 2024 14:12
ycsin
ycsin previously approved these changes Jul 30, 2024
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

Definitely make sense to me to increase the test coverage of these building blocks

keith-packard
keith-packard previously approved these changes Jul 30, 2024
nashif
nashif previously requested changes Aug 1, 2024
Add one tier0 platform per supported architecture.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt dismissed stale reviews from keith-packard and ycsin via a34026d August 4, 2024 02:47
@cfriedt cfriedt force-pushed the tests-posix-common-integration-platforms branch from dd4e5fc to a34026d Compare August 4, 2024 02:47
Copy link
Member

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

Refresh +1

@cfriedt cfriedt requested a review from nashif August 4, 2024 10:00
@cfriedt
Copy link
Member Author

cfriedt commented Aug 6, 2024

@keith-packard - feel up for a re-review?

@nashif - PTAL. Your change request was made.

@cfriedt cfriedt dismissed nashif’s stale review August 8, 2024 10:38

Changes made

@cfriedt
Copy link
Member Author

cfriedt commented Aug 8, 2024

re-ping @nashif

@cfriedt cfriedt merged commit 2aa31a3 into zephyrproject-rtos:main Aug 9, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: C Library C Standard Library area: POSIX POSIX API Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants