target: tests: Address review comments on PR#667 #677

metin-arm · 2024-03-28T11:46:46Z

PR#667: https://github.com/ARM-software/devlib/pull/667

Implement separate initialize / tear down methods and make various cleanups in test/test_target.py
Make docstrings Sphinx compatible
Make TargetRunner and its subclasses private
Cleanup tests/test_target.py
Replace print()'s with appropriate logging calls
Implement NOPTargetRunner class for simplifying tests
Improve Python v3.7 compatibility
Relax host machine type checking
Escape user input strings

and more..

douglas-raillard-arm · 2024-04-05T12:07:23Z

devlib/_target_runner.py

+        if runner_cmd is None:
+            return
+


Not a good idea. Doing so leaves the object in a half-initialized state where some attributes may or may not exist based on some runtime condition. If that is "actually what you meant to get", this indicates either a design problem and the class hierarchy needs to be reworked (e.g. you are conflating the base class defining the interface that all instances must provide with a specific implementation that manages a Popen object), or simply that you need a no-op kind of runner_process. Most likely it's a class hierarchy issue.

In case you are tempted, leaving that as-is and editing every method that use that attribute to make them check if the attribute exists first is also a terrible idea.

Thanks for your kind comment.
Will find a solution to clean runner_cmd check.

douglas-raillard-arm · 2024-04-05T12:51:03Z

tests/test_target.py

-
-    assert {k: v.strip() for k, v in data.items()} == result
+    @classmethod
+    def setUpClass(cls):


It looks like pytest has better alternatives than setupClass: https://stackoverflow.com/questions/40660842/pytest-setup-teardown-hooks-for-session
setupClass is a dodgy design as the class is a singleton for the entire duration of the entire process it's in. You can't cleanly re-import a module in Python without creating problems. As a general rule, I'd advise using pytest features over the unittest module for anything else that is not really basic. unittest was designed a loooonnng time ago, as a copy/paste from some Java lib. It's not really representative of today's Python best practices (if it ever was), as you can guess from the pre-PEP8 method names.

I did try to solve it in pytest, but could not find a setup and teardown methods which run exactly for once.
Will look into https://stackoverflow.com/a/40673918/1370011 to give another chance.

Looks like the fixture with module "scope" worked out

douglas-raillard-arm · 2024-04-05T14:03:27Z

tests/test_target.py

+
+            target_runners = []
+
+            if target_configs.get('AndroidTarget') is not None:


The config seems to be organized as:

<target type>: - <settings 1> - <settings 2> - ... ...

I wouldn't advise doing that as this makes modularizing the configuration basically impossible. The <target type> keys are always going to be unique as per YAML spec, so by organizing it this way you prevent e.g. concatenating multiple files together. You also prevent packaging some target settings in a separate file that would then be included, as what you could include this way would only be partial information (lacking the target type).

As a result, I'd strongly recommend you use a single list of target configurations, and each item of this list is fully self-descriptive and does not rely on the context (name of the parent node) for its interpretation.

Even better, if you host the entire configuration under one top-level key, you will also make it compatible with other YAML conf, like e.g. the LISA one, allowing users to use a single file for various things.

Moved all target configurations under a top-level key.

douglas-raillard-arm · 2024-04-05T14:06:07Z

tests/test_target.py

+            logger.debug('Removing %s...', target.working_directory)
+            target.remove(target.working_directory)


I've never gotten around to making the patch but I think this should just be standard behavior of target upon disconnecting a target. @marcbonnici do you see any reason not to wipe the working_directory ?

In WA we have a few workload that benefit from keeping large files (photos, videos etc.) on the device between runs to reduce the setup costs/overhead. By default most of these workloads keep their files under the working_directory so it could be undesirable to wipe this directory on each disconnect.

The general pattern we have tried to follow with our WA asset deployment mechanism is that if a files/directory is not needed after a given run is that we remove it but we still leave the option for workloads to override this as they choose.

Maybe it would make sense to default to cleaning up, and have WA override that default then ? In general, keeping resources deployed on the target are a recipe for trouble unless carefully implemented (e.g. checksuming the resources to ensure what is already deployed is actually what the code expects to work with). Even if WA does that carefully to avoid troubles, I wouldn't expect most client code to go to these length. In LISA, we push all our tools every time they are needed (at most once per Target instance, so it's "cached" but only very temporarily).

True, I agree it is safer in the general case to push to the target each time but even today a client can currently do so without issue. I would just question whether it makes sense for devlib to automatically handle the cleanup vs leave the decision to the client code itself via whatever mechanism they choose.

So far we haven't placed any restrictions on the expectations on the working directory, so I'd be hesitant to introduce a new default that would wipe it out as we can't say how other users / client code could be using this directory.

Essentially I think I might be missing what the benefit would be to the end user vs the potential downsides of clearing files the user may not have wanted to?

I'll keep this hunk as is since this discussion sounds like a separate/future improvement.

Let's move the discussion to #680, but let's keep that comment thread open as the code will need to be changed if we end up implementing the feature (or at least leave a # TODO: revisit based on https://github.com/ARM-software/devlib/issues/680 outcome in the code).

douglas-raillard-arm · 2024-05-13T09:36:58Z

devlib/_target_runner.py

+    """
+    Class for implementing a target runner which does nothing except providing .target attribute.
+
+    :class:`NOPTargetRunner` is a subclass of :class:`TargetRunner` which is implemented only for


Autodoc makes the subclass relationship obvious, there is no need to restate it here

OK. Will remove it.

douglas-raillard-arm · 2024-05-13T09:47:45Z

devlib/_target_runner.py

+    """
+
+    def __init__(self, target):
+        super().__init__(runner_cmd=['true'],


As per earlier review round, runner_cmd does not belong to TargetRunner, so it needs to be removed from there and a subclass created that does handle a running process (e.g. let's call it SubprocessTargetRunner). NOPTargetRunner will not inherit from SubprocessTargetRunner, it will inherit straight from TargetRunner.

In other words, having to execute a command is a pure implementation detail of a specific runner. It happens that a few of them do require that, but it should by no mean be part of the "public interface" of TargetRunner in general, as made obvious by NOPTargetRunner. That public interface is essentially limited to providing __enter__/__exit__, a .target attribute and a couple ancillary methods like terminate() and wait_boot_completed()

douglas-raillard-arm · 2024-05-13T10:03:18Z

tests/test_config.yml.example

+target-configs:
+  AndroidTarget:
+    entry-0:
+      timeout: 60
+      connection_settings:
+        device: 'emulator-5554'


This structure still can't be built easily with simple templating. What we need is

target-configs: entry-0: AndroidTarget: timeout: 60 connection_settings: device: 'emulator-5554' entry-1: ...

Concretely we want to be able to easily build a config file using e.g. jinja2 for loops. As it stands, you'd have to make jinja parse each config file you would be trying to merge, which is not possible. With the structure I'm recommending, this can be dealt by just including the content from multiple "snippet" files (and maybe use a filter to increase indentation level).

douglas-raillard-arm · 2024-05-13T10:12:38Z

tests/test_target.py

-@pytest.mark.parametrize("target, target_runner", build_targets())
-def test_read_multiline_values(target, target_runner):
+# pylint: disable=redefined-outer-name
+def test_read_multiline_values(build_target_runners):


Could you confirm that if another test is defined, build_target_runners() is not called again and the existing runners re-used ? I'd expect it considered that fixture has "module" scope but let's double check.

EDIT: The doc seems to indicate the behavior we want as it states the fixture return value is cached for the scope:

If a requested fixture was executed once for every time it was requested during a test, then this test would fail because both append_first and test_string_only would see order as an empty list (i.e. []), but since the return value of order was cached (along with any side effects executing it may have had) after the first time it was called, both the test and append_first were referencing the same object, and the test saw the effect append_first had on that object.

https://docs.pytest.org/en/7.1.x/how-to/fixtures.html

I did confirm already. Added Initializing/Destroying resources... logs mainly for that purpose.

PR#667: ARM-software#667 - Implement a test module initializer with a tear down method in test/test_target.py - Make various cleanups in test/test_target.py - Improve structure of test/test_config.yml (previously target_configs.yaml) - Make docstrings Sphinx compatible - Make ``TargetRunner`` and its subclasses private - Cleanup tests/test_target.py - Replace print()'s with appropriate logging calls - Implement ``NOPTargetRunner`` class for simplifying tests - Improve Python v3.7 compatibility - Relax host machine type checking - Escape user input strings and more.. Signed-off-by: Metin Kaya <[email protected]>

metin-arm · 2024-05-16T08:54:28Z

Only updated tools/docker/target_configs.yml in the last push (384c2aefa47).

douglas-raillard-arm · 2024-05-23T09:47:03Z

LGTM

metin-arm mentioned this pull request Mar 28, 2024

Integrate buildroot and add support for QEMU Linux target #667

Merged

metin-arm force-pushed the pr-667 branch from 88d8cfd to aaac681 Compare March 28, 2024 11:55

metin-arm mentioned this pull request Mar 28, 2024

tools/android: Address review comments on PR#668 #678

Merged

metin-arm force-pushed the pr-667 branch 6 times, most recently from c6f39de to 9d0126c Compare April 2, 2024 15:21

douglas-raillard-arm reviewed Apr 5, 2024

View reviewed changes

metin-arm force-pushed the pr-667 branch 2 times, most recently from 49731a3 to 49b0d42 Compare April 15, 2024 14:41

douglas-raillard-arm mentioned this pull request Apr 16, 2024

Wipe target working directory upon disconnect #680

Open

metin-arm force-pushed the pr-667 branch from 49b0d42 to eedbf43 Compare April 17, 2024 07:00

douglas-raillard-arm reviewed May 13, 2024

View reviewed changes

metin-arm force-pushed the pr-667 branch from eedbf43 to a8a842d Compare May 15, 2024 12:46

metin-arm force-pushed the pr-667 branch from a8a842d to 384c2ae Compare May 16, 2024 08:53

marcbonnici approved these changes May 29, 2024

View reviewed changes

marcbonnici merged commit 492d42d into ARM-software:master May 29, 2024

metin-arm deleted the pr-667 branch June 27, 2024 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target: tests: Address review comments on PR#667 #677

target: tests: Address review comments on PR#667 #677

metin-arm commented Mar 28, 2024 •

edited

Loading

douglas-raillard-arm Apr 5, 2024

metin-arm Apr 15, 2024

douglas-raillard-arm Apr 5, 2024

metin-arm Apr 15, 2024

douglas-raillard-arm May 13, 2024

douglas-raillard-arm Apr 5, 2024

metin-arm Apr 15, 2024

douglas-raillard-arm Apr 5, 2024

marcbonnici Apr 10, 2024

douglas-raillard-arm Apr 10, 2024

marcbonnici Apr 12, 2024

metin-arm Apr 15, 2024

douglas-raillard-arm Apr 16, 2024

metin-arm Apr 17, 2024

douglas-raillard-arm May 13, 2024

metin-arm May 13, 2024

douglas-raillard-arm May 13, 2024

metin-arm May 15, 2024

douglas-raillard-arm May 13, 2024

metin-arm May 15, 2024

douglas-raillard-arm May 13, 2024 •

edited

Loading

metin-arm May 13, 2024

metin-arm commented May 16, 2024

douglas-raillard-arm commented May 23, 2024


		target_runners = []

		if target_configs.get('AndroidTarget') is not None:

		logger.debug('Removing %s...', target.working_directory)
		target.remove(target.working_directory)

target: tests: Address review comments on PR#667 #677

target: tests: Address review comments on PR#667 #677

Conversation

metin-arm commented Mar 28, 2024 • edited Loading

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

douglas-raillard-arm May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metin-arm commented May 16, 2024

douglas-raillard-arm commented May 23, 2024

metin-arm commented Mar 28, 2024 •

edited

Loading

douglas-raillard-arm May 13, 2024 •

edited

Loading