-
Notifications
You must be signed in to change notification settings - Fork 8k
tests/samples: drivers: adc: rework tests filtering and configuration #94585
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
base: main
Are you sure you want to change the base?
tests/samples: drivers: adc: rework tests filtering and configuration #94585
Conversation
fee055b
to
60289a3
Compare
60289a3
to
c046f0e
Compare
5df6649
to
bcb7286
Compare
@JarmouniA Please clarify. |
If you mean in CI, they won't be built anymore because of the filtering in test conf yaml file. |
return True | ||
return False | ||
elif ast[0] == "dt_node_has_prop": | ||
# 1st arg 'label' must be a valid node alias or a node path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to rename dt_node_prop_enabled
into dt_nodelabel_prop_enabled
to clarify that its 1st argument is a label. There's only 1 place where it's used (samples/boards/nxp/mimxrt1170_evk_cm7/magic_addr/sample.yaml).
It would make your added dt_node_has_prop
clearer that its 1st arg is a node path/alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow-up PR where I will document the different expressions as well.
Not just CI. |
So here is how I see it, putting CI aside, samples are for demonstration purposes. In the case of ADC, the utility of an overlay/conf is to show a user how to configure a certain ADC IP/driver for a certain usecase. Hence, having 1 overlay/conf per IP/driver per usecase is sufficient in my opinion to fulfill this aim. There is absolutely no added value whatsoever in having 10s of overlays/confs for the same ADC IP/driver with different boards. Having said that, I think I will leave the overlays/confs clean up for another PR and focus on enhancing test scenarios filtering/selection in this one in order to get this merged sooner. |
The added value is that the samples/tests work out of the box for these boards. |
Following this logic, you would be fine with having 847 (number of supported boards rn) overlay/conf files in each sample? Or let's say just the ADC sample (assuming all Zephyr supported boards have an ADC)? |
Tests are a completely different matter than samples in my opinion, they are not supposed to build/run on any board, that makes no sense, they are not demos, rather they should build/run only one targets used on vendor test benches and Zephyr Project CI, and the selection of those targets should be based on well defined criterias like:
|
Hi @hakehuang |
we do not have aligned agreement on this yet. I would say for now the module owner can make the judgement . |
a425e78
to
80e5aa1
Compare
Add 'dt_node_has_prop' expression to use the existence of a node property as a filter in Twister test configuration yaml files. First argument must be a node alias (in aliases node) or a node path, not a nodelabel. Signed-off-by: Abderrahmane JARMOUNI <[email protected]>
Make ADC tests' test scenarios depend on the existence of required DT elements. Signed-off-by: Abderrahmane JARMOUNI <[email protected]>
Make ADC samples' test scenarios depend on the existence of required DT elements. Signed-off-by: Abderrahmane JARMOUNI <[email protected]>
80e5aa1
to
efacc1d
Compare
|
Update: postponed the clean-up of |
# 1st arg 'label' must be a valid node alias or a node path | ||
label = ast[1][0] | ||
try: | ||
node = edt.get_node(label) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not name this variable path
(as the get_node()
parameter is named)? label
is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be an alias too, but I agree it's confusing, will recheck in a follow-up PR to document these expressions and address #94585 (comment)
Make tests/samples test scenarios depend on the existence of required DT elements (aliases, "zephyr,user" properties...).
Motivation behind the filtering changes:
The number of board overlays in ADC tests/samples is completely out of control in my opinion. And the reason is: most of their test scenarios get triggered, & fail every single time a board PR is opened and has the
adc
tag in[board].yaml
(& almost always it does because most people assume those tags are a general list of supported features..., not test dedicated), then an overlay is added, as a fix for CI, eventhough the exact ADC IP/driver is already tested elsewhere on many boards.