gnrc_tcp: test improvement #11930

brummer-simon · 2019-07-29T15:20:51Z

Dear RIOTers,

The existing GNRC_TCP tests have their issues therefore a complete rewrite was in order.

The old test were two RIOT applications communicating with each other, testing client and server role, stream segmentation, window reopening and connection termination and the test had to be executed manually.

This PR replaces the existing test applications with four python based test scripts:

Connection life-cycle as client
Connection life-cycle as server
Sending of data (requires sending of a stream over multiple pakets)
Receiving of data (with receive window opening and closing)

The new tests require only a single RIOT instance and can be run in an automated test environment.
During the tests RIOT is communicating with the host systems TCP implementation. Because of this the test run currently only under native. The entire test should work under non-native to with "ethos", however I was not able to get it to run.

From my point of view this PR is already a huge improvement over the existing tests and could be merged as it is. If there are is anything on "how to use/setup ethos" I would be glad to add it to this PR.

Cheers Simon

[EDIT]: I was able to adress the remaining issues. The test works now on via ethos no non-native platforms. I was able to run it on a nucleo-f401re board. Additionally the test runs on bridged tap devices as well (those are create by dist/tools/tapsetup/tapsetup).

Fixes #9324

miri64 · 2019-07-30T18:15:41Z

Tested on native and samr21-xpro:

[…]
Timeout in expect script at "child.expect_exact('gnrc_tcp_open_passive: returns 0')" (tests/gnrc_tcp/tests/test_02-conn_lifecycle_as_server.py:36)

test_02-conn_lifecycle_as_server.py: failed.
[…]

miri64

First code review:

tests/gnrc_tcp/Makefile

tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py

tests/gnrc_tcp/tests/test_02-conn_lifecycle_as_server.py

tests/gnrc_tcp/tests/test_03-send_data.py

miri64

Tests work now.

However, please have a look at the Travis output. I recommend to install flake8 to check locally.

I also recommend to include gnrc_pktbuf_cmd to check if the packet buffer is cleaned properly.

Here are some more comments from my side:

tests/gnrc_tcp/tests/shared_func.py

tests/gnrc_tcp/README.md

tests/gnrc_tcp/main.c

tests/gnrc_tcp/tests/01-conn_lifecycle_as_client.py

tests/gnrc_tcp/tests/03-send_data.py

tests/gnrc_tcp/tests/shared_func.py

tests/gnrc_tcp/Makefile

brummer-simon · 2019-08-07T14:51:43Z

However, please have a look at the Travis output. I recommend to install flake8 to check locally.
I also recommend to include gnrc_pktbuf_cmd to check if the packet buffer is cleaned properly.

After adding "USEMODULE += gnrc_pktbuf_cmd" I have the pktbuf command.
I has no help and no output. What do I need to send to check if the packetbuffer is cleared ?

miri64 · 2019-08-07T15:14:30Z

I has no help and no output. What do I need to send to check if the packetbuffer is cleared ?

I think you need the od module to have it print something (it takes no parameters so you won't require help). If there is no hexdump, you are in the clear, as this means there is no data (represented by the hexdump) in the packet buffer.

brummer-simon · 2019-08-08T08:03:41Z

As far as I see I have solved all requested changes, exect subprocess.Popen. I wasn't able to get it to run and even if I were able to use it, usage would bloat every usage by a few additional lines. Miri64 if you are okay with it I would like to stick os.popen.

Aside from that I think this PR is ready for CI right?

brummer-simon · 2019-08-27T04:53:12Z

@miri64 - Is there anyything left todo with this PR? I would like to have it soon merged

brummer-simon · 2019-08-30T17:37:12Z

I guess its time to add the BOARD_INSUFFICIENT_MEMORY variable in the Makefile. :D

miri64 · 2019-08-30T17:43:06Z

@miri64 - Is there anyything left todo with this PR? I would like to have it soon merged

Sorry this somewhat got drowned in my post-vacation e-mail franzy ^^. I'll have a look at it next week.

brummer-simon · 2019-08-30T17:45:07Z

@miri64 - Is there anyything left todo with this PR? I would like to have it soon merged

Sorry this somewhat got drowned in my post-vacation e-mail franzy ^^. I'll have a look at it next week.

Until then I'll check the Build log to complete the INSUFFICIENT_MEMORY section.

miri64 · 2019-08-30T17:48:39Z

There are some camelCases in the C files. Please stick to our coding conventions

brummer-simon · 2019-09-21T08:48:31Z

@miri64: Ping!

miri64 · 2019-09-23T10:53:28Z

That's the problem with force pushes ;-) unless you say something I don't get any notification.

miri64 · 2019-09-30T13:05:41Z

native still works

samr21-xpro still doesn't run reliably with test 4 :-/. However, I think it is just because of a too tight timeout for the expect. I ran it a couple of times (>10) with

diff --git a/tests/gnrc_tcp/tests/04-receive_data.py b/tests/gnrc_tcp/tests/04-receive_data.py
index 23d246d83f..f43634f035 100755
--- a/tests/gnrc_tcp/tests/04-receive_data.py
+++ b/tests/gnrc_tcp/tests/04-receive_data.py
@@ -43,7 +43,7 @@ def testfunc(child):
 
     # Accept Data sent by the host system
     child.sendline('gnrc_tcp_recv 1000000 ' + str(data_len))
-    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len))
+    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len), timeout=20)
 
     # Close connection and verify that pktbuf is cleared
     child.sendline('gnrc_tcp_close')

and didn't run into any errors yet.

Please add a note that due to sudo ./start_network.sh ... the test requires sudo (I always get an error without any indication what's happening). See e.g.

RIOT/tests/gnrc_ipv6_ext/tests/01-run.py

Lines 641 to 645 in fe6d892

    
           if os.geteuid() != 0: 
        
               print("\x1b[1;31mThis test requires root privileges.\n" 
        
                     "It's constructing and sending Ethernet frames.\x1b[0m\n", 
        
                     file=sys.stderr) 
        
               sys.exit(1)

miri64 · 2019-09-30T13:08:05Z

Please add a note that due to sudo ./start_network.sh ... the test requires sudo (I always get an error without any indication what's happening). See e.g.

RIOT/tests/gnrc_ipv6_ext/tests/01-run.py

Lines 641 to 645 in fe6d892

if os.geteuid() != 0:

print("\x1b[1;31mThis test requires root privileges.\n"

"It's constructing and sending Ethernet frames.\x1b[0m\n",

file=sys.stderr)

sys.exit(1)

This should also contain a check if native is used (as we do not need sudo for that). See e.g.

RIOT/tests/gnrc_ipv6_ext_frag/tests/01-run.py

Line 356 in fe6d892

if os.environ.get("BOARD", "") != "native":

brummer-simon · 2019-10-03T11:30:39Z

* `native` still works

* `samr21-xpro` still doesn't run reliably with test 4 :-/. However, I think it is just because of a too tight timeout for the expect. I ran it a couple of times (>10) with
  ```diff
  diff --git a/tests/gnrc_tcp/tests/04-receive_data.py b/tests/gnrc_tcp/tests/04-receive_data.py
  index 23d246d83f..f43634f035 100755
  --- a/tests/gnrc_tcp/tests/04-receive_data.py
  +++ b/tests/gnrc_tcp/tests/04-receive_data.py
  @@ -43,7 +43,7 @@ def testfunc(child):
   
       # Accept Data sent by the host system
       child.sendline('gnrc_tcp_recv 1000000 ' + str(data_len))
  -    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len))
  +    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len), timeout=20)
   
       # Close connection and verify that pktbuf is cleared
       child.sendline('gnrc_tcp_close')
  ```
  
  
  and didn't run into any errors yet.

Please add a note that due to sudo ./start_network.sh ... the test requires sudo (I always get an error without any indication what's happening). See e.g.

RIOT/tests/gnrc_ipv6_ext/tests/01-run.py

Lines 641 to 645 in fe6d892

    
           if os.geteuid() != 0: 
        
               print("\x1b[1;31mThis test requires root privileges.\n" 
        
                     "It's constructing and sending Ethernet frames.\x1b[0m\n", 
        
                     file=sys.stderr) 
        
               sys.exit(1)

It is a good idea to loosen the timeout because different boards will behave differently. I'll add the requested changes.

miri64 · 2019-10-03T11:33:26Z

It is a good idea to loosen the timeout because different boards will behave differently. I'll add the requested changes.

Well additionally the timeout you configured for the expect is lesser than the ones you provide to the shell command, which makes it impossible for the shell command to finish ;-).

brummer-simon · 2019-10-03T12:20:06Z

It is a good idea to loosen the timeout because different boards will behave differently. I'll add the requested changes.

Well additionally the timeout you configured for the expect is lesser than the ones you provide to the shell command, which makes it impossible for the shell command to finish ;-).

Was it? the given timeout is specified in µs and I think it was only a 1000000µs = 1s.

miri64 · 2019-10-03T12:26:28Z

Oh, sorry I thought it was 10s ^^"

brummer-simon · 2019-10-03T12:26:48Z

Hopefully final changes:

Added sudo check before test execution.
Add synchronization before connection teardown (had some issues on nucleo-f401re)
Set the pexpect timeout in each script to 20. Should be enough even for the slowest boards.
Set the timeout in gnrc_tcp_recv to 2000000µs (the timeout is only there to prevent -EAGAIN from the test application)

@miri64: It works for me on native as well as nucleo-f401re. If you give me an okay for the latest changes I'll squash everything to merge it.

tests/gnrc_tcp/README.md

tests/gnrc_tcp/tests/shared_func.py

miri64 · 2019-10-03T12:31:37Z

tests/gnrc_tcp/tests/04-receive_data.py

@@ -58,4 +62,5 @@ def testfunc(child):


 if __name__ == '__main__':
-    sys.exit(run(testfunc, timeout=3, echo=False, traceback=True))
+    sudo_guard()
+    sys.exit(run(testfunc, timeout=20, echo=False, traceback=True))


Why make it 20 for all expects? It's enough to keep it to the

child.expect_exact('gnrc_tcp_recv: received ' + str(data_len), timeout=20)

is enough.

miri64 · 2019-10-03T12:32:39Z

tests/gnrc_tcp/tests/04-receive_data.py

@@ -42,10 +45,11 @@ def testfunc(child):
    child.expect_exact('gnrc_tcp_open_active: returns 0')

    # Accept Data sent by the host system
-    child.sendline('gnrc_tcp_recv 1000000 ' + str(data_len))
+    child.sendline('gnrc_tcp_recv 20000000 ' + str(data_len))


Why this change? It should remain lower then the expect timeout. Additionally, the test still failed when I set the expect timeout to e.g. 10, so increasing this timeout could make this test less reliable.

If this timeout fires the test is failed anyway. It is just there to wait for data instead of an immediately return with -EAGAIN. I though aligning it with the pexpect timeout is okay because this timeout should not fire anyway.

miri64 · 2019-10-03T12:34:12Z

Added sudo check before test execution.

Add synchronization before connection teardown (had some issues on nucleo-f401re)

Set the pexpect timeout in each script to 20. Should be enough even for the slowest boards.

Set the timeout in gnrc_tcp_recv to 2000000µs (the timeout is only there to prevent -EAGAIN from the test application)

Sorry saw this comment only after reviewing

miri64 · 2019-10-03T12:35:54Z

Still I can't follow the decision for your new timeout. My suggestion was just to increase the timeout for this line

    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len), timeout=20)

Everything else makes

the tests slower than they have to be
result in test fails on other boards (due to the increased timeout on the board side)

brummer-simon · 2019-10-03T12:55:54Z

Still I can't follow the decision for your new timeout. My suggestion was just to increase the timeout for this line
    child.expect_exact('gnrc_tcp_recv: received ' + str(data_len), timeout=20)
Everything else makes
1. the tests slower than they have to be

2. result in test fails on other boards (due to the increased timeout on the board side)

I just wanted to give the test some headroom for slower boards. To be finally clear on timouts I will use:

5s as pexpect default.
1s for gnrc_tcp_recv
20s for the pexpect on waiting for gnrc_tcp_recv to return

Are you okay with that?

miri64 · 2019-10-03T13:11:39Z

* 5s as pexpect default.

* 1s for gnrc_tcp_recv

* 20s for the pexpect on waiting for gnrc_tcp_recv to return

Are you okay with that?

👍 as a general rule: higher timeouts should only be selectively chosen, if and only if a board shows too slow response. Everything else would be premature.

brummer-simon · 2019-10-03T13:14:26Z

Okay latest version pushed. Should be all fine now.

miri64 · 2019-10-03T13:17:03Z

Ok. Let's finally get this merged, so please squash! Though I am not currently able to retest on a board, the code is straight forward enough to ensure for myself that the tests will pass on a non-native board.

brummer-simon · 2019-10-03T13:19:17Z

Ok. Let's finally get this merged, so please squash! Though I am not currently able to retest on a board, the code is straight forward enough to ensure for myself that the tests will pass on a non-native board.

Done finally. :D

After merging we can close the Issue #9324 right?

miri64

ACK. Tests were run on samr21-xpro in a previous iteration with similar local edits to the current version and on native in the current version on an Ubuntu 19.04

Operating System Environment
-----------------------------
       Operating System: "Ubuntu" "19.04 (Disco Dingo)"
                 Kernel: Linux 5.0.0-29-generic x86_64 x86_64

Installed compiler toolchains
-----------------------------
             native gcc: gcc (Ubuntu 8.3.0-6ubuntu1) 8.3.0
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 8-2018-q4-major) 8.2.1 20181213 (release) [gcc-8-branch revision 267074]
                avr-gcc: avr-gcc (GCC) 5.4.0
       mips-mti-elf-gcc: missing
             msp430-gcc: msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched)
   riscv-none-embed-gcc: missing
   xtensa-esp32-elf-gcc: missing
   xtensa-lx106-elf-gcc: missing
                  clang: clang version 8.0.0-3 (tags/RELEASE_800/final)

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "3.0.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
xtensa-esp32-elf-newlib: missing
xtensa-lx106-elf-newlib: missing
               avr-libc: "2.0.0" ("20150208")

Installed development tools
---------------------------
                  cmake: cmake version 3.13.4
               cppcheck: missing
                doxygen: 1.8.13
                    git: git version 2.20.1
                   make: GNU Make 4.2.1
                openocd: Open On-Chip Debugger 0.10.0+dev-00910-g4dbcb1e7 (2019-06-17-16:24)
                 python: Python 2.7.16
                python2: Python 2.7.16
                python3: Python 3.7.3
                 flake8: 3.6.0 (mccabe: 0.6.1, pycodestyle: 2.4.0, pyflakes: 2.0.0) CPython 3.7.3 on Linux
             coccinelle: missing

miri64 · 2019-10-03T13:24:44Z

After merging we can close the Issue #9324 right?

Even better: due to your "Fixes #9324" in the original comment, GitHub will do that automatically ;-)

brummer-simon · 2019-10-03T13:32:27Z

After merging we can close the Issue #9324 right?

Even better: due to your "Fixes #9324" in the original comment, GitHub will do that automatically ;-)

Amazing :D

miri64 · 2019-10-03T13:33:17Z

After merging we can close the Issue #9324 right?

Even better: due to your "Fixes #9324" in the original comment, GitHub will do that automatically ;-)

Amazing :D

The wonders of modern technology! :-)

miri64 · 2019-10-03T13:41:01Z

Aand done. Thanks for your patience :-)

brummer-simon · 2019-10-03T13:42:45Z

Youre Welcome. And thanks for reviewing bloated PRs. :)

miri64 self-assigned this Jul 30, 2019

miri64 self-requested a review July 30, 2019 16:57

miri64 requested changes Jul 30, 2019

View reviewed changes

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from ed493b9 to ce7dd50 Compare August 7, 2019 11:19

miri64 reviewed Aug 7, 2019

View reviewed changes

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from ce7dd50 to 8ef8322 Compare August 7, 2019 14:29

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from 8ef8322 to 9100d1b Compare August 8, 2019 07:57

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from 9100d1b to 93196e3 Compare August 9, 2019 07:10

miri64 mentioned this pull request Aug 10, 2019

gnrc_tcp: tests need work #9324

Closed

3 tasks

brummer-simon mentioned this pull request Aug 27, 2019

gnrc_tcp: add minimum length check to _receive #12084

Merged

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 30, 2019

brummer-simon force-pushed the gnrc_tcp-improve_tests branch 3 times, most recently from b241f1c to 850b4f7 Compare September 1, 2019 10:57

miri64 added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Sep 23, 2019

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from 3b51c1b to 1433a13 Compare October 3, 2019 12:18

miri64 requested changes Oct 3, 2019

View reviewed changes

gnrc_tcp: test improvement

56998e8

brummer-simon force-pushed the gnrc_tcp-improve_tests branch from dce11ca to 56998e8 Compare October 3, 2019 13:18

miri64 approved these changes Oct 3, 2019

View reviewed changes

miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 3, 2019

miri64 added this to the Release 2019.10 milestone Oct 3, 2019

miri64 merged commit e9e0001 into RIOT-OS:master Oct 3, 2019

brummer-simon deleted the gnrc_tcp-improve_tests branch October 3, 2019 13:40

This was referenced Oct 3, 2019

gnrc_tcp: check if option has valid length set #12368

Merged

tests/gnrc_tcp: provide regression tests for fixed issues #12369

Merged

gnrc_tcp: test improvement #11930

gnrc_tcp: test improvement #11930

Conversation

brummer-simon commented Jul 29, 2019 • edited by miri64 Loading

miri64 commented Jul 30, 2019

miri64 left a comment

Choose a reason for hiding this comment

miri64 left a comment

Choose a reason for hiding this comment

brummer-simon commented Aug 7, 2019

miri64 commented Aug 7, 2019

brummer-simon commented Aug 8, 2019 • edited Loading

brummer-simon commented Aug 27, 2019

brummer-simon commented Aug 30, 2019

miri64 commented Aug 30, 2019

brummer-simon commented Aug 30, 2019 • edited Loading

miri64 commented Aug 30, 2019

brummer-simon commented Sep 21, 2019

miri64 commented Sep 23, 2019

miri64 commented Sep 30, 2019

miri64 commented Sep 30, 2019

brummer-simon commented Oct 3, 2019

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019

miri64 Oct 3, 2019

Choose a reason for hiding this comment

miri64 Oct 3, 2019

Choose a reason for hiding this comment

brummer-simon Oct 3, 2019

Choose a reason for hiding this comment

miri64 commented Oct 3, 2019

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019 • edited Loading

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019 • edited Loading

miri64 left a comment

Choose a reason for hiding this comment

miri64 commented Oct 3, 2019

brummer-simon commented Oct 3, 2019

miri64 commented Oct 3, 2019

miri64 commented Oct 3, 2019 • edited Loading

brummer-simon commented Oct 3, 2019

brummer-simon commented Jul 29, 2019 •

edited by miri64

Loading

brummer-simon commented Aug 8, 2019 •

edited

Loading

brummer-simon commented Aug 30, 2019 •

edited

Loading

brummer-simon commented Oct 3, 2019 •

edited

Loading

brummer-simon commented Oct 3, 2019 •

edited

Loading

miri64 commented Oct 3, 2019 •

edited

Loading