[riot-notifications] [RIOT-OS/RIOT] gnrc_tcp: test improvement (#11930)

Martine Lenders notifications at github.com
Tue Jul 30 20:26:21 CEST 2019


miri64 requested changes on this pull request.

First code review:

> +
+# 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

Please clear this list for now (or comment it out) so we can see which boards really don't fit.

> +
+# 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

Please clear this list for now (or comment it out) so we can see which boards really don't fit.

> +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()

```py
    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as sock:
        sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
        sock.bind(('::', SERVER_PORT))
        sock.listen(1)

    conn, addr = sock.accept()
    conn.close()
```

should assure `sock` is closed.

> +    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

This is very old style. The inclusion of `testrunner` into PYTHONPATH is guaranteed by just running `make test`

> @@ -0,0 +1,60 @@
+#!/usr/bin/env python3

On naming: I'd prefer keeping the current style and name the scripts e.g. `01-conn_lifecycle_as_client.py` without the `test_` prefix.

> +    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)

Why not just exit as all the other scripts to

> +    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)

Also `pep8`: `timeout=3, echo=False, ...` for parameter assignments

> +    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)

Also also: why deactivate traceback? It's really helpful to find the line that failed ;-)

> +    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')

I checked with `echo=True` and this line never comes (nor any similar line).

> +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)

This get's repeated pretty often. You could just have a helper module for that, added to `tests/gnrc_tcp/tests/` in a non-executable python file.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11930#pullrequestreview-268576887
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190730/ad5a6b5b/attachment-0001.htm>


More information about the notifications mailing list