<p><b>@miri64</b> requested changes on this pull request.</p>

<p>First code review:</p><hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308868765">tests/gnrc_tcp/Makefile</a>:</p>
<pre style='color:#555'>> +
+# Basic Configuration
+BOARD ?= native
+
+# Shorten default TCP timeouts to speedup testing
+MSL_US ?= 1000000
+TIMEOUT_US ?= 3000000
+
+# Mark Boards with insufficient memory
+BOARD_INSUFFICIENT_MEMORY := airfy-beacon arduino-duemilanove arduino-mega2560 \
+                             arduino-uno calliope-mini chronos hifive1 mega-xplained microbit \
+                             msb-430 msb-430h nrf51dongle nrf6310 nucleo-f031k6 \
+                             nucleo-f042k6 nucleo-f303k8 nucleo-l031k6 nucleo-f030r8 \
+                             nucleo-f070rb nucleo-f072rb nucleo-f302r8 nucleo-f334r8 nucleo-l053r8 \
+                             sb-430 sb-430h stm32f0discovery telosb \
+                             wsn430-v1_3b wsn430-v1_4 yunjia-nrf51822 z1
</pre>
<p>Please clear this list for now (or comment it out) so we can see which boards really don't fit.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308868893">tests/gnrc_tcp/Makefile</a>:</p>
<pre style='color:#555'>> +
+# Basic Configuration
+BOARD ?= native
+
+# Shorten default TCP timeouts to speedup testing
+MSL_US ?= 1000000
+TIMEOUT_US ?= 3000000
+
+# Mark Boards with insufficient memory
+BOARD_INSUFFICIENT_MEMORY := airfy-beacon arduino-duemilanove arduino-mega2560 \
+                             arduino-uno calliope-mini chronos hifive1 mega-xplained microbit \
+                             msb-430 msb-430h nrf51dongle nrf6310 nucleo-f031k6 \
+                             nucleo-f042k6 nucleo-f303k8 nucleo-l031k6 nucleo-f030r8 \
+                             nucleo-f070rb nucleo-f072rb nucleo-f302r8 nucleo-f334r8 nucleo-l053r8 \
+                             sb-430 sb-430h stm32f0discovery telosb \
+                             wsn430-v1_3b wsn430-v1_4 yunjia-nrf51822 z1
</pre>
<p>Please clear this list for now (or comment it out) so we can see which boards really don't fit.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308870358">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> +import socket
+import threading
+from shared_func import getHostLinkLocalAddr
+from shared_func import getRiotInterfaceId
+
+HOST_IF = 'tap0'
+SERVER_PORT = 15000
+
+def tcpServer():
+    sock = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
+    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+    sock.bind(('::', SERVER_PORT))
+    sock.listen(1)
+
+    conn, addr = sock.accept()
+    conn.close()
</pre>
<div class="highlight highlight-source-python"><pre>    <span class="pl-k">with</span> socket.socket(socket.<span class="pl-c1">AF_INET6</span>, socket.<span class="pl-c1">SOCK_STREAM</span>) <span class="pl-k">as</span> sock:
        sock.setsockopt(socket.<span class="pl-c1">SOL_SOCKET</span>, socket.<span class="pl-c1">SO_REUSEADDR</span>, <span class="pl-c1">1</span>)
        sock.bind((<span class="pl-s"><span class="pl-pds">'</span>::<span class="pl-pds">'</span></span>, <span class="pl-c1">SERVER_PORT</span>))
        sock.listen(<span class="pl-c1">1</span>)

    conn, addr <span class="pl-k">=</span> sock.accept()
    conn.close()</pre></div>
<p>should assure <code>sock</code> is closed.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308870943">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> +    serverHandle.start()
+
+    targetAddr = getHostLinkLocalAddr(HOST_IF) + '%' + getRiotInterfaceId(child)
+
+    # Setup RIOT Node to connect to Hostsystems TCP Server
+    child.sendline('gnrc_tcp_tcb_init')
+    child.sendline('gnrc_tcp_open_active AF_INET6 ' + targetAddr + ' ' + str(SERVER_PORT) + ' 0')
+    child.expect_exact('gnrc_tcp_open_active: returns 0')
+
+    # Close connection
+    child.sendline('gnrc_tcp_close')
+    serverHandle.join()
+
+if __name__ == '__main__':
+    sys.path.append(os.path.join(os.environ['RIOTTOOLS'], 'testrunner'))
+    import testrunner
</pre>
<p>This is very old style. The inclusion of <code>testrunner</code> into PYTHONPATH is guaranteed by just running <code>make test</code></p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308871613">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,60 @@
+#!/usr/bin/env python3
</pre>
<p>On naming: I'd prefer keeping the current style and name the scripts e.g. <code>01-conn_lifecycle_as_client.py</code> without the <code>test_</code> prefix.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308872038">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> +    child.sendline('gnrc_tcp_open_active AF_INET6 ' + targetAddr + ' ' + str(SERVER_PORT) + ' 0')
+    child.expect_exact('gnrc_tcp_open_active: returns 0')
+
+    # Close connection
+    child.sendline('gnrc_tcp_close')
+    serverHandle.join()
+
+if __name__ == '__main__':
+    sys.path.append(os.path.join(os.environ['RIOTTOOLS'], 'testrunner'))
+    import testrunner
+
+    scriptName = os.path.basename(sys.argv[0]);
+    res = 1
+
+    try:
+        res = testrunner.run(testfunc, timeout = 3, echo = False, traceback = False)
</pre>
<p>Why not just exit as all the other scripts to</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308872216">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> +    child.sendline('gnrc_tcp_open_active AF_INET6 ' + targetAddr + ' ' + str(SERVER_PORT) + ' 0')
+    child.expect_exact('gnrc_tcp_open_active: returns 0')
+
+    # Close connection
+    child.sendline('gnrc_tcp_close')
+    serverHandle.join()
+
+if __name__ == '__main__':
+    sys.path.append(os.path.join(os.environ['RIOTTOOLS'], 'testrunner'))
+    import testrunner
+
+    scriptName = os.path.basename(sys.argv[0]);
+    res = 1
+
+    try:
+        res = testrunner.run(testfunc, timeout = 3, echo = False, traceback = False)
</pre>
<p>Also <code>pep8</code>: <code>timeout=3, echo=False, ...</code> for parameter assignments</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308872467">tests/gnrc_tcp/tests/test_01-conn_lifecycle_as_client.py</a>:</p>
<pre style='color:#555'>> +    child.sendline('gnrc_tcp_open_active AF_INET6 ' + targetAddr + ' ' + str(SERVER_PORT) + ' 0')
+    child.expect_exact('gnrc_tcp_open_active: returns 0')
+
+    # Close connection
+    child.sendline('gnrc_tcp_close')
+    serverHandle.join()
+
+if __name__ == '__main__':
+    sys.path.append(os.path.join(os.environ['RIOTTOOLS'], 'testrunner'))
+    import testrunner
+
+    scriptName = os.path.basename(sys.argv[0]);
+    res = 1
+
+    try:
+        res = testrunner.run(testfunc, timeout = 3, echo = False, traceback = False)
</pre>
<p>Also also: why deactivate traceback? It's really helpful to find the line that failed ;-)</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308872932">tests/gnrc_tcp/tests/test_02-conn_lifecycle_as_server.py</a>:</p>
<pre style='color:#555'>> +    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+
+    addrInfo = socket.getaddrinfo(serverAddr + '%' + HOST_IF, SERVER_PORT, type = socket.SOCK_STREAM)
+
+    sock.connect(addrInfo[0][-1])
+    sock.close()
+
+def testfunc(child):
+    clientHandle = threading.Thread(target = tcpClient, args = (getRiotLinkLocalAddr(child),))
+
+    # Setup RIOT Node wait for incomming connections from Hostsystem
+    child.sendline('gnrc_tcp_tcb_init')
+    child.sendline('gnrc_tcp_open_passive AF_INET6 ' + str(SERVER_PORT))
+
+    clientHandle.start()
+    child.expect_exact('gnrc_tcp_open_passive: returns 0')
</pre>
<p>I checked with <code>echo=True</code> and this line never comes (nor any similar line).</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/11930#discussion_r308873429">tests/gnrc_tcp/tests/test_03-send_data.py</a>:</p>
<pre style='color:#555'>> +import sys
+import socket
+import threading
+from shared_func import getHostLinkLocalAddr
+from shared_func import getRiotInterfaceId
+from shared_func import setupTcpBuffer
+from shared_func import addDataToTcpBuffer
+
+HOST_IF = 'tap0'
+SERVER_PORT = 15000
+
+def tcpServer(expectedData):
+    sock = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
+    sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+    sock.bind(('::', SERVER_PORT))
+    sock.listen(1)
</pre>
<p>This get's repeated pretty often. You could just have a helper module for that, added to <code>tests/gnrc_tcp/tests/</code> in a non-executable python file.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/11930?email_source=notifications&email_token=ABE7WYFDFYBN3WKAOMYHBN3QCCBU3A5CNFSM4IHTRDD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCABCQ5Y#pullrequestreview-268576887">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYHA6CYDGP76YW4RJITQCCBU3ANCNFSM4IHTRDDQ">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABE7WYBAAJUKOQIK2Z4MVH3QCCBU3A5CNFSM4IHTRDD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCABCQ5Y.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/11930?email_source=notifications\u0026email_token=ABE7WYFDFYBN3WKAOMYHBN3QCCBU3A5CNFSM4IHTRDD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCABCQ5Y#pullrequestreview-268576887",
"url": "https://github.com/RIOT-OS/RIOT/pull/11930?email_source=notifications\u0026email_token=ABE7WYFDFYBN3WKAOMYHBN3QCCBU3A5CNFSM4IHTRDD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCABCQ5Y#pullrequestreview-268576887",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>