Twister: additional scripts #72808

majunkier · 2024-05-15T14:09:11Z

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:

    - scenarios:
    - sample.basic.helloworld
    platforms:
    - frdm_k64f
    pre_script:
        path: <PATH_TO_PRE_SCRIPT>
        timeout: <TIMEOUT TIME>
        override_script: <BOOLEAN>
    post_flash_script:
        path: <PATH_TO_PRE_SCRIPT>
        timeout: <TIMEOUT TIME>
        override_script: <BOOLEAN>
    post_script:
        path: <PATH_TO_PRE_SCRIPT>
        timeout: <TIMEOUT TIME>
        override_script: <BOOLEAN>
    comment:
        Testing extra scripts feature

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.

evgeniy-paltsev · 2024-05-15T20:05:10Z

doc/develop/test/twister.rst

+        pre_script:
+            <PATH_TO_PRE_SCRIPT>
+        post_flash_script:
+            <PATH_TO_POST_FLASH_SCRIPT>
+        post_script:
+            <PATH_TO_POST_SCRIPT>
+        comment:
+            Testing extra scripts


Are we planning to use it for tests in Zephyr tree?

Solution is designed with flexibility in mind, making it suitable for use in a variety of contexts, including the Zephyr tree.

hakehuang · 2024-05-16T04:18:44Z

why no just put to the map.yaml file? along with --device-testing?

majunkier · 2024-05-16T08:48:58Z

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.
By this approach its easy to execute scripts tailored to individual scenarios.

hakehuang · 2024-05-16T10:55:23Z

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.
By this approach its easy to execute scripts tailored to individual scenarios.

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?

majunkier · 2024-05-16T12:04:28Z

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.

hakehuang · 2024-05-16T12:42:41Z

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.

majunkier · 2024-05-29T07:59:30Z

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

majunkier · 2024-06-07T13:27:12Z

@evgeniy-paltsev could you take a look?

doc/develop/test/twister.rst

scripts/pylib/twister/twisterlib/testplan.py

scripts/pylib/twister/twisterlib/environment.py

scripts/pylib/twister/twisterlib/testplan.py

golowanow · 2024-06-08T12:47:54Z

scripts/pylib/twister/twisterlib/testplan.py

+                                    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)


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.

this is explained in twister.rst, you manage this which script will be executed so you don't need adding extra parameter

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.

golowanow · 2024-06-08T13:49:21Z

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. By this approach its easy to execute scripts tailored to individual scenarios.

@majunkier what about these alternative improvements then:

extend test-config-schema with additional attributes instead of introducing the separate scripting-schema ?
run twister several times with different command line options for a test scope, platforms and scripts (+ missed options) ?

github-actions · 2024-08-08T00:29:41Z

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.

hakehuang · 2024-08-19T09:17:35Z

@majunkier do you have time to follow up with @golowanow comments? besides that this PR looks ok.

majunkier · 2024-08-21T06:41:55Z

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. By this approach its easy to execute scripts tailored to individual scenarios.

@majunkier what about these alternative improvements then:

extend test-config-schema with additional attributes instead of introducing the separate scripting-schema ?

run twister several times with different command line options for a test scope, platforms and scripts (+ missed options) ?

This works as quarantine list file - reduce complexity of test-config-schema, easier to manage, test-config-schema contains the structure and rules for how tests should be written and this is extra action without rules
There are few tests that check functionality

golowanow

@majunkier, sorry for so many comments, it took a while to review.

scripts/schemas/twister/scripting-schema.yaml

scripts/pylib/twister/twisterlib/testplan.py

scripts/pylib/twister/twisterlib/scripting.py

golowanow · 2024-09-06T15:29:36Z

scripts/pylib/twister/twisterlib/testplan.py

+                            return any(board in platform_scope for board in platform_from_yaml)
+
+                        # Define the types of scripts we are interested in
+                        script_types = {'pre_script': 'pre_script','post_flash_script': 'post_flash_script', 'post_script': 'post_script' }


why it needs a dictionary with keys == values, not just a list ?

updated, now added timeout which will use dict.
Before it was part of old code with you pointed

golowanow · 2024-09-06T15:42:11Z

scripts/pylib/twister/twisterlib/testplan.py

+                                    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)


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.

scripts/pylib/twister/twisterlib/testplan.py

PerMac · 2024-09-16T08:55:34Z

I saw @hakehuang asking about it, but haven't seen an answer: isn't using pytest for such scripting already a more general solution with the ease of expandability with fixtures?

majunkier · 2024-09-16T10:47:51Z

I saw @hakehuang asking about it, but haven't seen an answer: isn't using pytest for such scripting already a more general solution with the ease of expandability with fixtures?

Indeed, there are numerous methods to attain the same or comparable solutions. However, this solution emphasizes enhancing the core functionality of Twister, making it more apt for testing within the Zephyr ecosystem.

hakehuang · 2024-09-19T07:09:32Z

Indeed, there are numerous methods to attain the same or comparable solutions. However, this solution emphasizes enhancing the core functionality of Twister, making it more apt for testing within the Zephyr ecosystem.

I do support to add such case by case control method, however, as @PerMac and @golowanow suggested, maybe we need consider to use pytest instead of on the fly shell scripts, as those scripts are not likely be maintained in tree, this diverge may introduce inconsistency to test result. so for your case, would you try use pytest to organize your logic and which will be in-tree and benefit for all other developers. or you find it is impossible to do in pytest?

majunkier · 2024-09-23T06:36:06Z

Indeed, there are numerous methods to attain the same or comparable solutions. However, this solution emphasizes enhancing the core functionality of Twister, making it more apt for testing within the Zephyr ecosystem.

I do support to add such case by case control method, however, as @PerMac and @golowanow suggested, maybe we need consider to use pytest instead of on the fly shell scripts, as those scripts are not likely be maintained in tree, this diverge may introduce inconsistency to test result. so for your case, would you try use pytest to organize your logic and which will be in-tree and benefit for all other developers. or you find it is impossible to do in pytest?

This change does not affect pytest as a plugin, it is an extension to the basic twister implementation. The functionality already exists, this changes are adding an additional extension for the currently working solution.

majunkier · 2024-09-23T10:26:26Z

Apart from review changes I have implemented additional parameters to the scripting schema:
'timeout': parameter to specify the duration for script execution. @hakehuang based on your changes
'override_script': a parameter that requires confirmation before proceeding to override and execute the existing script.

scripts/schemas/twister/scripting-schema.yaml

scripts/pylib/twister/twisterlib/scripting.py

golowanow · 2024-09-27T06:52:32Z

scripts/pylib/twister/twisterlib/scripting.py

+                continue
+            if element.platforms and not _matches_element(platform, element.re_platforms):
+                continue
+            return element


To deal with several matching entries situation I'd suggest to look over all elements and return a list with all matched scripting elements.

scripts/pylib/twister/twisterlib/testplan.py

golowanow · 2024-09-27T07:06:38Z

scripts/pylib/twister/twisterlib/testplan.py

+                    # 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):


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.

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_scriptingmethod to return a list as you proposed, we will not encounter multiple matches in this context.

scripts/pylib/twister/twisterlib/testplan.py

golowanow

the multiple match problem using the override_script attribute seems not addressed yet

golowanow · 2024-10-15T07:20:15Z

scripts/pylib/twister/twisterlib/testplan.py

+        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")


Suggested change

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")

golowanow · 2024-10-15T07:29:40Z

scripts/pylib/twister/twisterlib/testplan.py

+    def handle_additional_scripts(self, platform_name: Platform, testsuide: TestInstance):
+        matched_scripting = self.scripting.get_matched_scripting(testsuide, platform_name)
+        if matched_scripting:


Suggested change

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:

Suggested change

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

golowanow · 2024-10-15T07:30:47Z

scripts/pylib/twister/twisterlib/testplan.py

+            return True
+        return False


Suggested change

return True

return False

return True

golowanow · 2024-10-15T07:37:12Z

scripts/pylib/twister/twisterlib/scripting.py

+                continue
+            if element.platforms and not _matches_element(platform, element.re_platforms):
+                continue
+            return element


it still looks for only one first matching pattern combination - see all my previous comments on this problem, eg. #72808 (comment)

golowanow · 2024-10-15T07:48:16Z

scripts/pylib/twister/twisterlib/testplan.py

+                        # 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'):


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

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

hakehuang · 2024-10-22T15:25:57Z

@majunkier would update this PR, I think there are only some code snippet changes by now. Thanks. Am I right? @golowanow

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]>

zephyrbot added area: Twister Twister area: Documentation labels May 15, 2024

zephyrbot requested review from carlescufi, gchwier, golowanow, gopiotr, hakehuang, KamilxPaszkiet, kartben, LukaszMrugala, nashif and PerMac May 15, 2024 14:09

zephyrbot assigned nashif May 15, 2024

majunkier force-pushed the twister_additional_scripts branch 3 times, most recently from 70a6788 to 1aff18b Compare May 15, 2024 19:52

evgeniy-paltsev reviewed May 15, 2024

View reviewed changes

majunkier force-pushed the twister_additional_scripts branch from 1aff18b to a0b6ba1 Compare May 16, 2024 08:52

hakehuang previously approved these changes May 30, 2024

View reviewed changes

majunkier requested a review from evgeniy-paltsev June 4, 2024 10:13

golowanow requested changes Jun 8, 2024

View reviewed changes

zephyrbot requested review from jeremybettis and yperess August 19, 2024 07:23

github-actions bot removed the Stale label Aug 20, 2024

majunkier force-pushed the twister_additional_scripts branch 2 times, most recently from 8f057d8 to 62cd444 Compare August 26, 2024 08:54

majunkier requested a review from golowanow August 26, 2024 08:58

golowanow requested changes Sep 6, 2024

View reviewed changes

majunkier force-pushed the twister_additional_scripts branch 2 times, most recently from ac28429 to 9c50aac Compare September 23, 2024 07:16

hakehuang approved these changes Sep 23, 2024

View reviewed changes

hakehuang previously approved these changes Sep 23, 2024

View reviewed changes

majunkier requested a review from golowanow September 24, 2024 12:23

golowanow requested changes Sep 27, 2024

View reviewed changes

majunkier dismissed hakehuang’s stale review via f8bf516 October 8, 2024 09:30

majunkier force-pushed the twister_additional_scripts branch 2 times, most recently from f8bf516 to ab7a80d Compare October 14, 2024 10:14

majunkier requested a review from golowanow October 14, 2024 10:15

golowanow requested changes Oct 15, 2024

View reviewed changes

majunkier added 2 commits November 29, 2024 11:29

twister: add tests for additional scripts feature

6850ab1

Add unit tests designed for testing additional scripting feature. Signed-off-by: Mateusz Junkier <[email protected]>

majunkier force-pushed the twister_additional_scripts branch from ab7a80d to 6850ab1 Compare November 29, 2024 11:20

majunkier requested a review from golowanow November 29, 2024 20:35

Twister: additional scripts #72808

Are you sure you want to change the base?

Twister: additional scripts #72808

Conversation

majunkier commented May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakehuang commented May 16, 2024

majunkier commented May 16, 2024 • edited Loading

hakehuang commented May 16, 2024

majunkier commented May 16, 2024 • edited Loading

hakehuang commented May 16, 2024 • edited Loading

majunkier commented May 29, 2024

majunkier commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

golowanow commented Jun 8, 2024

github-actions bot commented Aug 8, 2024

hakehuang commented Aug 19, 2024

majunkier commented Aug 21, 2024

golowanow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

PerMac commented Sep 16, 2024

majunkier commented Sep 16, 2024

hakehuang commented Sep 19, 2024

majunkier commented Sep 23, 2024

majunkier commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golowanow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakehuang commented Oct 22, 2024

majunkier commented May 15, 2024 •

edited

Loading

majunkier commented May 16, 2024 •

edited

Loading

majunkier commented May 16, 2024 •

edited

Loading

hakehuang commented May 16, 2024 •

edited

Loading

golowanow Sep 6, 2024 •

edited

Loading

golowanow Sep 6, 2024 •

edited

Loading