twister: add instance name as default parameter #81389

hakehuang · 2024-11-14T09:58:55Z

add testinstance name as default parameter to
customer scripts

golowanow · 2024-11-15T12:22:21Z

scripts/pylib/twister/twisterlib/handlers.py

@@ -694,7 +694,10 @@ def handle(self, harness):
            timeout = 30
            if script_param:
                timeout = script_param.get("pre_script_timeout", timeout)
-            self.run_custom_script(pre_script, timeout)
+            pre_script_cmd = ([pre_script] +


what if with this change we also allow these *script properties to convey a script command line with parameters,
not just a script file path as it is now, but doing like this

pre_script_cmd = (pre_script.split() + [self.instance.name] )

Maybe instead of passing the instance name every time to those scripts, better to allow user to add custom parameters in hardware map, e.g.
pre_script: '<PATH>/script.py --input platform_name'
and better to modify run_custom_script method:

just add shell=True to subprocess.Popen

or use shlex.split(script) to safely split input parameters (to avoid issues with shell injections)

If you add additional parameter to every call, you definitely must to document it (as @golowanow requested), because some of downstream users can use custom scripts that do not allow additional parameters

platform name is not enough, most of time we need to know the testinstance name to decide how to run pre_script.

shell-True sometime does not work well in a cloud system.

most of time we need to know the testinstance name to decide how to run pre_script.

I still don't think it is good idea to pass this info every time to the pre/post scripts. If you need something extra in your internal scripts, you can just read it from build folder (name of directory consists a platform name and test name ... or just take it from CMakeCache.txt or other build artefact).

shell-True sometime does not work well in a cloud system.

try to use split to pass a list to subprocess

if we have 8 instance running, how do I know which one is the current directory to parse?

I still don't think it is good idea to pass this info every time to the pre/post scripts. If you need something extra in your internal scripts, you can just read it from build folder (name of directory consists a platform name and test name ... or just take it from CMakeCache.txt or other build artefact).

By parsing the current build system to get the instance name, this costs more. and the instance name is so far the only thing we need to identify, as we know the platform + instance_name, it will uniquely identify the test case context.

golowanow · 2024-11-15T12:28:44Z

scripts/pylib/twister/twisterlib/handlers.py

@@ -694,7 +694,10 @@ def handle(self, harness):
            timeout = 30
            if script_param:
                timeout = script_param.get("pre_script_timeout", timeout)
-            self.run_custom_script(pre_script, timeout)
+            pre_script_cmd = ([pre_script] +


please document this new behavior for --pre-script command

https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/pylib/twister/twisterlib/environment.py#L627-L630

add testinstance name as default parameter to customer scripts Signed-off-by: Hake Huang <[email protected]>

update test_handler for new parameters Signed-off-by: Hake Huang <[email protected]>

No scripts will have a default parameter testinstance.name also add twister document for scripts Signed-off-by: Hake Huang <[email protected]>

golowanow · 2024-11-26T20:09:48Z

doc/develop/test/twister.rst

+        "post_script_timeout": 100
+        "post_flash_timeout":  100
+
+also a default patameter <testinstance.name> will be pass as fist parameter for scripts


@hakehuang, to be on the same page: why you need the instance name to pass to scripts ?

btw. it is platform/suite_path/suite_name where platform prefix is not 'normalized' (it is in HWMv2 format), whereas suite_path might be excluded with --no-detailed-test-id option.

we have a platform, for security reasons, it need pre-load different security blobs to board first, before run test. @golowanow .

btw. it is platform/suite_path/suite_name where platform prefix is not 'normalized' (it is in HWMv2 format), whereas suite_path might be excluded with --no-detailed-test-id option.

it is not an issue at my case, as per_script is always defined in platform level, so we know which platform it is running already. and suite_path is not need usually, as pre_script does not always go with the build but run

it is not an issue at my case, as per_script is always defined in platform level, so we know which platform it is running already. and suite_path is not need usually, as pre_script does not always go with the build but run

so in your use case the parameter it is just for suite_name actually ?

well, I think it is a quite frequent situation when the same flash- script is re-used with different platforms/variants (currently that can be solved creating links to the same script file with different names), and the better and general solution would be to call these scripts with a basic set of expected parameters (like what Twister has --platform, --scenario, etc.) in addition to the command line from the map file the script is called with.

the platform information is already contained in the map.yaml, which pre/post scripts rely on. and this could be fixed to the only interface.

hakehuang · 2025-01-14T03:06:59Z

@golowanow @gchwier @LukaszMrugala @nashif , please help to review, this is to unified the post and pre script interface for different cases.

nashif

Please review and fix your commit messages, they have a few typos.
Also, please add more information and rationale in the commits and documentation how the test instance name will be used.

I do not think it is a good idea to rely on twister internal variables for the scripts (test instance name), this should be something that is visible at the test definition, i.e. yaml file, suite name, scenario etc. and not an internal twister variable that is subject to change and is not known at the time of writing the test.

nashif · 2025-01-17T12:56:43Z

doc/develop/test/twister.rst

+
+* post_flash_script: <path to pre script>
+
+each of above scripts can have timeout defined as below in hardwar map file by script_param


hardware map

hakehuang · 2025-01-19T14:07:39Z

there is another approach to use the yaml file to control the script, so I close this PR.

zephyrbot added the area: Twister Twister label Nov 14, 2024

zephyrbot requested review from gchwier, golowanow, KamilxPaszkiet, LukaszMrugala, nashif and PerMac November 14, 2024 09:59

zephyrbot assigned nashif Nov 14, 2024

golowanow reviewed Nov 15, 2024

View reviewed changes

hakehuang force-pushed the add_script_para branch from 35711ef to 93df5ec Compare November 18, 2024 06:47

zephyrbot added the area: Testsuite Testsuite label Nov 18, 2024

zephyrbot requested review from aaronemassey, asemjonovs, jeremybettis and yperess November 18, 2024 06:47

hakehuang force-pushed the add_script_para branch from 93df5ec to ad2417d Compare November 20, 2024 03:47

gchwier mentioned this pull request Nov 20, 2024

twister: Allow sharing hardware platform between variants #81465

Merged

hakehuang requested a review from golowanow November 26, 2024 15:10

hakehuang added 3 commits November 26, 2024 23:11

twister: add instance name as default parameter

f2686c1

add testinstance name as default parameter to customer scripts Signed-off-by: Hake Huang <[email protected]>

test: twister: update test_handler

8172513

update test_handler for new parameters Signed-off-by: Hake Huang <[email protected]>

twister: document: update document help for script

1dd532e

No scripts will have a default parameter testinstance.name also add twister document for scripts Signed-off-by: Hake Huang <[email protected]>

hakehuang force-pushed the add_script_para branch from ad2417d to 1dd532e Compare November 26, 2024 15:13

golowanow reviewed Nov 26, 2024

View reviewed changes

hakehuang requested a review from golowanow December 9, 2024 07:23

nashif requested changes Jan 17, 2025

View reviewed changes

hakehuang closed this Jan 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

twister: add instance name as default parameter #81389

twister: add instance name as default parameter #81389

hakehuang commented Nov 14, 2024

golowanow Nov 15, 2024

gchwier Nov 15, 2024

hakehuang Nov 17, 2024

gchwier Nov 20, 2024

hakehuang Nov 20, 2024

hakehuang Dec 9, 2024

golowanow Nov 15, 2024

hakehuang Nov 17, 2024

golowanow Nov 26, 2024 •

edited

Loading

hakehuang Nov 27, 2024

golowanow Nov 27, 2024

hakehuang Nov 27, 2024 •

edited

Loading

hakehuang commented Jan 14, 2025

nashif left a comment

nashif Jan 17, 2025

hakehuang commented Jan 19, 2025


		* post_flash_script: <path to pre script>

		each of above scripts can have timeout defined as below in hardwar map file by script_param

twister: add instance name as default parameter #81389

twister: add instance name as default parameter #81389

Conversation

hakehuang commented Nov 14, 2024

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

Choose a reason for hiding this comment

golowanow Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakehuang Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

hakehuang commented Jan 14, 2025

nashif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakehuang commented Jan 19, 2025

golowanow Nov 26, 2024 •

edited

Loading

hakehuang Nov 27, 2024 •

edited

Loading