Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 50 additions & 0 deletions doc/develop/test/twister.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1360,6 +1360,56 @@ using an external J-Link probe. The ``probe_id`` keyword overrides the
runner: jlink
serial: null

Additional Scripts
++++++++++++++++++

Twister offers users the flexibility to automate 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.

To leverage the scripting capability, users must append the argument
golowanow marked this conversation as resolved.
Show resolved Hide resolved
``--scripting-list <PATH_TO_SCRIPTING_LIST_YAML>`` to a twister call.
Parameter ``override_script`` added to explicitly confirm the intent
to execute the specified script. When set to true, this flag allow to
override ``--pre_script``, ``--post_flash_script``, ``--post_script``
commands specified via other sources.

The scripting YAML should consist of a series of dictionaries,
each containing the keys scenarios, ``scenarios``, ``platforms``,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
each containing the keys scenarios, ``scenarios``, ``platforms``,
each containing the keys: ``scenarios``, ``platforms``,

``pre_script``, ``post_flash_script``, ``post_script``. Each script
is defined by ``path`` representing path to script, ``timeout`` optional
integer specifying the maximum duration allowed for the script execution,
and ``override_script``. These keys define the specific combinations
of scenarios and platforms, as well as the corresponding scripts
to be executed at each stage. Additionally, it is mandatory
to include a comment entry ``comment`` which is used to give more
golowanow marked this conversation as resolved.
Show resolved Hide resolved
details about scripts and purpose of use.

An example of entries in a scripting list yaml:

.. code-block:: yaml

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

Using Single Board For Multiple Variants
++++++++++++++++++++++++++++++++++++++++

Expand Down
14 changes: 14 additions & 0 deletions scripts/pylib/twister/twisterlib/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,16 @@ def add_parse_arguments(parser = None) -> argparse.ArgumentParser:
help="""quit twister once there is build / run failure
""")

parser.add_argument(
"--scripting-list",
action="append",
metavar="YAML_FILE",
help="YAML configuration file with device handler hooks to run additional "
"pre-/post- flash phase scripts for selected platform and test scenario combinations. "
"The file must comply with `scripting-schema.yaml`. "
"Overrides `--pre-script` and `--hardware-map` settings. "
"Requires `--device-testing`")

parser.add_argument(
"--report-name",
help="""Create a report with a custom name.
Expand Down Expand Up @@ -922,6 +932,10 @@ def parse_arguments(
)
sys.exit(1)

if options.scripting_list and not options.device_testing:
logger.error("When --scripting_list is used --device-testing is required")
sys.exit(1)

if (
options.device_testing
and (options.device_serial or options.device_serial_pty) and len(options.platform) != 1
Expand Down
141 changes: 141 additions & 0 deletions scripts/pylib/twister/twisterlib/scripting.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
# Copyright (c) 2024 Intel Corporation.
# SPDX-License-Identifier: Apache-2.0

from __future__ import annotations

import logging
import sys
from dataclasses import dataclass, field
from pathlib import Path

import scl

logger = logging.getLogger('twister')


# Handles test scripting configurations.
class Scripting:
def __init__(self, scripting_files: list[Path | str], scripting_schema: dict) -> None:
self.scripting = ScriptingData()
self.scripting_files = scripting_files or []
self.scripting_schema = scripting_schema
self.load_and_validate_files()

# Finds and returns the scripting element that matches the given test name and platform.
def get_matched_scripting(self, testname: str, platform: str) -> ScriptingElement | None:
matched_scripting = self.scripting.find_matching_scripting(testname, platform)
if matched_scripting:
logger.debug(
f"'{testname}' on '{platform}' device handler scripts '{str(matched_scripting)}'"
)
return matched_scripting
return None

def load_and_validate_files(self):
for scripting_file in self.scripting_files:
self.scripting.extend(
ScriptingData.load_from_yaml(scripting_file, self.scripting_schema)
)


@dataclass
class Script:
path: str | None = None
timeout: int | None = None
override_script: bool = False


@dataclass
# Represents a single scripting element with associated scripts and metadata.
class ScriptingElement:
scenarios: list[str] = field(default_factory=list)
platforms: list[str] = field(default_factory=list)
pre_script: Script | None = None
post_flash_script: Script | None = None
post_script: Script | None = None
comment: str = 'NA'

# Ensures all required scripts are present and validates the element.
def __post_init__(self):
if not any([self.pre_script, self.post_flash_script, self.post_script]):
logger.error("At least one of the scripts must be specified")
sys.exit(1)
self.pre_script = self._convert_to_script(self.pre_script)
self.post_flash_script = self._convert_to_script(self.post_flash_script)
self.post_script = self._convert_to_script(self.post_script)

# Converts a dictionary to a Script instance if necessary.
def _convert_to_script(self, script: dict | Script | None) -> Script | None:
if isinstance(script, dict):
return Script(**script)
return script


@dataclass
# Holds a collection of scripting elements.
class ScriptingData:
elements: list[ScriptingElement] = field(default_factory=list)

# Ensures all elements are ScriptingElement instances.
def __post_init__(self):
self.elements = [
elem if isinstance(elem, ScriptingElement) else ScriptingElement(**elem)
for elem in self.elements
]

@classmethod
# Loads scripting data from a YAML file.
def load_from_yaml(cls, filename: Path | str, schema: dict) -> ScriptingData:
try:
raw_data = scl.yaml_load_verify(filename, schema) or []
return cls(raw_data)
except scl.EmptyYamlFileException:
logger.error(f'Scripting file {filename} is empty')
golowanow marked this conversation as resolved.
Show resolved Hide resolved
sys.exit(1)
except FileNotFoundError:
logger.error(f'Scripting file {filename} not found')
sys.exit(1)
except Exception as e:
logger.error(f'Error loading {filename}: {e}')
sys.exit(1)

# Extends the current scripting data with another set of scripting data.
def extend(self, other: ScriptingData) -> None:
self.elements.extend(other.elements)
Copy link
Member

Choose a reason for hiding this comment

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

Without duplication check, the resulting match result will be difficult to rely on a real-world config, especially with regular expressions on both scenarios and platforms.


# Finds a scripting element that matches the given scenario and platform.
def find_matching_scripting(self, scenario: str, platform: str) -> ScriptingElement | None:
matched_elements = []

for element in self.elements:
if element.scenarios and not _matches_element(scenario, element.scenarios):
continue
if element.platforms and not _matches_element(platform, element.platforms):
continue
matched_elements.append(element)

# Check for override_script
Copy link
Member

@golowanow golowanow Dec 10, 2024

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.

Copy link
Collaborator Author

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.

Copy link
Member

@golowanow golowanow Dec 16, 2024

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.

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
Comment on lines +118 to +136
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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



# 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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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 ?

87 changes: 87 additions & 0 deletions scripts/pylib/twister/twisterlib/testplan.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from twisterlib.error import TwisterRuntimeError
from twisterlib.platform import Platform
from twisterlib.quarantine import Quarantine
from twisterlib.scripting import Scripting
from twisterlib.statuses import TwisterStatus
from twisterlib.testinstance import TestInstance
from twisterlib.testsuite import TestSuite, scan_testsuite_path
Expand Down Expand Up @@ -94,6 +95,10 @@ class TestPlan:
os.path.join(ZEPHYR_BASE,
"scripts", "schemas", "twister", "quarantine-schema.yaml"))

scripting_schema = scl.yaml_load(
os.path.join(ZEPHYR_BASE,
"scripts", "schemas", "twister", "scripting-schema.yaml"))

tc_schema_path = os.path.join(
ZEPHYR_BASE,
"scripts",
Expand All @@ -113,6 +118,7 @@ def __init__(self, env: Namespace):
# Keep track of which test cases we've filtered out and why
self.testsuites = {}
self.quarantine = None
self.scripting = None
self.platforms = []
self.platform_names = []
self.selected_platforms = []
Expand Down Expand Up @@ -225,6 +231,11 @@ def discover(self):
logger.debug(f'Quarantine file {quarantine_file} is empty')
self.quarantine = Quarantine(ql)

# handle extra scripts
sl = self.options.scripting_list
if sl:
self.scripting = Scripting(sl, self.scripting_schema)

def load(self):

if self.options.report_suffix:
Expand Down Expand Up @@ -265,6 +276,21 @@ def load(self):
else:
self.apply_filters()

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)
)
Comment on lines +282 to +287
Copy link
Member

Choose a reason for hiding this comment

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

it can be

Suggested change
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())


if not was_script_matched:
logger.info(
"Scripting list was provided, none of the specified conditions were met"
)

if self.options.subset:
s = self.options.subset
try:
Expand Down Expand Up @@ -1307,6 +1333,67 @@ def _create_build_dir_link(self, links_dir_path, instance):

self.link_dir_counter += 1

def handle_additional_scripts(
self, platform_name: str, testsuite: TestInstance
) -> bool:
logger.debug(testsuite.testsuite.id)
matched_scripting = self.scripting.get_matched_scripting(
testsuite.testsuite.id, platform_name
)
if matched_scripting:
# Define a function to validate
# if the platform is supported by the matched scripting
def validate_boards(platform_scope, platform_from_yaml):
return any(board in platform_scope for board in platform_from_yaml)
Comment on lines +1346 to +1347
Copy link
Member

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 ?

Suggested change
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


# Define the types of scripts we are interested in as a set
script_types = {
"pre_script": "pre_script_timeout",
"post_flash_script": "post_flash_timeout",
"post_script": "post_script_timeout",
}

# 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 in platform_name and validate_boards(
platform_name, matched_scripting.platforms
):
Comment on lines +1361 to +1363
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if dut.platform in platform_name and validate_boards(
platform_name, matched_scripting.platforms
):
if dut.platform in platform_name:

Copy link
Collaborator Author

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 :

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

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

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

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

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

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

for script_type, script_timeout in script_types.items():
# Get the script object from matched_scripting
script_obj = getattr(matched_scripting, script_type, None)
# If a script object is provided, check if the script path is a valid file
if script_obj and script_obj.path:
# Check if there's an existing script and if override is not allowed
if not script_obj.override_script:
logger.info(
f"{script_type} will not be overridden on {platform_name}."
)
continue
# Check if the script path is a valid file and set it on the DUT
if Path(script_obj.path).is_file():
setattr(dut, script_type, script_obj.path)
# Check if the script timeout is provided and set it on the DUT
if script_obj.timeout is not None:
setattr(dut, script_timeout, script_obj.timeout)
logger.info(
f"{script_type} {script_obj.path} will be executed on "
f"{platform_name} with timeout {script_obj.timeout}"
)
else:
logger.info(
f"{script_type} {script_obj.path} will be executed on "
f"{platform_name} with no timeout specified"
)
else:
raise TwisterRuntimeError(
f"{script_type} script not found under path: {script_obj.path}"
)
return True
return False
golowanow marked this conversation as resolved.
Show resolved Hide resolved


def change_skip_to_error_if_integration(options, instance):
''' All skips on integration_platforms are treated as errors.'''
Expand Down
Loading
Loading