gnrc_tcp: test improvement#11930
gnrc_tcp: test improvement#11930miri64 merged 1 commit intoRIOT-OS:masterfrom brummer-simon:gnrc_tcp-improve_tests
Conversation
|
Tested on |
miri64
left a comment
There was a problem hiding this comment.
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:
After adding "USEMODULE += gnrc_pktbuf_cmd" I have the pktbuf command. |
I think you need the |
|
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? |
|
@miri64 - Is there anyything left todo with this PR? I would like to have it soon merged |
|
I guess its time to add the BOARD_INSUFFICIENT_MEMORY variable in the Makefile. :D |
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. |
|
There are some camelCases in the C files. Please stick to our coding conventions |
|
@miri64: Ping! |
|
That's the problem with force pushes ;-) unless you say something I don't get any notification. |
miri64
left a comment
There was a problem hiding this comment.
Mostly, there is nothing to add. The test does not work on a non-native board due to some misconfiguration though (see my comment in the application's Makefile). uncrustify has the following to say about the application's Makefile:
diff --git a/tests/gnrc_tcp/main.c b/tests/gnrc_tcp/main.c
index 8a40a19dca..a2eb980f74 100644
--- a/tests/gnrc_tcp/main.c
+++ b/tests/gnrc_tcp/main.c
@@ -35,7 +35,8 @@ int get_af_family(char *family)
{
if (memcmp(family, "AF_INET6", sizeof("AF_INET6")) == 0) {
return AF_INET6;
- } else if (memcmp(family, "AF_INET", sizeof("AF_INET")) == 0) {
+ }
+ else if (memcmp(family, "AF_INET", sizeof("AF_INET")) == 0) {
return AF_INET;
}
return AF_UNSPEC;
@@ -105,7 +106,8 @@ int gnrc_tcp_open_active_cmd(int argc, char **argv)
uint16_t target_port = atol(argv[3]);
uint16_t local_port = atol(argv[4]);
- int err = gnrc_tcp_open_active(&tcb, af_family, target_addr, target_port, local_port);
+ int err = gnrc_tcp_open_active(&tcb, af_family, target_addr, target_port,
+ local_port);
switch (err) {
case -EAFNOSUPPORT:
printf("%s: returns -EAFNOSUPPORT\n", argv[0]);
@@ -150,7 +152,8 @@ int gnrc_tcp_open_passive_cmd(int argc, char **argv)
if (argc == 3) {
local_port = atol(argv[2]);
- } else if (argc == 4) {
+ }
+ else if (argc == 4) {
local_addr = argv[2];
local_port = atol(argv[3]);
}
@@ -209,7 +212,7 @@ int gnrc_tcp_send_cmd(int argc, char **argv)
sent += ret;
}
- printf("%s: sent %u\n", argv[0], (unsigned) sent);
+ printf("%s: sent %u\n", argv[0], (unsigned)sent);
return sent;
}
@@ -222,7 +225,8 @@ int gnrc_tcp_recv_cmd(int argc, char **argv)
size_t rcvd = 0;
while (rcvd < to_receive) {
- int ret = gnrc_tcp_recv(&tcb, buffer + rcvd, to_receive - rcvd, timeout);
+ int ret =
+ gnrc_tcp_recv(&tcb, buffer + rcvd, to_receive - rcvd, timeout);
switch (ret) {
case -EAGAIN:
printf("%s: returns -EAGAIN\n", argv[0]);
@@ -247,7 +251,7 @@ int gnrc_tcp_recv_cmd(int argc, char **argv)
rcvd += ret;
}
- printf("%s: received %u\n", argv[0], (unsigned) rcvd);
+ printf("%s: received %u\n", argv[0], (unsigned)rcvd);
return 0;
}
@@ -267,18 +271,25 @@ int gnrc_tcp_abort_cmd(int argc, char **argv)
/* Exporting GNRC TCP Api to for shell usage */
static const shell_command_t shell_commands[] = {
- {"gnrc_tcp_tcb_init", "gnrc_tcp: init tcb", gnrc_tcp_tcb_init_cmd},
- {"gnrc_tcp_open_active", "gnrc_tcp: open active connection", gnrc_tcp_open_active_cmd},
- {"gnrc_tcp_open_passive", "gnrc_tcp: open passive connection", gnrc_tcp_open_passive_cmd},
- {"gnrc_tcp_send", "gnrc_tcp: send data to connected peer", gnrc_tcp_send_cmd},
- {"gnrc_tcp_recv", "gnrc_tcp: recv data from connected peer", gnrc_tcp_recv_cmd},
- {"gnrc_tcp_close", "gnrc_tcp: close connection gracefully", gnrc_tcp_close_cmd},
- {"gnrc_tcp_abort", "gnrc_tcp: close connection forcefully", gnrc_tcp_abort_cmd},
- {"buffer_init", "init internal buffer", buffer_init_cmd},
- {"buffer_get_max_size", "get max size of internal buffer", buffer_get_max_size_cmd},
- {"buffer_write", "write data into internal buffer", buffer_write_cmd},
- {"buffer_read", "read data from internal buffer", buffer_read_cmd},
- {NULL, NULL, NULL}
+ { "gnrc_tcp_tcb_init", "gnrc_tcp: init tcb", gnrc_tcp_tcb_init_cmd },
+ { "gnrc_tcp_open_active", "gnrc_tcp: open active connection",
+ gnrc_tcp_open_active_cmd },
+ { "gnrc_tcp_open_passive", "gnrc_tcp: open passive connection",
+ gnrc_tcp_open_passive_cmd },
+ { "gnrc_tcp_send", "gnrc_tcp: send data to connected peer",
+ gnrc_tcp_send_cmd },
+ { "gnrc_tcp_recv", "gnrc_tcp: recv data from connected peer",
+ gnrc_tcp_recv_cmd },
+ { "gnrc_tcp_close", "gnrc_tcp: close connection gracefully",
+ gnrc_tcp_close_cmd },
+ { "gnrc_tcp_abort", "gnrc_tcp: close connection forcefully",
+ gnrc_tcp_abort_cmd },
+ { "buffer_init", "init internal buffer", buffer_init_cmd },
+ { "buffer_get_max_size", "get max size of internal buffer",
+ buffer_get_max_size_cmd },
+ { "buffer_write", "write data into internal buffer", buffer_write_cmd },
+ { "buffer_read", "read data from internal buffer", buffer_read_cmd },
+ { NULL, NULL, NULL }
};
int main(void)Also, there is a last nit on documentation. Other than those three things, I think we are ready to merge this
|
@brummer-simon and I discussed this offline, but something broke in test 2 the latest rebase and adaptation. This is why I took this PR out of the CI queue for now. |
|
@Miri64- Would you do me a favor and test this again on your samr32-xpro ? |
Will do in the coming week. |
Please add a note that due to RIOT/tests/gnrc_ipv6_ext/tests/01-run.py Lines 641 to 645 in fe6d892 |
This should also contain a check if native is used (as we do not need RIOT/tests/gnrc_ipv6_ext_frag/tests/01-run.py Line 356 in fe6d892 |
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. |
|
Oh, sorry I thought it was 10s ^^" |
|
Hopefully final changes:
@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. |
| 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)) |
There was a problem hiding this comment.
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.
|
|
||
| # 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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Sorry saw this comment only after reviewing |
|
Still I can't follow the decision for your new timeout. My suggestion was just to increase the timeout for this line Everything else makes
|
I just wanted to give the test some headroom for slower boards. To be finally clear on timouts I will use:
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. |
|
Okay latest version pushed. Should be all fine now. |
|
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- |
Done finally. :D After merging we can close the Issue #9324 right? |
miri64
left a comment
There was a problem hiding this comment.
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
|
Aand done. Thanks for your patience :-) |
|
Youre Welcome. And thanks for reviewing bloated PRs. :) |
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:
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