Read target connection settings from a YAML file #670

metin-arm · 2024-02-05T13:06:05Z

Commits:

PRs #667 and ARM-software/workload-automation#1248 can be merged after this one.

douglas-raillard-arm · 2024-02-12T11:18:36Z

devlib/utils/misc.py

@@ -37,6 +37,7 @@
 import types
 import warnings
 import wrapt
+import yaml


I can't remember the specifics but there is a fundamental design issue with that library. I used ruamel.yaml in Lisa instead. That might have to do with a mutable singleton used in the lib for something central that could conflict between different users in the same process.

I remember now, it's this pattern that is bonkers:

import yaml yaml.add_constructor()

So any other dependency can decide to change the behavior of pyyaml and everyone in the same process will get the change of behavior, devlib included. That may come from the underlying C library that this wraps but this is the best recipe for unsolvable issues.

Ruamel.yaml added a way to get some sort of context object and custom constructors are only registered against it. Every user has their own instance and no-one is stepping on each other's toes.

Thanks for the hint. Move from yaml to ruamel.

douglas-raillard-arm

That's not a deep review on the added code, just superficial comments

douglas-raillard-arm · 2024-02-19T15:17:55Z

devlib/utils/misc.py

@@ -590,6 +591,31 @@ def __str__(self):
        return message.format(self.filepath, self.lineno, self.message)


+def load_struct_from_yaml(filepath):


For now we don't want the testing infrastructure to be a public API, so this needs a non-public name starting with a single underscore (https://peps.python.org/pep-0008/)

load_struct_from_yaml() is not for testing purposes only.
It's just moved from WA to devlib: ARM-software/workload-automation#1248

douglas-raillard-arm · 2024-02-19T15:18:52Z

devlib/utils/misc.py

+    Args:
+        filepath (str): Input file which contains YAML data.
+
+    Raises:
+        LoadSyntaxError: if there is a syntax error in YAML data.
+
+    Returns:
+        dict: which contains parsed YAML data.


I would recommend using the autodoc style as this is the most native one with Sphinx (you can see what we use in LISA)

douglas-raillard-arm · 2024-02-19T15:21:08Z

devlib/utils/misc.py

+    except yaml.YAMLError as ex:
+        message = ex.message if hasattr(ex, 'message') else ''
+        lineno = ex.problem_mark.line if hasattr(ex, 'problem_mark') else None
+        raise LoadSyntaxError(message, filepath=filepath, lineno=lineno) from ex


I'm not sure what LoadSyntaxError was initially for. Currently it seems like dead code in devlib. Unless it really adds something, I'd recommend just letting the YAMLError bubble up.

LoadSyntaxError() is referenced in load_struct_from_python(), too.
I don't know the history, but apparently it makes the syntax error location more explicit, IMHO.

We use this in WA to try and provide a more friendly error message to clearly indicate the location of the syntax error.
Since this code is being moved, I'd vote for keeping the same error class.

Sounds good, I did not look beyond devlib codebase

douglas-raillard-arm · 2024-02-19T15:22:11Z

tests/test_target.py

@@ -14,17 +14,30 @@
 # limitations under the License.
 #

+''' Module for testing targets. '''


Docstrings should generally be trimmed, no space at the beginning or the end (unless it's needed for indentation after a newline)

douglas-raillard-arm · 2024-02-19T15:23:06Z

tests/test_target.py



 class TestReadTreeValues(TestCase):
+    ''' Class testing Target.read_tree_values_flat() '''
+
+    TARGETS_CONFIG_FILE = 'tests/target_configs.yaml'


Does pytest guarantees the current folder when running tests ? If not this could be made relative to __file__'s folder for example.

I run tests under /devlib dir. Will use __file__'s folder to ensure that.

douglas-raillard-arm · 2024-02-19T15:23:25Z

tests/test_target.py


    def test_read_multiline_values(self):
+        ''' Test Target.read_tree_values_flat() '''


s/^ // and s/ $//

douglas-raillard-arm · 2024-02-19T15:24:27Z

tests/test_target.py

@@ -34,11 +47,13 @@ def test_read_multiline_values(self):
        tempdir = tempfile.mkdtemp(prefix='devlib-test-')


should use context manager classes available in tempfile

I don't have a change here in this PR.
79a91b7 uses context manager anyway.

douglas-raillard-arm · 2024-02-19T15:25:18Z

devlib/utils/android.py

@@ -294,6 +294,10 @@ def __init__(
            total_transfer_timeout=total_transfer_timeout,
            transfer_poll_period=transfer_poll_period,
        )
+
+        self.logger.debug('AdbConnection(): server=%s port=%s device=%s as_root=%s',


s/AdbConnection(): //

The name of the class should be used when the logger is made.

douglas-raillard-arm · 2024-02-19T15:27:13Z

devlib/utils/android.py

@@ -294,6 +294,10 @@ def __init__(
            total_transfer_timeout=total_transfer_timeout,
            transfer_poll_period=transfer_poll_period,
        )
+
+        self.logger.debug('AdbConnection(): server=%s port=%s device=%s as_root=%s',


There is a slight technical benefit of using this way of interpolating (i.e. avoiding interpolation unless that debug level is enabled) but it's negligible and mostly just increases the risk of mixup. You can use an f-string instead to avoid that, and even let the compiler avoid redundancy with e.g. {adb_server=}.

I think pylint recommended using lazy formatting instead of f-string.

While we are there, also fix a trivial pylint issue regarding string format. Signed-off-by: Metin Kaya <[email protected]>

This is copied from WA (workload-automation/wa/utils/misc.py). Hence, published another PR [1] removes the implementation from WA. OTOH, this patch uses ``ruamel`` instead of ``yaml`` because of the latter's design issues. And also this patch fixes pylint issues in ``load_struct_from_yaml()``. [1] ARM-software/workload-automation#1248 Signed-off-by: Metin Kaya <[email protected]>

marcbonnici · 2024-02-20T20:11:38Z

devlib/utils/misc.py

+    except yaml.YAMLError as ex:
+        message = ex.message if hasattr(ex, 'message') else ''
+        lineno = ex.problem_mark.line if hasattr(ex, 'problem_mark') else None
+        raise LoadSyntaxError(message, filepath=filepath, lineno=lineno) from ex


We use this in WA to try and provide a more friendly error message to clearly indicate the location of the syntax error.
Since this code is being moved, I'd vote for keeping the same error class.

marcbonnici · 2024-02-20T20:21:24Z

tests/test_target.py



 class TestReadTreeValues(TestCase):
+    """Class testing Target.read_tree_values_flat()"""
+
+    TARGETS_CONFIG_FILE = os.path.join(os.path.dirname(os.path.realpath(__file__)),


If the intention is to be able to perform these tests on different device types based on the config, do we want the parsing / initialization to be performed within each test case?
Should these actions be performed in some setup method instead that can be reused?

The final product is something like this: a7d0b9d
Yes, there are different target types and there can be multiple entries for a target.
I read config file first and see which targets need to be tested. So, I could keep target classes as is.

Hmm.. I see. Apologies in advanced if I'm missing the obvious but in the final product do we end up with this code just for a single test_read_multiline_values unittest?
Does this mean this will need to be duplicated when we add another test or test Class?

Also does this require setting up a new connection to the target for each test? I'm wondering what the overhead/performance would be on this?

There is only one test_read_multiline_values() function. It parses the YAML file and executes the test specified in run_test() for each target in the YAML file. YAML file has necessary connection information and QEMU params for each target. Depending on the target type, running run_test() can take ~1 (LocalLinux target) or ~15 secs (for QEMU target).

Final version of TestReadTreeValues() looks like this:

class TestReadTreeValues(TestCase): def test_read_multiline_values(self): def run_test(target): ... target_configs = load_struct_from_yaml(self.TARGETS_CONFIG_FILE) ... if target_configs.get('AndroidTarget') is not None: ... target = AndroidTarget( connection_settings=entry['connection_settings'], conn_cls=lambda **kwargs: AdbConnection(adb_as_root=True, **kwargs), ) run_test(target) if target_configs.get('LinuxTarget') is not None: ... target = LinuxTarget(connection_settings=entry['connection_settings']) run_test(target) if target_configs.get('LocalLinuxTarget') is not None: ... target = LocalLinuxTarget(connection_settings=entry['connection_settings']) run_test(target) if target_configs.get('QEMUTargetRunner') is not None: ... with QEMUTargetRunner( qemu_params=qemu_params, connection_settings=connection_settings, ) as qemu_target: run_test(qemu_target.target)

And this is a sample YAML file:

AndroidTarget: entry-0: connection_settings: device: 'emulator-5554' entry-1: connection_settings: device: 'emulator-5556' LinuxTarget: entry-0: connection_settings: host: 'example.com' username: 'username' password: 'password' LocalLinuxTarget: entry-0: connection_settings: unrooted: True QEMUTargetRunner: entry-0: qemu_params: kernel_image: '/devlib/tools/buildroot/buildroot-v2023.11.1-aarch64/output/images/Image' entry-1: qemu_params: kernel_image: '/devlib/tools/buildroot/buildroot-v2023.11.1-x86_64/output/images/bzImage' arch: 'x86_64' cmdline: 'console=ttyS0'

I see, thanks for the information.

Although we have only have a single test case test_read_multiline_values at the moment, if we have this nice infrastructure to be able to run these tests and have more meaningful output, I'm thinking if we were expecting to add more test cases going forward, how would that fit into the current implementation? For example if we wanted to add a test_read_escape_values test case that checks certain characters are escaped correctly.

The parsing / creation of the various targets looks like it should be mostly generic and not tied to this single test case, therefore I'm wondering if there is a common location this would be better suited to be shared?

That totally makes sense to me.

How about the new version? If it looks OK, I'm gonna update other PRs.
Moving to pytest is the simplest solution I could find to prevent duplicate target config reading, passing target parameter to the test function (as well as future ones).

Thanks.

This will be useful in automating CI tests without modifying the source code. Replace unittest with pytest in order to make parameter passing to test functions easier. Move target configuration reading and generating target object outside of the test function. Because we will run the test function for new targets and may want to add new test functions. While we are here, also fix pylint issues. Signed-off-by: Metin Kaya <[email protected]>

douglas-raillard-arm · 2024-03-27T14:22:09Z

tests/test_target.py

-            path = os.path.join(tempdir, key)
-            with open(path, 'w') as wfh:
-                wfh.write(value)
+    tempdir = tempfile.mkdtemp(prefix='devlib-test-')


This should use tempfile.TemporaryDirectory, as a context manager.

Latest version of this code:

with target.make_temp() as tempdir: ... raw_result = target.read_tree_values_flat(tempdir) result = {os.path.basename(k): v for k, v in raw_result.items()}

And this is Target()::make_temp() implementation (295f126):

@contextmanager def make_temp(self, is_directory=True, directory='', prefix='devlib-test'): ... try: cmd = f'mktemp -p {directory} {prefix}-XXXXXX' if is_directory: cmd += ' -d' temp_obj = self.execute(cmd).strip() yield temp_obj finally: if temp_obj is not None: self.remove(temp_obj)

metin-arm changed the title ~~Clean yaml~~ Read target connection settings from a YAML file Feb 5, 2024

metin-arm mentioned this pull request Feb 5, 2024

utils/misc: Clean duplicated code ARM-software/workload-automation#1248

Merged

metin-arm force-pushed the clean-yaml branch 3 times, most recently from 2096387 to cd7bea9 Compare February 5, 2024 14:45

This was referenced Feb 5, 2024

Integrate buildroot and add support for QEMU Linux target #667

Merged

tools/docker: Add Docker image support for devlib #672

Merged

douglas-raillard-arm reviewed Feb 12, 2024

View reviewed changes

metin-arm force-pushed the clean-yaml branch from cd7bea9 to 7756317 Compare February 12, 2024 11:40

douglas-raillard-arm reviewed Feb 19, 2024

View reviewed changes

utils/android: Add debug log about connection settings

2169e61

While we are there, also fix a trivial pylint issue regarding string format. Signed-off-by: Metin Kaya <[email protected]>

metin-arm force-pushed the clean-yaml branch 2 times, most recently from b9ffd3f to 49f1b39 Compare February 20, 2024 08:43

metin-arm force-pushed the clean-yaml branch from 49f1b39 to 36731c5 Compare February 20, 2024 09:26

marcbonnici reviewed Feb 20, 2024

View reviewed changes

metin-arm force-pushed the clean-yaml branch 4 times, most recently from aefcf76 to fc0c531 Compare February 23, 2024 14:44

metin-arm force-pushed the clean-yaml branch from fc0c531 to 6217c77 Compare February 23, 2024 15:36

metin-arm mentioned this pull request Feb 23, 2024

Add support for testing ChromeOS targets #673

Merged

marcbonnici approved these changes Feb 23, 2024

View reviewed changes

marcbonnici merged commit a1718c3 into ARM-software:master Feb 23, 2024

metin-arm deleted the clean-yaml branch March 22, 2024 09:13

douglas-raillard-arm reviewed Mar 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read target connection settings from a YAML file #670

Read target connection settings from a YAML file #670

metin-arm commented Feb 5, 2024 •

edited

Loading

douglas-raillard-arm Feb 12, 2024

douglas-raillard-arm Feb 12, 2024

metin-arm Feb 12, 2024

douglas-raillard-arm left a comment

douglas-raillard-arm Feb 19, 2024

metin-arm Feb 20, 2024

douglas-raillard-arm Feb 19, 2024

douglas-raillard-arm Feb 19, 2024

metin-arm Feb 20, 2024

marcbonnici Feb 20, 2024

douglas-raillard-arm Feb 23, 2024

douglas-raillard-arm Feb 19, 2024

douglas-raillard-arm Feb 19, 2024

metin-arm Feb 19, 2024

douglas-raillard-arm Feb 19, 2024

douglas-raillard-arm Feb 19, 2024

metin-arm Feb 20, 2024

douglas-raillard-arm Feb 19, 2024

douglas-raillard-arm Feb 19, 2024

metin-arm Feb 19, 2024

marcbonnici Feb 20, 2024

marcbonnici Feb 20, 2024

metin-arm Feb 21, 2024

marcbonnici Feb 21, 2024

metin-arm Feb 22, 2024

marcbonnici Feb 22, 2024

metin-arm Feb 23, 2024

douglas-raillard-arm Mar 27, 2024

metin-arm Mar 27, 2024

		@@ -590,6 +591,31 @@ def __str__(self):
		return message.format(self.filepath, self.lineno, self.message)


		def load_struct_from_yaml(filepath):


		def test_read_multiline_values(self):
		''' Test Target.read_tree_values_flat() '''

		@@ -34,11 +47,13 @@ def test_read_multiline_values(self):
		tempdir = tempfile.mkdtemp(prefix='devlib-test-')

Read target connection settings from a YAML file #670

Read target connection settings from a YAML file #670

Conversation

metin-arm commented Feb 5, 2024 • edited Loading

PRs #667 and ARM-software/workload-automation#1248 can be merged after this one.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

douglas-raillard-arm 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metin-arm commented Feb 5, 2024 •

edited

Loading