riotctrl_shell: initial import of shell interaction spawn

[miri64]

Contribution description

One thing I really like about the test utils in RIOT-OS/Release-Specs#79 are the mixins to activate functionality to interact with a shell command. Based on that I provide ShellInteraction class here that is supposed to be used as a base class for these kind of mixins. As an example I provided a shell interaction implementation for ping6 and reboot. They allow for something like

    class Pinger(GNRCICMPv6Echo, Reboot):
        pass

    node = riotnode.node.RIOTCtrl(application_directory=`optional_dir_if_not_curr_dir`,
                                  env=node_specific_env_dict)
    pinger = Pinger(node)
    pinger.reboot()
    pinger.ping6("ff02::1")

Testing procedure

I provided some pytest tests

virtualenv -p python3 /tmp/env
. /tmp/env/bin/activate
pip install -r dist/pythonlibs/riotctrl_shell/requirements.txt pytest
pytest dist/pythonlibs/riotctrl_shell/
PYTHONPATH=${PWD}/dist/pythonlibs:${PYTHONPATH} pytest dist/pythonlibs/riotctrl_shell/

If it fails you might just need to initialize the TAP interface first:

sudo ./dist/tools/tapsetup/tapsetup

Issues/PRs references

Depends on #10949 (now on the currently still private but visible for maintainers repo).

[cladmi]

Without looking at the implementation, I am more thinking about a wrapper around node or the expect instance than as a base class to expect.
I do not really want to go with the inheritance scheme here and do more composition.

shell =  MyTermClass(node)
shell.reboot()

One of the reason is it could allow having different wrappers for the same instance. Which is way better for namespacing.

udp_server = MyUDPServer(node)
tcp_server = MyTCPServer(node)

udp_server.listen('1234')
tcp_server.listen('80')

# EDIT: hmm the callback is not configured in python but you can see the naming conflict
udp_server.packet_callback = test_udp_cb
tcp_server.packet_callback = test_tcp_cb

Otherwise every method must be namespaced shell.tcp_server_listen() which feels weird.
And multiple inheritance leads to issues when you cannot really substitute an object for the other anymore.

This still allows to do composed objects with a separate class, that would here be the mixins:

class MySuperClassUnderTest():
    def __init__(self, node):
            self.tcp_server = TCPServerShell(node)
            self.udp_server = UDPServerShell(node)

I am just not sure where to put the Shell but I think I would still prefer to use the shell as a composition and have maybe a provided class already if you really want to just inherit from it.

class BasicShellClass():
    SHELL_CLASS = RiotNodeShell
    def __init__(self, node):
        self.shell = self.SHELL_CLASS(node)
    # maybe with calls being forwarded directly to the shell class to be able to do
    # basicshell.wait_ready()

[miri64]

@cladmi like this (see last commits). I'm not sure about the decorator myself though (neither correctness nor appropriateness 😅)

[miri64]

Note to self: rtt can also be omitted (when packet_size < sizeof(uint32_t)).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[miri64]

Ping? I still think that this can help streamline the tests a lot and automate the release testings.

[miri64]

If @cladmi agrees I could take over his commits.

[aabadie]

If @cladmi agrees I could take over his commits.

With @fjmolinas, we also have the plan to take over this PR.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

[miri64]

Needs adaptation for https://github.com/RIOT-OS/riotctrl/

[miri64]

Rebased to current master, adapted for https://github.com/RIOT-OS/riotctrl/, and removed dependency of ShellInteraction to TermSpawn (it made little sense). There might be a benefit to move ShellInteraction to https://github.com/RIOT-OS/riotctrl/ too. It is basically just a soft wrapper around RIOTCtrl and does not provide any RIOT-specific code.

[miri64]

One of the reason is it could allow having different wrappers for the same instance. Which is way better for namespacing.

udp_server = MyUDPServer(node)
tcp_server = MyTCPServer(node)

udp_server.listen('1234')
tcp_server.listen('80')

# EDIT: hmm the callback is not configured in python but you can see the naming conflict
udp_server.packet_callback = test_udp_cb
tcp_server.packet_callback = test_tcp_cb

Otherwise every method must be namespaced shell.tcp_server_listen() which feels weird.

Yepp. Using Mixins is now not required anymore with the current approach. It is however still possible, if the test implementor prefers so :-).

[miri64]

Regarding riotctrl_shell, I think its Ok but I would remove the class Reboot(ShellInteraction), I think that with a class Help(ShellInteraction) they could be part of a another module that would have only have reboot, help, version interactions. With that I think we can move it to riotctrl repo.

Maybe a sys.py module to match sc_sys.c.

👍

[miri64]

Done.

[fjmolinas]

Done.

IMO you can split riotctrl_shell and push into riotctrl now. @aabadie any objections?

[miri64]

In the last round of commits I

• moved over to riotctrl.shell and removed riotctrl_shell.base and its test.
• simplified and sped up the tests by initializing the RIOTctrl object test-wise (all the special cases for term handling are now tested in riotctrl anyways).
• Made GNRCPktbufStatsParser.Results a non-nested class GNRCPktbufStatsResults, the manipulation of this dict-like items happens in private methods of GNRCPktbufStatsParser, GNRCPktbufStatsResults extends dict, to have the is_empty() and fullest_capacity() methods.

[miri64]

May I squash @aabadie @fjmolinas? It's getting out of hand again ^^"

[aabadie]

I tried to run the tests locally but had issues.

Can you add a tox.ini file in riotctrl_shell with the following content:

[tox]
envlist = test,flake8
skipsdist = True

[testenv]
commands =
    test:       {[testenv:test]commands}
    flake8:     {[testenv:flake8]commands}

[testenv:test]
deps =
    pytest
    -rrequirements.txt
commands =
    pytest -v

[testenv:flake8]
deps = flake8
commands =
    flake8 .

Also add __init__.py in riotctrl_shell and riotctrl_shell/tests
And finally apply the comments below. Then the tests can be run automatically using tox. You can squash the issues reported by flake8 directly (minor unused imports) :)

[miri64]

Applied your latest round of comments and squashed.

[aabadie]

All good for me.

I was able to run the tests locally and they are all green:

Details

tox --recreate
test recreate: /work/riot/RIOT/dist/pythonlibs/riotctrl_shell/.tox/test
test installdeps: pytest, -rrequirements.txt
test installed: attrs==19.3.0,more-itertools==8.4.0,packaging==20.4,pexpect==4.8.0,pluggy==0.13.1,psutil==5.7.0,ptyprocess==0.6.0,py==1.9.0,pyparsing==2.4.7,pytest==5.4.3,riotctrl @ git+ssh://git@github.com/RIOT-OS/riotctrl@dcca00a88e04c19661b60a1e574a869ca9a915bf,six==1.15.0,wcwidth==0.2.5
test run-test-pre: PYTHONHASHSEED='3279086202'
test run-test: commands[0] | pytest -v
============================================================ test session starts ============================================================
platform linux -- Python 3.8.2, pytest-5.4.3, py-1.9.0, pluggy-0.13.1 -- /work/riot/RIOT/dist/pythonlibs/riotctrl_shell/.tox/test/bin/python
cachedir: .tox/test/.pytest_cache
rootdir: /work/riot/RIOT/dist/pythonlibs/riotctrl_shell
collected 12 items                                                                                                                          

tests/test_gnrc.py::test_ping6 PASSED                                                                                                 [  8%]
tests/test_gnrc.py::test_ping6_parser_success PASSED                                                                                  [ 16%]
tests/test_gnrc.py::test_ping6_parser_empty PASSED                                                                                    [ 25%]
tests/test_gnrc.py::test_ping6_parser_missing_rtts PASSED                                                                             [ 33%]
tests/test_gnrc.py::test_pktbuf PASSED                                                                                                [ 41%]
tests/test_gnrc.py::test_pktbuf_parser_success_empty PASSED                                                                           [ 50%]
tests/test_gnrc.py::test_pktbuf_parser_success_not_empty PASSED                                                                       [ 58%]
tests/test_gnrc.py::test_pktbuf_parser_empty PASSED                                                                                   [ 66%]
tests/test_gnrc.py::test_pktbuf_parser_2nd_header_not_found PASSED                                                                    [ 75%]
tests/test_sys.py::test_help PASSED                                                                                                   [ 83%]
tests/test_sys.py::test_reboot PASSED                                                                                                 [ 91%]
tests/test_sys.py::test_version PASSED                                                                                                [100%]

============================================================ 12 passed in 0.03s =============================================================
flake8 recreate: /work/riot/RIOT/dist/pythonlibs/riotctrl_shell/.tox/flake8
flake8 installdeps: flake8
flake8 installed: flake8==3.8.3,mccabe==0.6.1,pycodestyle==2.6.0,pyflakes==2.2.0
flake8 run-test-pre: PYTHONHASHSEED='3279086202'
flake8 run-test: commands[0] | flake8 .
__________________________________________________________________ summary __________________________________________________________________
  test: commands succeeded
  flake8: commands succeeded
  congratulations :)

Let's move forward. ACK

[miri64]

Yay 🎉, thanks for the review and the patience with my impatience 😅

[fjmolinas]

Awesome to see this in!