Skip to content

twister: add instance name as default parameter #81389

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.

Sign up for GitHub

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

Closed
hakehuang wants to merge 3 commits into from
Closed

twister: add instance name as default parameter #81389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2686c1
twister: add instance name as default parameter
hakehuang Nov 14, 2024
8172513
test: twister: update test_handler
hakehuang Nov 14, 2024
1dd532e
twister: document: update document help for script
hakehuang Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions doc/develop/test/twister.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1315,6 +1315,38 @@ Using Single Board For Multiple Variants
runner: nrfjprog
serial: /dev/ttyACM1

Scripts Usage for platform
++++++++++++++++++++++++++

Below scripts are supported

* pre_script: <path to pre script>

* post_script: <path to pre script>

* post_flash_script: <path to pre script>

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

@nashif nashif Jan 17, 2025

Choose a reason for hiding this comment

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

hardware map


.. code-block:: yaml

- connected: false
id: 0229000005d9ebc600000000000000000000000097969905
platform: mimxrt1060_evk
probe_id: 000609301751
product: DAPLink CMSIS-DAP
runner: jlink
serial: null
pre_script: pre_scripts.sh
post_script: post_scripts.sh
post_flash_script: post_flash_scripts.sh
script_param:
"pre_script_timeout": 100
"post_script_timeout": 100
"post_flash_timeout": 100

also a default patameter <testinstance.name> will be pass as fist parameter for scripts
Copy link
Member

@golowanow golowanow Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@hakehuang hakehuang Nov 27, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@golowanow golowanow Nov 27, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hakehuang hakehuang Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Quarantine
++++++++++

Expand Down
3 changes: 2 additions & 1 deletion scripts/pylib/twister/twisterlib/environment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,8 @@ def add_parse_arguments(parser = None) -> argparse.ArgumentParser:

parser.add_argument("--pre-script",
help="""specify a pre script. This will be executed
before device handler open serial port and invoke runner.
before device handler open serial port and invoke runner,
and testinstance name will be its first parameter.
""")

parser.add_argument(
Expand Down
15 changes: 12 additions & 3 deletions scripts/pylib/twister/twisterlib/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,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] +
Copy link
Member

@golowanow golowanow Nov 15, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@gchwier gchwier Nov 15, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hakehuang hakehuang Nov 17, 2024

Choose a reason for hiding this comment

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

  1. platform name is not enough, most of time we need to know the testinstance name to decide how to run pre_script.
  2. shell-True sometime does not work well in a cloud system.

Copy link
Contributor

@gchwier gchwier Nov 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hakehuang hakehuang Nov 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hakehuang hakehuang Dec 9, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@golowanow golowanow Nov 15, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hakehuang hakehuang Nov 17, 2024

Choose a reason for hiding this comment

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

I will add

[self.instance.name]
)
self.run_custom_script(pre_script_cmd, timeout)

flash_timeout = hardware.flash_timeout
if hardware.flash_with_test:
Expand Down Expand Up @@ -782,7 +785,10 @@ def handle(self, harness):
timeout = 30
if script_param:
timeout = script_param.get("post_flash_timeout", timeout)
self.run_custom_script(post_flash_script, timeout)
post_flash_script_cmd = ([post_flash_script] +
[self.instance.name]
)
self.run_custom_script(post_flash_script_cmd, timeout)

# Connect to device after flashing it
if hardware.flash_before:
Expand Down Expand Up @@ -824,7 +830,10 @@ def handle(self, harness):
timeout = 30
if script_param:
timeout = script_param.get("post_script_timeout", timeout)
self.run_custom_script(post_script, timeout)
post_script_cmd = ([post_script] +
[self.instance.name]
)
self.run_custom_script(post_script_cmd, timeout)

self.make_dut_available(hardware)

Expand Down
8 changes: 4 additions & 4 deletions scripts/tests/twister/test_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1479,16 +1479,16 @@ def mock_popen(command, *args, **kwargs):
return

handler.run_custom_script.assert_has_calls([
mock.call('dummy pre script', mock.ANY)
mock.call(['dummy pre script', mock.ANY], mock.ANY)
])

if raise_create_serial:
return

handler.run_custom_script.assert_has_calls([
mock.call('dummy pre script', mock.ANY),
mock.call('dummy post flash script', mock.ANY),
mock.call('dummy post script', mock.ANY)
mock.call(['dummy pre script', mock.ANY], mock.ANY),
mock.call(['dummy post flash script', mock.ANY], mock.ANY),
mock.call(['dummy post script', mock.ANY], mock.ANY)
])

if expected_reason:
Expand Down
Loading