Standalone Greentea host #301

wernerlewis · 2021-05-24T09:59:51Z

Development of greentea host was moved into mbed-os-tools and this repository was deprecated. Following the decision to make Greentea standalone again, this repository is reused.

The following changes are made:

mbed-os-tools is not required, now using mbedls directly for device detection. htrun is to be included in this repo.
Unused yotta files and references are removed.
Mbed references are removed, package has not yet been renamed.
Errors found with black, mypy and flake8 are resolved.
Docstrings are updated and standardised.

rwalton-arm

In the commit "Initial greentea host" we seem to be doing far too much. We should split this into separate commits:

Copies the code over as is
Reformats with black
Adds tox.ini
Removes mbed refs

It doesn't matter as much about every commit passing tests at this point as we don't have any CI enabled. At the moment it's too difficult to review any changes we've made.

We also need to work out how we integrate this PR and #302 - I would've expected one of the PRs to be based on the other if there's any crossover. If they merge cleanly without conflicts as-is then great.

wernerlewis · 2021-06-04T16:22:17Z

I've rebased and updated so that this integrates on top of the previous work, and split out the commits.

When running gt, htrun passes a few test suites, but fails the majority. Replacing htrun with mbedhtrun to use the existing version from mbed-os-tools doesn't show the same issue.

This seems to be due to the host_tests not being loaded from mbed-os: for example, due to the change in module name. So in host_registry.py host_tests aren't being registered, leading to "host test not detected" errors. This means gt/htrun will fail to run tests that rely on host_tests defined in mbed-os until those are updated.

.gitignore

.pre-commit-config.yaml

rwalton-arm · 2021-06-10T10:42:18Z

src/greentea/gtea/mbed_greentea_cli.py

    ctest_test_list, test_by_names, skip_test, test_spec=None
 ):
    """! Filters test case list (filtered with switch -n) and return filtered list.
-    @ctest_test_list List iof tests, originally from CTestTestFile.cmake in yotta module. Now comes from test specification


Interesting, seems we had support for CTest in some form. Although it looks like we were trying to use this python tool to replace CTest by grepping CTestTestFile.cmake.

src/greentea/__main__.py

src/greentea/gtea/greentea_hooks.py

rwalton-arm · 2021-06-10T11:28:06Z

src/greentea/gtea/greentea_log.py


 class GreenTeaSimpleLockLogger(object):
-    """Simple locking printing mechanism."""
+    """Simple lock printing mechanism."""


What's a lock printing mechanism? Do we mean a thread-safe printing mechanism? I assume we're trying to guarantee serialised output in a multi-threaded context with this object.

rwalton-arm · 2021-06-10T11:35:50Z

src/greentea/gtea/report_api.py

+    Returns:
+        String containing Junit XML formatted test result output.
    """
    from junit_xml import TestSuite, TestCase


I think we should create a task to remove the function scoped imports: it's not good form to import a bunch of stuff in a public api function, especially if it's going to be called more than once (we'll run the import code again on each call). It's a small cost to import these things at file scope, and a cost we only need to pay once at "import time". The function scoped imports are misguided attempts at micro-optimisation imo.

LDong-Arm

Thanks, there are lots of nice improvements in this PR.

But as an initial code-import PR, it might be easier if we just imported mbed-os-tools code "as-is" and make it run? As mbed-os-tools is in a working state, keeping it working in a different repo might only involve updating paths, etc. if I didn't miss anything? Maybe we can enable tests (e.g. tox) and fix failures incrementally in separate PRs?

The commits to import code look good to me, but the other fixes and improvements seem to have some learning curve for reviewers, and there are several things to review. Shall we consider limiting the scope and getting the imported code in first, so subsequent work (incl. other items in #303) can be worked on parallel? Or, if we are "overall" fine with all the commits, maybe get them in?

.github/issue_template.md

.github/pull_request_template.md

CONTRIBUTING.md

LDong-Arm · 2021-06-30T16:10:28Z

.flake8

+ignore =
+    # H301: one import per line
+    H301,
+    # H306: imports not in alphabetical order
+    H306,
+    # W503:  line break before binary operator (this is no longer PEP8 compliant)
+    W503,
+    # E203: whitespace before ':' (this is not PEP8 compliant)
+    E203,


It seems to be added by this PR, as it looks different from what exists in mbed-os-tools?
The rules look good to have, maybe we should acknowledge the issues and plan to fix them in the future, instead of ignoring the rules?

Yes, I added these from mbed-tools. I've removed H301 and H306 as these do not need to be ignored. W503 is ignored as it goes against best practices, and E203 is ignored for PEP compliance, otherwise errors are raised due to formatting of list slicing.

This pulls in greentea from mbed-os-tools into this repo.

linting, py3* environments are added. requirements.txt is updated to include requirements that previously were used in mbed-os-tools.

License headers in files vary significantly in format. All files are updated to use a standard template.

Yotta is deprecated, and this content is no longer needed in greentea.

The following changes are made: - mbed prefix is removed from files. - mbed_os_tools greentea imports are replaced with greentea imports. - mbed_devices are renamed as devices. - MUT (Mbed Under Test) is changed to DUT (Device Under Test). - mbedhtrun in greentea CLI is changed to htrun. - mbedgt is changed to gt.

Dependency on mbed_os_tools is replaced with mbedls_tools. htrun is used from this repo.

Remove unused arguments and reword to improve understandability.

In greentea_hooks two different methods are given for running hooks, which have the same behaviour, implemented differently. This removes run_hook_ext to avoid this unnecessary repeat.

target_info relied on a local dict of information for a few Mbed OS targets, with data identical to that in targets.json. target_info is reworked to remove this case, just using targets.json if present, with a default fallback.

Some parser options are not used and can be removed. digest_source is removed along with corresponding code, due to lack of implementation.

optparse has been deprecated since Python 3.2. argparse is recommended as a replacement, minor changes must be made to support this.

A large HTML template was in report_api.py, which is moved into a separate template file. Old-style % string formatting is also replaced with f-strings and .format().

LDong-Arm

So, I verified using tox -e py39 and tests passed. Also did a smoke test using gtinstalled from this PR to run tests on a board.

Btw, what about calling the command greentea instead of gt? I just feel that two-letter commands are too easy to clash, and normally system commands are this short.

@ARMmbed/mbed-os-core I strongly feel that we should prioritise getting this merged, because it's been open for a long time and blocking further work on this repo.

rwalton-arm · 2021-07-01T13:11:40Z

src/greentea/gtea/greentea_hooks.py

 import json
-from subprocess import Popen, PIPE
+
+from subprocess import run


Usually the style is to import subprocess and then call subprocess.run as it's then explicit what the run function is doing. The function name itself is rather generic, so using the namespaced form at the call site is better in this case.

rwalton-arm · 2021-07-01T13:25:33Z

src/greentea/gtea/greentea_hooks.py

+        except OSError as e:
+            gt_logger.gt_log_err(str(e))
+            return -1
+        if p.stdout:


Why not just set capture_output=False? We seem to print stdout and stderr all the time if they aren't empty.

I would also leave shell=False unless absolutely necessary, as shell=True causes subprocess to spawn an intermediate "shell instance" to run the command, which could make us vulnerable to shell injection exploits.

We might have to change self.cmd to a list to make this work with shell=False, if it's currently just a string. We should probably deal with the shell spawning in another PR anyway as it was preexisting.

I've removed the capture_output arg. cmd is a string, so that will need to be changed to remove shell=True.

rwalton-arm · 2021-07-01T13:44:12Z

So, I verified using tox -e py39 and tests passed. Also did a smoke test using gtinstalled from this PR to run tests on a board.

Btw, what about calling the command greentea instead of gt? I just feel that two-letter commands are too easy to clash, and normally system commands are this short.

@ARMmbed/mbed-os-core I strongly feel that we should prioritise getting this merged, because it's been open for a long time and blocking further work on this repo.

I'm OK with the overall commits (apart from one point about how we import subprocess.run). One potentially important change is that we've removed the lockfile mechanism when spawning multiple instances. Most of the other changes are just cosmetic or small amounts of refactoring. I'm fine with merging the PR after my latest comment is addressed.

subprocess.run() is supported in Python 3.5+, making this wrapper not required. An unused implementation of the same wrapper is also removed.

The locking mechanism, greentea kettle, is marked as experimental, and the implementation is not complete. The package used by the mechanism is deprecated, and the value of implementing a locking feature is not clear, so this mechanism can be removed.

CTestTestfile.cmake is no longer parsed by greentea. References to CTest and unused code for CTest parsing are removed.

Previous testing had ignored greentea source files when running flake8. As a result, a number of style issues needed to be resolved. Along with this, docstrings are updated, in google-style.

rwalton-arm

LGTM. Thanks.

LDong-Arm

LGTM

wernerlewis force-pushed the greentea-host branch from c653379 to f6b92ad Compare May 25, 2021 08:32

wernerlewis marked this pull request as draft May 25, 2021 10:09

wernerlewis force-pushed the greentea-host branch from f6b92ad to a6dbdf9 Compare June 3, 2021 09:03

wernerlewis marked this pull request as ready for review June 3, 2021 09:20

rwalton-arm mentioned this pull request Jun 3, 2021

Make greentea tools standalone #303

Closed

22 tasks

wernerlewis force-pushed the greentea-host branch 2 times, most recently from 729cdc7 to fb146d2 Compare June 3, 2021 12:51

rwalton-arm suggested changes Jun 3, 2021

View reviewed changes

wernerlewis force-pushed the greentea-host branch 2 times, most recently from 3dd6af9 to 1180f2b Compare June 4, 2021 14:58

rwalton-arm suggested changes Jun 10, 2021

View reviewed changes

wernerlewis force-pushed the greentea-host branch 4 times, most recently from 0b6a4db to b05e20c Compare June 21, 2021 09:30

wernerlewis force-pushed the greentea-host branch from b05e20c to b2a1c03 Compare June 24, 2021 09:14

Patater requested a review from rwalton-arm June 25, 2021 10:56

Move htrun tests into subdir

19c1f94

wernerlewis force-pushed the greentea-host branch from b2a1c03 to 3238106 Compare June 29, 2021 14:04

LDong-Arm reviewed Jun 30, 2021

View reviewed changes

wernerlewis added 9 commits July 1, 2021 09:10

Standalone greentea CLI

69941bb

This pulls in greentea from mbed-os-tools into this repo.

Apply changes from black

9431a27

Add tox.ini and pre-commit requirements

9d8491c

linting, py3* environments are added. requirements.txt is updated to include requirements that previously were used in mbed-os-tools.

Update license headers

f930d40

License headers in files vary significantly in format. All files are updated to use a standard template.

Remove implementations for yotta

773b1cc

Yotta is deprecated, and this content is no longer needed in greentea.

Replace mbed_os_tools dependency

b96a1ef

Dependency on mbed_os_tools is replaced with mbedls_tools. htrun is used from this repo.

Remove unused imports

8ddd41c

Refactor greentea log

1cefee1

Remove unused arguments and reword to improve understandability.

wernerlewis added 5 commits July 1, 2021 09:10

Remove repeat method for hooks

50c1824

In greentea_hooks two different methods are given for running hooks, which have the same behaviour, implemented differently. This removes run_hook_ext to avoid this unnecessary repeat.

Rework target properties handling

9bdddce

target_info relied on a local dict of information for a few Mbed OS targets, with data identical to that in targets.json. target_info is reworked to remove this case, just using targets.json if present, with a default fallback.

cli: Remove unused parser options

100fa2a

Some parser options are not used and can be removed. digest_source is removed along with corresponding code, due to lack of implementation.

cli: Replace optparse with argparse

229aa29

optparse has been deprecated since Python 3.2. argparse is recommended as a replacement, minor changes must be made to support this.

Move HTML template into a separate file

0b73ec3

A large HTML template was in report_api.py, which is moved into a separate template file. Old-style % string formatting is also replaced with f-strings and .format().

wernerlewis force-pushed the greentea-host branch from 3238106 to ca7a51f Compare July 1, 2021 08:14

LDong-Arm added story: standalone greentea/htrun and removed story: standalone greentea/htrun labels Jul 1, 2021

LDong-Arm previously approved these changes Jul 1, 2021

View reviewed changes

rwalton-arm suggested changes Jul 1, 2021

View reviewed changes

wernerlewis added 5 commits July 1, 2021 15:34

Replace subprocess Popen wrapper with run

10de88e

subprocess.run() is supported in Python 3.5+, making this wrapper not required. An unused implementation of the same wrapper is also removed.

Remove kettle locking mechanism

01ce433

The locking mechanism, greentea kettle, is marked as experimental, and the implementation is not complete. The package used by the mechanism is deprecated, and the value of implementing a locking feature is not clear, so this mechanism can be removed.

Remove CTest references

a1e6caa

CTestTestfile.cmake is no longer parsed by greentea. References to CTest and unused code for CTest parsing are removed.

Update isAlive to is_alive for Python 3.9 support

03d14c6

style: Fix flake8 errors and docstrings

fed2b1a

Previous testing had ignored greentea source files when running flake8. As a result, a number of style issues needed to be resolved. Along with this, docstrings are updated, in google-style.

wernerlewis dismissed LDong-Arm’s stale review via fed2b1a July 1, 2021 14:39

wernerlewis force-pushed the greentea-host branch from ca7a51f to fed2b1a Compare July 1, 2021 14:39

rwalton-arm approved these changes Jul 1, 2021

View reviewed changes

Patater requested a review from LDong-Arm July 1, 2021 15:34

LDong-Arm approved these changes Jul 1, 2021

View reviewed changes

Patater merged commit 2d527c0 into ARMmbed:master Jul 5, 2021

Patater assigned wernerlewis Jul 6, 2021

Standalone Greentea host #301

Standalone Greentea host #301

Uh oh!

Conversation

wernerlewis commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwalton-arm left a comment

Choose a reason for hiding this comment

Uh oh!

wernerlewis commented Jun 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwalton-arm commented Jul 1, 2021

Uh oh!

rwalton-arm left a comment

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wernerlewis commented May 24, 2021 •

edited

Loading

LDong-Arm left a comment •

edited

Loading