-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Twister: additional scripts #72808
base: main
Are you sure you want to change the base?
Twister: additional scripts #72808
Conversation
70a6788
to
1aff18b
Compare
doc/develop/test/twister.rst
Outdated
pre_script: | ||
<PATH_TO_PRE_SCRIPT> | ||
post_flash_script: | ||
<PATH_TO_POST_FLASH_SCRIPT> | ||
post_script: | ||
<PATH_TO_POST_SCRIPT> | ||
comment: | ||
Testing extra scripts |
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.
Are we planning to use it for tests in Zephyr tree?
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.
Solution is designed with flexibility in mind, making it suitable for use in a variety of contexts, including the Zephyr tree.
why no just put to the map.yaml file? along with --device-testing? |
Currently, we have the capability to run scripts based on specified runner parameters; however, this approach limits us to the execution of pre scripts only. An alternative method would involve enhancing the map file to accommodate script execution, but this leads to redundancy when dealing with multiple identical boards, as we would need to replicate the script type, scenario, and path information for each entry. |
1aff18b
to
a0b6ba1
Compare
This could be easily out of control as no one knows what you do in the rep/post, why we need customer for each case? why not just use pytest framework to do so? |
I concur that allowing unrestricted modifications to scripts by everyone could pose risks. However, the proposed solution does not necessitate storing the scripts or the scripting-list.yaml file within the Zephyr repository, similar to how the quarantine-list file is managed. You will have the flexibility to manage scripts within your own environment. This file is not mandatory, it serves to enhance the functionality of Twister for use in personalized settings. |
this thing is if go this way, the twister will be diverged in its behavour, quaratine is to skip test cases. but adding pre/post staff will impact the way test runs. it is better in a controlable way. unless you have strong user case to supporting doing so. |
There are many ways of usage, i could take an example power management tests, where we can set up the necessary power conditions, calibrate or reset power measurement tools, perform initial checks to validate that the device is in the right state for power measurements and gather power usage data |
@evgeniy-paltsev could you take a look? |
if script: | ||
if Path(script).is_file(): | ||
logger.info(f"{script_name} {script} will be executed") # Validate if test exists | ||
setattr(dut, script_name, script) |
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.
if the the dut
already have a script defined, then it will be overridden - this behavior should be documented.
imo, the new scripting schema should have an attribute like override
(default = true ?) to resolve which configuration takes precedence.
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.
this is explained in twister.rst, you manage this which script will be executed so you don't need adding extra parameter
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.
My concern is around the complexity introduced with this feature now with 3 configuration sources: command line, device map, and this new config (not mention its regexp selection rules).
Looks like a DUT can be selected several times matching different test case & platform pattens, so without explicit constrains its pre-/post- script constellation will, quite likely, composed in a funny way.
as @hakehuang also noted:
this thing is if go this way, the twister will be diverged in its behavour, quaratine is to skip test cases. but adding pre/post staff will impact the way test runs. it is better in a controlable way. unless you have strong user case to supporting doing so.
@majunkier what about these alternative improvements then:
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
ac28429
to
9c50aac
Compare
Apart from review changes I have implemented additional parameters to the scripting schema: |
continue | ||
if element.platforms and not _matches_element(platform, element.re_platforms): | ||
continue | ||
return element |
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.
To deal with several matching entries situation I'd suggest to look over all elements and return a list with all matched scripting elements.
# Iterate over all DUTs to set the appropriate scripts if they match the platform and are supported | ||
for dut in self.env.hwm.duts: | ||
# Check if the platform matches and if the platform is supported by the matched scripting | ||
if dut.platform == plat.name and validate_boards(plat.name, matched_scripting.platforms): |
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.
handle several matched entries case here to check either a single match, or only one override
element is present which becomes the resulting entry. All other cases will be configuration error.
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.
Currently, matched_scripting = self.scripting.get_matched_scripting(instance.testsuite.id, plat.name)
is designed to return either None or a single ScriptingElement. Without modifying thefind_matching_scripting
method to return a list as you proposed, we will not encounter multiple matches in this context.
f8bf516
to
ab7a80d
Compare
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.
the multiple match problem using the override_script
attribute seems not addressed yet
if self.scripting: | ||
# Check if at least one provided script met the conditions. | ||
# Summarize logs for all calls. | ||
was_script_matched = False | ||
for instance in self.instances.values(): | ||
was_script_matched = was_script_matched or self.handle_additional_scripts(instance.platform.name, instance.testsuite.id) | ||
|
||
if not was_script_matched: | ||
logger.info("Scripting list was provided, none of the specified conditions were met") |
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.
if self.scripting: | |
# Check if at least one provided script met the conditions. | |
# Summarize logs for all calls. | |
was_script_matched = False | |
for instance in self.instances.values(): | |
was_script_matched = was_script_matched or self.handle_additional_scripts(instance.platform.name, instance.testsuite.id) | |
if not was_script_matched: | |
logger.info("Scripting list was provided, none of the specified conditions were met") | |
if self.scripting and not any([self.handle_additional_scripts(inst_) for inst_ in self.instances.values()]): | |
logger.warning("Scripting list was provided, none of the specified conditions were met") |
def handle_additional_scripts(self, platform_name: Platform, testsuide: TestInstance): | ||
matched_scripting = self.scripting.get_matched_scripting(testsuide, platform_name) | ||
if matched_scripting: |
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.
def handle_additional_scripts(self, platform_name: Platform, testsuide: TestInstance): | |
matched_scripting = self.scripting.get_matched_scripting(testsuide, platform_name) | |
if matched_scripting: | |
def handle_additional_scripts(self, platform_name: Platform, testsuide: TestInstance): | |
matched_scripting = self.scripting.get_matched_scripting(testsuide, platform_name) | |
if matched_scripting: |
def handle_additional_scripts(self, platform_name: Platform, testsuide: TestInstance): | |
matched_scripting = self.scripting.get_matched_scripting(testsuide, platform_name) | |
if matched_scripting: | |
def handle_additional_scripts(self, platform_name: Platform, test_instance: TestInstance): | |
platform_name = test_instance.platform.name | |
matched_scripting = self.scripting.get_matched_scripting(test_instance.testsuite.id, platform_name) | |
if not matched_scripting: | |
return False |
continue | ||
if element.platforms and not _matches_element(platform, element.re_platforms): | ||
continue | ||
return element |
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 still looks for only one first matching pattern combination - see all my previous comments on this problem, eg. #72808 (comment)
# If a script object is provided, check if the script path is a valid file | ||
if script_obj and script_obj.get('path'): | ||
# Check if there's an existing script and if override is not allowed | ||
if not script_obj.get('override_script'): |
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'm puzzled - it seems the override_script
must be always present in the config otherwise the script will never considered irregardless of match duplications.
This attribute should be taken into account at get_matched_scripting()
as the better and the only one choice among others.
https://github.com/zephyrproject-rtos/zephyr/pull/72808/files#diff-cf21072267d967b317067e8df4b6946aada828f0dec1e7705505100e36b4d43fR1271-R1274
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.
your suggestions was implemented. Now entry must have override_script set to true to be executed and if there are duplicates job will be stopped
@majunkier would update this PR, I think there are only some code snippet changes by now. Thanks. Am I right? @golowanow |
ab7a80d
to
6850ab1
Compare
@majunkier please rebase |
d681d01
to
2096de5
Compare
@majunkier can you add/update a testcase to example the usage? |
|
||
# Checks if the given element matches any of the provided regex patterns. | ||
def _matches_element(element: str, patterns: list[re.Pattern]) -> bool: | ||
return any(pattern.match(element) for pattern in patterns) |
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.
regexp matching should be mentioned in twister.rst
continue | ||
matched_elements.append(element) | ||
|
||
# Check for override_script |
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 seems that this function allow duplicate entries all without override
attribute, doesn't it ? so it is still a problem with duplicates and their precedence. Override
should help when (1) there are duplicate matches, and (2) there is only one with override
among them to choose it. All other situations for several matched_elements (several overrides, have no overrides) are conflicts and configuration error.
Also, It is an overcomplication to have override_script
for each of 3 scripts in an entry when duplicates are possible with (platform, scenario) key. Much easier and clear is to have the override
attribute for an element.
Let's resolve this design problem first as the main reason for my -1 in this PR for quite a while.
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.
After the request to include the override_script parameter code was updated. Now, if override_script is not set to true, the script will not run. Additionally, if there are duplicates (e.g., two different scripts for the same board with override_script set to true), no script will be executed.
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.
To move forward I'd suggest: 1) don't use regexp matching at _matches_element()
, 2) just fail on any duplicate found, so forget the idea of override
attribute.
It will simplify the proposed change significantly, whereas keep most of its functionality.
Until this solution is merged, I can't provide a real usage example. However, I can share that I intend to use this approach for resetting the environment before a specific test is executed. This is particularly useful in cases where a test might crash the board, as it allows for a cleanup script to restore the environment prior to running the next test. One example I shared earlier relates to power management tests. This solution can be used in various ways, it is flexible and mainly could be used for setting up or resetting test environment. |
commands specified via other sources. | ||
|
||
The scripting YAML should consist of a series of dictionaries, | ||
each containing the keys scenarios, ``scenarios``, ``platforms``, |
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.
each containing the keys scenarios, ``scenarios``, ``platforms``, | |
each containing the keys: ``scenarios``, ``platforms``, |
was_script_matched = False | ||
for instance in self.instances.values(): | ||
was_script_matched = ( | ||
was_script_matched | ||
or self.handle_additional_scripts(instance.platform.name, instance) | ||
) |
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
was_script_matched = False | |
for instance in self.instances.values(): | |
was_script_matched = ( | |
was_script_matched | |
or self.handle_additional_scripts(instance.platform.name, instance) | |
) | |
was_script_matched = any(self.handle_additional_scripts(instance.platform.name, instance) for instance in self.instances.values()) |
def validate_boards(platform_scope, platform_from_yaml): | ||
return any(board in platform_scope for board in platform_from_yaml) |
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.
looks like this check can be called only once, right ?
def validate_boards(platform_scope, platform_from_yaml): | |
return any(board in platform_scope for board in platform_from_yaml) | |
if not any(board in platform_name for board in matched_scripting.platforms): | |
return False |
if dut.platform in platform_name and validate_boards( | ||
platform_name, matched_scripting.platforms | ||
): |
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.
if dut.platform in platform_name and validate_boards( | |
platform_name, matched_scripting.platforms | |
): | |
if dut.platform in platform_name: |
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 was asked to enhance security to ensure that users are fully aware when using the --scripting-list parameter. To address this, I have introduced parameter override_scripts
which manages the redeclaration of provided scripts. Your concerns about _matches_elemet function and duplicates, lets clear how its work based on examples :
-
example_script.sh
is provided from command line via parameter-pre-script ./example_script.sh
or from map.file
_matches_elemet
is not called, script executed :example_script.sh
-
example_script.sh
is provided from command line via parameter-pre-script ./example_script.sh
or from map.file other_script.sh is provided with --scripting-list
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script.sh
comment: >
<COMMENT>
there will be no script executed for scenario " EXAMPLE_SCENARIO " on platform "EXAMPLE_PLATFORM", there will be log message pre_script will not be overridden on EXAMPLE_PLATFORM
and none script will be executed, on other scenarios and boards -pre-script ./example_script.sh
will be executed
example_script.sh
is provided from command line via parameter-pre-script ./example_script.sh
or from map.file
other_script.sh is provided with --scripting-list
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script.sh
**override_script: true**
comment: >
<COMMENT>
script /example_script.sh
will be called only for scenario EXAMPLE_SCENARIO on platform EXAMPLE_PLATFORM, other configurations will be executed with -pre-script ./example_script.sh
- only --scripting-list is provided with duplicate without override_script :
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script1.sh
comment: >
<COMMENT>
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script2.sh
comment: >
<COMMENT>
there will be log message that none of this scripts will be executed
- only --scripting-list is provieded with duplicated override_script :
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script1.sh
**override_script: true**
comment: >
<COMMENT>
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script2.sh
**override_script: true**
comment: >
<COMMENT>
Execution will be stopped with log message
- only --scripting-list is provieded with duplicated records (only one override_script is declared) :
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script1.sh
comment: >
<COMMENT>
- scenarios:
- <EXAMPLE_SCENARIO>
platforms:
- <EXAMPLE_PLATFORM>
pre_script:
path: /example_script2.sh
**override_script: true**
comment: >
<COMMENT>
/example_script2.sh will be executed for scenario EXAMPLE_SCENARIO for platform
Handling duplicates works, i see that you aware about Input sources for scenarios and platforms could be not controlled cause of regexp, but data for _matches_script
are loaded and validated form schema scripting_schema
using the scl.yaml_load_verify
function - this ensures that the input conforms to known structure, load_from_yaml()
function - watch for invalid regex pattern is encountered if yes, exception will be raised.
continue | ||
matched_elements.append(element) | ||
|
||
# Check for override_script |
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.
To move forward I'd suggest: 1) don't use regexp matching at _matches_element()
, 2) just fail on any duplicate found, so forget the idea of override
attribute.
It will simplify the proposed change significantly, whereas keep most of its functionality.
51be920
to
38462e9
Compare
Add the execution of external scripts at precise moments. These scripts can be strategically deployed in three distinct phases: pre-script, post-flash-script and post-script. This functionality could help configuring the environment optimally before testing. Signed-off-by: Mateusz Junkier <[email protected]>
Add unit tests designed for testing additional scripting feature. Signed-off-by: Mateusz Junkier <[email protected]>
38462e9
to
826aa69
Compare
|
||
# Checks if the given element matches any of the provided patterns. | ||
def _matches_element(element: str, patterns: list[str]) -> bool: | ||
return any(pattern in element for pattern in patterns) |
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.
return any(pattern in element for pattern in patterns) | |
return any(pattern == element for pattern in patterns) |
otherwise when an element has e.g. just 'mec' in platforms it will match like *mec*
, isn't it ?
override_scripts = [ | ||
elem | ||
for elem in matched_elements | ||
if ( | ||
(elem.pre_script and elem.pre_script.override_script) | ||
or (elem.post_flash_script and elem.post_flash_script.override_script) | ||
or (elem.post_script and elem.post_script.override_script) | ||
) | ||
] | ||
|
||
if len(override_scripts) > 1: | ||
logger.error("Multiple override definition for scripts found") | ||
sys.exit(1) | ||
elif len(override_scripts) == 1: | ||
return override_scripts[0] | ||
elif matched_elements: | ||
return matched_elements[0] | ||
|
||
return None |
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.
override_scripts = [ | |
elem | |
for elem in matched_elements | |
if ( | |
(elem.pre_script and elem.pre_script.override_script) | |
or (elem.post_flash_script and elem.post_flash_script.override_script) | |
or (elem.post_script and elem.post_script.override_script) | |
) | |
] | |
if len(override_scripts) > 1: | |
logger.error("Multiple override definition for scripts found") | |
sys.exit(1) | |
elif len(override_scripts) == 1: | |
return override_scripts[0] | |
elif matched_elements: | |
return matched_elements[0] | |
return None | |
if not matched_elements: | |
return None | |
elif len(matched_elements) == 1: | |
return matched_elements[0] | |
override_scripts = [ | |
elem | |
for elem in matched_elements if (elem.override_script) | |
] | |
if len(override_scripts) == 1: | |
return override_scripts[0] | |
raise ConfigurationError("Multiple override definition for scripts found", str(override_scripts)) |
this way it should allow only:
- no match
- single match
- single override attributed element among duplicate matches
"override_script": | ||
type: bool | ||
default: false | ||
required: false |
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.
"override_script": | |
type: bool | |
default: false | |
required: false |
"override_script": | ||
type: bool | ||
default: false | ||
required: false |
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.
"override_script": | |
type: bool | |
default: false | |
required: false |
"override_script": | ||
type: bool | ||
default: false | ||
required: false |
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.
one level up
"override_script": | |
type: bool | |
default: false | |
required: false | |
"override_script": | |
type: bool | |
default: false | |
required: false |
Add the execution of external scripts at precise moments. These scripts can be strategically deployed in three distinct phases: pre-script, post-flash-script and post-script. This functionality could help configuring the environment optimally before testing.
Script phases are already implemented solution, this PR serves as an extension to broaden their application and utility.
Usage is similar to quarantine file :
twister -p <BOARD> -T <TEST_SCENARIO> --scripting-list <PATH_TO_SCRIPTING_YAML>
An example of entries in a scripting list yaml:
Usage of all (pre_script, post_flash_script, post_script) is not mandatory, it could be only one parameter.
One of the primary benefits of this approach is the ability to execute scripts tailored to individual scenarios.