[take over] riotnode: node abstraction package #13612

leandrolanzieri · 2020-03-11T08:25:59Z

Contribution description

As discussed offline with @fjmolinas, this PR is taking over @cladmi's work in #10949. I added some missing tests that he proposed as TODOs, and cleaned up a bit git history.

There seems to be an agreement on #10949 on merging the package with the current features (and without the changes to 'testrunner'), and improve it in follow-up PRs. It would be nice to have this soonish, in order to start working on release tests before the freezes.

Testing procedure

Running the test suite can be done with tox (pip install --user tox).

Issues/PRs references

For a full description see #10949

It is a python directory to start developing the package. I include all files around for testing and integrating in RIOT. Test dependencies in tox are already ready for testing with pytest. * sitecustomize.py: allow implementing 'riotnode' in a subdirectory * setup.py implement a releasable package * requirements.txt: uses 'setup.py' could then be used by our standard way of installing * tox.ini: run the test suite with 'tox' * setup.cfg/.coveragerc: test suites configuration

The abstraction is a basic class to replicate behavior that was in `testrunner. Implementation was sometimes directly taken from `testrunner` without being technically justified to keep backward compatibility. This is described in `TODO.rst` and dedicated implementation. This also adds a test `node` implementation with a Makefile. It is currently an easy to use node as no output is lost and reset correctly resets. TODO: Maybe put this as a comment in the file When pexpect 'close' a 'ptyprocess' it starts by 'SIGHUP'. It is not handled by default to call 'atexit'. To not do a specific handling for 'SIGHUP' just forward all signals to the child. This wrapper should not do any specific things anyway. TODO: Split the 'safe_term_close' to utils with a test not using Make term and so directly only wanting 'sigkill' This should allow merging it before and using it in testrunner TODO: Settle the `env` handling. Get feedback on what to do here. Disable local echo otherwise pexpect matches sent characters.

This allows finer analysis of exception as sometimes the calling line is only `expect(variable)` without knowing the value.

This allows having a nicely printed output on `pytest`. This could be moved outside of here too.

Add functions that handle killing all child processes using psutil. This allows killing child processes that may not have been correctly terminated by its parent. It should not be necessary in an ideal world, but it would allow detecting errors in the tests.

This shows the current handling that could correctly terminate a subprocess that is not always killed. It is here as a regression test when removing the process group sigkill.

In case all children are not correctly cleaned by '_safe_term_close', sigkill them anyway. This prepares for replacing the global kill with SIGKILL with a less violent method. Also, adds a test where the terminal program tries to do a cleanup. It is important that a terminal can clean itself correctly to close files, or remove created interfaces.

The process should be closed alone with 'self.term.close'. Closing child processes must be done by the `make term` program.

Remove now unused _kill_term.

aabadie · 2020-03-24T10:49:52Z

I started to have a look at this PR and have several general comments/questions:

In the current state, this PR doesn't make use of riotnode for the existing tests. So it is a PR with > 1k unused lines of codes.
In the documentation or inline comments, there are a lot of sentences written using the first person (e.g. I do this, I do that), this is not right IMHO. The sentences should be re-written in a something more impersonal.
Is riotnode a good name for this feature ? I think something like riottest or riotrunner would be better.
What is the need for putting the whole riotnode implementation as a complete Python package (with versions, setup.py, etc) inside the RIOT codebase ? Wouldn't it make more sense to provide this package in an external public repository (if it meant to be used from another project, like riot-release-specs) ? This package could then be installed using regular Python packaging tools (pip) from any project that needs it (and not only RIOT or RIOT-release-specs). It would be great to have some discussion on this particular point.

miri64 · 2020-03-24T11:49:10Z

* What is the need for putting the whole riotnode implementation as a complete Python package (with versions, setup.py, etc) inside the RIOT codebase ? Wouldn't it make more sense to provide this package in an external public repository (if it meant to be used from another project, like riot-release-specs) ? This package could then be installed using regular Python packaging tools (pip) from any project that needs it (and not only RIOT or RIOT-release-specs). It would be great to have some discussion on this particular point.

👍

leandrolanzieri · 2020-03-25T09:17:20Z

In the current state, this PR doesn't make use of riotnode for the existing tests. So it is a PR with > 1k unused lines of codes.

True, this is following the proposal in the original PR, introducing the package as is with no changes on testrunner, and integrate it later.

In the documentation or inline comments, there are a lot of sentences written using the first person (e.g. I do this, I do that), this is not right IMHO. The sentences should be re-written in a something more impersonal.

Sure, I will address this!

Is riotnode a good name for this feature ? I think something like riottest or riotrunner would be better.

Discussion about naming is seems to be ongoing since the original PR. IMHO as the package provides an abstraction on the node (or whatever we would like to call it) which runs a certain firmware, the current name reflects more what the package is, as opposed to riottest or riotrunner which talk about one use case of it.

That being said, I'm not 100% convinced by riotnode and I would like to continue this discussion. Proposals which came up in the original PR are:

riotdevice
riotthing
riotagent
riotauthority

What is the need for putting the whole riotnode implementation as a complete Python package (with versions, setup.py, etc) inside the RIOT codebase ? Wouldn't it make more sense to provide this package in an external public repository (if it meant to be used from another project, like riot-release-specs) ? This package could then be installed using regular Python packaging tools (pip) from any project that needs it (and not only RIOT or RIOT-release-specs). It would be great to have some discussion on this particular point.

Regarding this the main concern seemed to be keeping the package in sync with RIOT's shell interfaces. What I see is that a big part of this package (and code) will be in the mixins we write to interact with commands (and those are the ones which need to be kept in sync with RIOT's shell, mostly). Would we have some on RIOT and some on Release-Specs? Will it come the time when we need to group them to reduce duplication?

I can see how keeping the package out of the codebase could be a good thing. Let's discuss further how this would look like.

miri64 · 2020-03-25T09:25:38Z

Regarding this the main concern seemed to be keeping the package in sync with RIOT's shell interfaces. What I see is that a big part of this package (and code) will be in the mixins we write to interact with commands (and those are the ones which need to be kept in sync with RIOT's shell, mostly). Would we have some on RIOT and some on Release-Specs? Will it come the time when we need to group them to reduce duplication?

The framework could go into its own repository, while the mixins for the shell commands could remain in RIOT-OS/RIOT. Regarding the ReleaseSpecs: we need the RIOT repo for testing those anyways (its what we are testing after all), so we would then also pull in the latest version of the mixins ;-).

leandrolanzieri · 2020-04-16T13:35:10Z

I'm fine with having the package on a separate repo if the mixins can be kept in RIOT-OS/RIOT and stay in sync with shell commands. @aabadie how should we proceed in this direction?

fjmolinas · 2020-04-16T13:43:36Z

The framework could go into its own repository, while the mixins for the shell commands could remain in RIOT-OS/RIOT. Regarding the ReleaseSpecs: we need the RIOT repo for testing those anyways (its what we are testing after all), so we would then also pull in the latest version of the mixins ;-).

+1 for this

aabadie · 2020-04-16T14:22:09Z

I'm not in favor of using the mixins but rather follow @cladmi's suggestion in #10949 and in #11406 (comment).

Otherwise I'm also OK for moving riotnode (or whatever better name it takes) to a separate repo.

miri64 · 2020-04-16T16:13:31Z

Regardless if we do it as mixins or associations, this is something we can decide once this framework is in place. I want this (and the shell command wrappers, however they look like) in for 2020.07, so we should do the first step ASAP :-).

cladmi · 2020-04-17T06:50:29Z

Without re-reading my comments, what I was "against" was to have the implementation in the mixins.
If it is a separate class, you can use it with composition, or inheritance if you really want, and wrap it after as mixin.

Defining mixins allows simple instantiation for the price of an enforced usage convention.
Restricting tools at that level is for me a bad idea, so preferred having both. The low level powerful layer where you must be explicit, and on top the easy to instantiate but constrained mixins.

I had in mind (and played with) a layer on top with mixins that would create a composed class:

class MyNodeUnderTest(IfconfigWrapperMixin, RouteWrapperMixin): with the hierarchy node.shell.ifconfig.list_interfaces() or something.

And even the next abstraction being class MyNodeUnderTest(RiotNodeShellMixinsFromModulesList): with an horrible metaclass.

cladmi · 2020-04-17T07:04:53Z

The term target should be replaced by cleanterm.

For a first version, you can remove the exception handling from there I think. The shorter exception is a nice thing but may better be done after using it and seeing what is wanted.

The "ensuring all process stopped" is an horrible thing that should be removed
I did it to match the replace the SIGKILL, yes kill dash nine!, the whole group in testrunner as I was planning to be a drop-in replacement. If the terminal process does not stop on sigterm/sigint, the terminal process must be fixed, not the caller there that must do horrible ps listing, as dev will also want to do ctrl+c to stop it.

Only go for the first two commits. It was developed with separate steps to demo what could be done at what price, not take everything as is.

miri64 · 2020-06-09T08:16:59Z

That being said, I'm not 100% convinced by riotnode and I would like to continue this discussion. Proposals which came up in the original PR are:
* `riotdevice`

* `riotthing`

* `riotagent`

* `riotauthority`

riottest? IMHO as this is primarily a test framework it should be immediately, that should be immediately obvious from the name.

fjmolinas · 2020-06-09T08:19:43Z

riottest

+1

fjmolinas · 2020-06-09T08:31:21Z

As I see this is missing:

replace term by cleanterm
squash into 2 commits
remove _ensure_process_stopped
rename to riottest

With that we can move it to the organization.

fjmolinas

small nitpick

fjmolinas · 2020-03-14T12:08:35Z

dist/pythonlibs/riotnode/riotnode/node.py

+        command = ['make']
+        command.extend(self.MAKE_ARGS)
+        if self._application_directory != '.':
+            dir_cmd = '--no-print-directory', '-C', self.application_directory


Suggested change

dir_cmd = '--no-print-directory', '-C', self.application_directory

dir_cmd = '--no-print-directory', '-C', self._application_directory

fjmolinas · 2020-03-14T12:11:06Z

dist/pythonlibs/riotnode/riotnode/node.py

+        self.logger = logging.getLogger(__name__)
+
+    @property
+    def application_directory(self):


Should we add a setter function for this?

Discussing it offline with @cladmi, it makes sense to not be able to set it since some application have different requirements for the terminal baudrate use of ethos, etc.. so it makes sense to not be able to change the application directory.

fjmolinas · 2020-06-09T12:52:48Z

riottest? IMHO as this is primarily a test framework it should be immediately, that should be immediately obvious from the name.

Are we ok with node in riottest? dist/pythonlibs/riotnode/riotnode/node.py?

miri64 · 2020-06-09T12:58:46Z

riottest? IMHO as this is primarily a test framework it should be immediately, that should be immediately obvious from the name.

Are we ok with node in riottest? dist/pythonlibs/riotnode/riotnode/node.py?

If the node module controls a node under test, yes.

aabadie · 2020-06-09T13:03:39Z

If the node module controls a node under test, yes.

What is a node ? If we talk about networking, it's a node in a mesh network. But otherwise ? I think there's an abuse of the term here.

miri64 · 2020-06-09T13:07:04Z

Then maybe (though maybe confusing with the device terminus within RIOT) device.py?

miri64 · 2020-06-09T13:07:37Z

Or dut.py for Device Under Test.

aabadie · 2020-06-09T13:09:41Z

If we look back at @leandrolanzieri proposals it could also be agent.py. I also like runner.py :)

fjmolinas · 2020-06-09T13:17:08Z

So options would be:

pkgName: riotrunner
- moduleName; runner.py
pkgName: riotagent
- moduleName; agent.py

miri64 · 2020-06-09T13:29:35Z

No, as far as I understood the conversation, the package's name stays riottest either way. The question was about the name of the node sub-module

miri64 · 2020-06-09T13:32:37Z

Remember, that it is also important what the URL will look like / the name on the RIOT-OS org landing page:

https://github.com/RIOT-OS/riottest makes it pretty obvious that this is about testing.

https://github.com/RIOT-OS/riotrunner sounds like something mandatory to get RIOT to run, https://github.com/RIOT-OS/riotagent "is this something like SSH agent for RIOT?". If we want to keep the names of riotnode and node.py together, maybe a more non-functional name is better?

miri64 · 2020-06-09T13:34:13Z

riotctrl for RIOT control? In the sense that it is for controlling if RIOT is working?

kaspar030 · 2020-06-09T14:13:44Z

https://github.com/RIOT-OS/riottest makes it pretty obvious that this is about testing.

but riotnode would only be a small wheel in the whole RIOT testing infrastructure?

miri64 · 2020-06-09T14:15:39Z

https://github.com/RIOT-OS/riottest makes it pretty obvious that this is about testing.

but riotnode would only be a small wheel in the whole RIOT testing infrastructure?

True, but riottest does not claim to be RIOT's whole testing infrastructure IMHO.

aabadie · 2020-06-09T14:20:01Z

Now I think that riottest is too specific and the actual project can be used in other contexts. The actual README says that already: https://github.com/RIOT-OS/RIOT/pull/13612/files#diff-1134727da204f7baca0c000c630766daR4-R9

leandrolanzieri · 2020-06-09T14:25:21Z

As I mentioned here testing is one of the possible use cases for this package. Some people commented that riotnode may imply networking connectivity and that will not be always true.

Any strong opinions against riotdevice?

miri64 · 2020-06-09T14:49:01Z

As I mentioned here testing is one of the possible use cases for this package. Some people commented that riotnode may imply networking connectivity and that will not be always true.

Any strong opinions against riotdevice?

That has the same problem IMHO. As a new contributor I would ask myself: Do I need this to run my device with RIOT?

leandrolanzieri · 2020-06-17T15:10:46Z

This is being moved to its own repo now.

cladmi added 9 commits March 11, 2020 09:06

riotnode: Replace pexpect exception value with pattern

1bcfacb

This allows finer analysis of exception as sometimes the calling line is only `expect(variable)` without knowing the value.

riotnode: Remove exception ctx from inside pexpect implementation

3648088

This allows having a nicely printed output on `pytest`. This could be moved outside of here too.

riotnode/tests: add a test for term sigkill

3213d72

This shows the current handling that could correctly terminate a subprocess that is not always killed. It is here as a regression test when removing the process group sigkill.

riotnode/node.py: use pexpect.close for killing process

a284c40

The process should be closed alone with 'self.term.close'. Closing child processes must be done by the `make term` program.

riotnode/node: remove _kill_term

174f9b9

Remove now unused _kill_term.

leandrolanzieri added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels Mar 11, 2020

leandrolanzieri added this to the Release 2020.04 milestone Mar 11, 2020

leandrolanzieri requested review from miri64, jia200x, aabadie and fjmolinas March 11, 2020 08:26

fjmolinas added this to Backlog / Proposals in RIOT Sprint Day via automation Mar 17, 2020

fjmolinas moved this from Backlog / Proposals to Ready for Review in RIOT Sprint Day Mar 17, 2020

aabadie self-assigned this Mar 19, 2020

aabadie moved this from Ready for Review to Under Review in RIOT Sprint Day Mar 19, 2020

leandrolanzieri modified the milestones: Release 2020.04, Release 2020.07 Apr 8, 2020

fjmolinas moved this from Under Review to In progress in RIOT Sprint Day Apr 14, 2020

fjmolinas removed this from In progress in RIOT Sprint Day Apr 14, 2020

fjmolinas mentioned this pull request Jun 2, 2020

drivers/at86rf215: implement MR-OFDM #14010

Merged

fjmolinas reviewed Jun 9, 2020

View reviewed changes

fjmolinas self-assigned this Jun 9, 2020

leandrolanzieri closed this Jun 17, 2020

aabadie mentioned this pull request Jun 23, 2020

riotnode: node abstraction package #10949

Closed

leandrolanzieri deleted the pr/riotnode_cleanup_squashed branch June 23, 2020 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[take over] riotnode: node abstraction package #13612

[take over] riotnode: node abstraction package #13612

leandrolanzieri commented Mar 11, 2020 •

edited

aabadie commented Mar 24, 2020

miri64 commented Mar 24, 2020

leandrolanzieri commented Mar 25, 2020

miri64 commented Mar 25, 2020

leandrolanzieri commented Apr 16, 2020

fjmolinas commented Apr 16, 2020

aabadie commented Apr 16, 2020

miri64 commented Apr 16, 2020

cladmi commented Apr 17, 2020

cladmi commented Apr 17, 2020

miri64 commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

fjmolinas left a comment

fjmolinas Mar 14, 2020

fjmolinas Mar 14, 2020

fjmolinas Mar 20, 2020

fjmolinas commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

kaspar030 commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

leandrolanzieri commented Jun 9, 2020

miri64 commented Jun 9, 2020

leandrolanzieri commented Jun 17, 2020

	dir_cmd = '--no-print-directory', '-C', self.application_directory
	dir_cmd = '--no-print-directory', '-C', self._application_directory

[take over] riotnode: node abstraction package #13612

[take over] riotnode: node abstraction package #13612

Conversation

leandrolanzieri commented Mar 11, 2020 • edited

Contribution description

Testing procedure

Issues/PRs references

aabadie commented Mar 24, 2020

miri64 commented Mar 24, 2020

leandrolanzieri commented Mar 25, 2020

miri64 commented Mar 25, 2020

leandrolanzieri commented Apr 16, 2020

fjmolinas commented Apr 16, 2020

aabadie commented Apr 16, 2020

miri64 commented Apr 16, 2020

cladmi commented Apr 17, 2020

cladmi commented Apr 17, 2020

miri64 commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas Mar 14, 2020

Choose a reason for hiding this comment

fjmolinas Mar 14, 2020

Choose a reason for hiding this comment

fjmolinas Mar 20, 2020

Choose a reason for hiding this comment

fjmolinas commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

fjmolinas commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

miri64 commented Jun 9, 2020

kaspar030 commented Jun 9, 2020

miri64 commented Jun 9, 2020

aabadie commented Jun 9, 2020

leandrolanzieri commented Jun 9, 2020

miri64 commented Jun 9, 2020

leandrolanzieri commented Jun 17, 2020

leandrolanzieri commented Mar 11, 2020 •

edited