riotnode: node abstraction package

[cladmi]

Contribution description

This is a start to try providing a common interface that could be used as a test agnostic base that could be used for automating experiments but also, on a shorter term for testing with the current tests, the i2c testing, some framework directions with tests (RobotFramework/pytest), release specs with multiple nodes under test.

Different parallel directions have been taken with different low level wrapper (see refs below). The goal here would be to share a common base where then every usage could benefit of the common developments.

It is currently only a base class without any shell abstraction but wanted to publish early to get feedback and it could also be used in this state. I focused to put the test ecosystem around to help developing.

Usage can be seen in testrunner wrapping and in test_running_error_cases(app_pidfile_env) test.
Current usage in code is

    node = riotnode.node.RIOTNode(application_directory=`optional_dir_if_not_curr_dir`, env=node_specific_env_dict)
    node.start_term(logfile=sys.stdout)  # or use the context manager
    node.term.expect('something')
    node.stop_term()

One important thing I think, is that it implements wrapping for only one node, as it is the only thing we can do in RIOT. Handling two nodes means having two different environments (specific env variables) with two instances of one node.

This pull requests is an aggregate of:

1. empty python package integration with test suite
2. an object implementation of a node abstraction. It gives access to the serial port as an expect spawn object. So somehow what testrunner does but with a different interface.
3. @miri64 pull request dist/pythonlibs: provide unittest TestCase wrapper for testrunner #10431
4. Usage of this object with previous pull request
5. Some other changes that could come in

Upcoming work

After different IRL discussions, I am thinking about providing other abstraction classes for shell commands more as a composition than inherited objects (or more let other provide abstractions, as it is the goal to share this between different developers).
And implemented with independent classes that you use alone instead of an aggregated object with everything in the same object.
This could allow a light usage with a specific object, while still allowing to create an aggregated objects if needed but not enforcing it.

class RIOTNodeShellIfconfig():
    RIOTNODE_SHELL_CLASS = RIOTNodeShell
    def __init__(self, node):
        self.node = node
        self.term = self.RIOTNODE_SHELL_CLASS(node)
    def get_default_interface(self):
        ret = self.term.send_command("ifconfig")
        interface = # parse ret to get the correct string
        return int(interface)


node = RIOTNode()
ifconfig_node = RIOTNodeShellIfconfig(node)
interface_num = ifconfig_node.get_default_interface()

This one could still maybe use a common RIOTNodeShellWrapper as base class, it is thing that should be discussed depending on the usages.

Reviewing procedure

I would advise to review commit by commit as they are focusing on different concepts.
Also see if the abstraction could currently work for you.

I still have TODO comments in the code I want to address before merging I would gladly get feedback on them. They are shown when running tox test command.
There is also a TODO.rst for things that were here before in testrunner and could be addressed later.

I was told that node that I took from my work on IoT-LAB was maybe not a good name for this.
I did not choose board as it as the BOARD meaning, you could have two boards being the same BOARD but maybe it is just an issue with me.

Please show me where this could help you or not and if you already have needs around this.
And also, if you have issues with the current implementation.

Testing procedure

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

Note: there should now be TODO` warning as I thought about something I want to address before any merging.

See if it can be used a as base for your cases and if you have feedback on the next steps.

Issues/PRs references

Depend on having a terminal without decoration/additional behavior:

• Makefile.include: add cleanterm target and use it for tests Makefile.include: add cleanterm target and use it for tests #12107

My comment on RobotFramework RFC #10241 (comment)

Some example place where it could be used as a base

• testutils: add helpers for running Release Specs Release-Specs#79
• https://github.com/RIOT-OS/Release-Specs/blob/a1a46649574cc2b7885869789fd924ad672d2915/07-multi-hop/IOTLABHelper.py (but it currently uses serial_aggregator as a base)
• our tests, even more for shell tests
• as a base for a RIOTDriver in riot_pal.

[miri64]

ret = self.term.send_command("ifconfig get_interfaces")

Either you are sending a wrong command here or I don't really understand what you are doing in the background and why (the command is just ifconfig to get all the interfaces ;-)).

[cladmi]

The issue I noted in here for matching stdin is actually a bug in the current implementation I will do a pull request to fix it.
Funny how just writing a simple echo test shows issues.

[cladmi]

ret = self.term.send_command("ifconfig get_interfaces")

Either you are sending a wrong command here or I don't really understand what you are doing in the background and why (the command is just ifconfig to get all the interfaces ;-)).

It was a test to see who will read this, I updated the code :p

[cladmi]

I added tests for process management and started refactoring commits (so rewrote history) to go in a direction where bug fixes could be merged alone and used in the current testrunner.

As long as a name is chosen for the package, some 'utils' functions could be added and used.
I do not like fixing bugs by replacing everything at once so iterating would be easier.

[cladmi]

After one night on this, should the wrapper really take care of a term that cannot clean itself correctly ?

It requires adding psutil dependency (or play with ps output) and doing complex and not guaranteed solutions (if a new process is spawned after the first kill) to detect an issue where the terminal process would not correctly clean its children.

Which means, when a user uses make term and does not do a process group kill, it could leave remaining processes.

I would be maybe in favor of documenting correctly to which signals the TERMPROG should react to cleanup and fix them to work properly.

[tcschmidt]

I asked feedback from @adjih and he suggested the alternative name riotdevice instead of riotnode.
Or my funny one riotthing as it is the internet of things :p

On a non-meta level, device relates to a physical board and thing relates to a real-world entity that is represented by an IoT device ... so IMO rather misleading. Node fits better from the perspective of an Internet member ... more generically, agent could be a choice or authority?

[kb2ma]

Thanks for adding me as a reviewer. Until I get a chance to review, please take a look at my pytest repo for CoAP. In particular see conftest.py for a pexpect host class.

[cladmi]

@kb2ma I somehow try to provide the same, in a riot specific way. So without specifying "make term" as argument because it is "of course" make term. Currently I do not have the "send_recv" as I see it more maybe as an other layer on top.

I like your "nanocoap_server", "gcoap_example" fixtures. "Return me a firmware in a ready state to use". A plan I have for further steps, would be to add a class describing this "nanocoap_server" and the operations you could have on it. Not mandatory as you could still use the "node.term" but giving you other methods that could be useful.

[cladmi]

However, my idea here, would be to give the nanocoap_server object, but not describe it as a fixture.
Describing as a fixture would be in an adaptation layer for the test (it should be almost transparent to implement by saying, wrap this as a) as I would like to keep this non test framework related.

[miri64]

How about

Suggested change

	self.logger.critical('Could not close make term')
	self.logger.critical('Could not close make term')
	finally:
	self.term = None

? Thes way one can check (e.g. in a subclass) if the terminal is running or not.

[cladmi]

I may even maybe put a started variable. As currently when starting we wait some time. I will see with the multi nodes handling.
Also, the start could just do nothing if it is already started.

However I did not really see a use case for calling start multiple times. In the current tests, the test function receive a started child. So I assumed to somehow have this initialization done before anything else.

[cladmi]

I will look into the usage of the asyncio integration of pexpect.
It would allow using multiple nodes more easily if you want to expect from multiple nodes at the same time to find for example "who received the message" for asynchronous events.

I also found pexpect shell wrapper https://pexpect.readthedocs.io/en/stable/api/replwrap.html It was added in pexpect 4.3 so not the ubuntu bionic version but it could be temporarily extracted if needed or require an upgraded pexpect version.

[fjmolinas]

I think this is a great contribution that could help improving our test suit. I can see a direct usage in putting in common a lot of the riotboot functions that are used in #11843, #11818 & #11808. It could definitively help with avoiding that much code duplication. Same goes for other WIP branch I have been working on.

@kb2ma I somehow try to provide the same, in a riot specific way. So without specifying "make term" as argument because it is "of course" make term. Currently I do not have the "send_recv" as I see it more maybe as an other layer on top.

I like having a "send_recv" function, I've ended up implementing something like that many times.

I like your "nanocoap_server", "gcoap_example" fixtures. "Return me a firmware in a ready state to use". A plan I have for further steps, would be to add a class describing this "nanocoap_server" and the operations you could have on it. Not mandatory as you could still use the "node.term" but giving you other methods that could be useful.

+1

https://github.com/RIOT-OS/Release-Specs/blob/a1a46649574cc2b7885869789fd924ad672d2915/07-multi-hop/IOTLABHelper.py (but it currently uses serial_aggregator as a base)

An iotlab node extension would be great to have. It would help scripting "manually automatic" (#11954) integrations tests for our release specs, and other features.

I think we should work on integrating this, it will definitively help with having common building blocks.

[fjmolinas]

@cladmi was there a reason why you wanted this in RIOT and not as an external pkg, hosted in RIOT-OS for examples. I would like to make this available, although maybe not yet used since Feature freeze is coming soon.

[cladmi]

@fjmolinas It would be mapped to exactly the exposed interface of RIOT shell so subject to change at any commit. Keeping them in the same repo allows staying in sync without having to merge the tool PR before the code modification.
Also, I am not sure how packages for make test would work in CI, and adding a package for each test may not be the best.

But it is a long term and ecosystem concern, during dev or release you could store it elsewhere indeed.

[fjmolinas]

@miri64 @aabadie Thoughts on this one? Should we postpone again or target integrating as a tool although not used? It could get the ball rolling, it would basically just mean removing the usage in testrunner? I have started to use it to refactor some of the Release-Speces tests, so far it is fitting my use case.

[miri64]

I would like to have this in rather sooner than later, even if there are things still to kink out. As you said, this would rather get the ball rolling (also we can go ahead with #11406).

[fjmolinas]

I would like to have this in rather sooner than later, even if there are things still to kink out. As you said, this would rather get the ball rolling (also we can go ahead with #11406).

I would propose going with but without the modifications to testrunner, they can be merged after the release. This also reduces the impact of the change and gives us more time before feature freeze. @aabadie would you agree?

[fjmolinas]

Suggested change

	return self.env['BOARD']
	return self.env.get('BOARD')

so it doesn't return a key error, or is this on purpose?

[aabadie]

#13612 was closed because the project moved to https://github.com/riot-os/riotctrl