From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 905CF441841 for ; Thu, 26 Mar 2020 03:47:35 +0300 (MSK) Date: Thu, 26 Mar 2020 03:47:37 +0300 From: Alexander Turenko Message-ID: <20200326004737.nu4dcii7zpwesrsp@tkn_work_nb> References: <20200324100347.15405-1-gorcunov@gmail.com> <20200324100347.15405-3-gorcunov@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200324100347.15405-3-gorcunov@gmail.com> Subject: Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cyrill Gorcunov Cc: tml I think it is good thing to depend less on a system environment. It is not clear why the problem appears in the first place and why it is gone after the patch. This confuses me a bit. Anyway, I'll CC Nikita to do necessary amount runs with and withour the patch to say for sure whether it actually helps versus flaky test fails. Nikita, can you please verify it? Other than that the patch looks okay except several minor points, see below. Whether you'll fix those point or not, I think the patch does not require re-review with me: Nikita's check should be enough. LGTM. WBR, Alexander Turenko. On Tue, Mar 24, 2020 at 01:03:47PM +0300, Cyrill Gorcunov wrote: > Testing via plain C interface with a shell is not stable, > the shell might simply be misconfigured or not found and > we will simply stuck forever (the signal handling in libev > is tricky and requires at least idle cycles or similar to > pass event processing). Thus lets rather run a program we > know is presenting in the system (popen-child executable). > > Fixes #4811 > > Signed-off-by: Cyrill Gorcunov > --- > test/unit/CMakeLists.txt | 4 +++ > test/unit/popen-child.c | 71 ++++++++++++++++++++++++++++++++++++++++ > test/unit/popen.c | 18 ++++++---- > 3 files changed, 86 insertions(+), 7 deletions(-) > create mode 100644 test/unit/popen-child.c > > diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt > index bc6aebdcb..e1d506f58 100644 > --- a/test/unit/CMakeLists.txt > +++ b/test/unit/CMakeLists.txt > @@ -246,5 +246,9 @@ target_link_libraries(swim_errinj.test unit swim) > add_executable(merger.test merger.test.c) > target_link_libraries(merger.test unit core box) > > +# > +# Client for popen.test > +add_executable(popen-child popen-child.c) > + Let's add the generated executable to .gitignore (for in-source build). > + // read -n X and the just echo data read Nit: Our style guide follows kernel's one and forbids C99 style comments. > + if (!strcmp(argv[1], "read") && > + !strcmp(argv[2], "-n")) { > + ssize_t nr = (ssize_t)atoi(argv[3]); > + if (nr <= 0) { > + fprintf(stderr, "Wrong number of args\n"); > + exit(1); > + } > + > + if (nr >= (ssize_t)sizeof(buf)) { > + fprintf(stderr, "Too many bytes to read\n"); > + exit(1); > + } > + > + ssize_t n = read(STDIN_FILENO, buf, nr); > + if (n != nr) { > + fprintf(stderr, "Can't read from stdin\n"); > + exit(1); > + } I understand that `nr` is small here and it is hard to find a condition when a singletone read will be partial, but since the patch is about stability of testing (and we maybe will add more cases: maybe for even border conditions like pipe buffer overrun) I would handle this situation just in case and read in a loop. The same for write below. > + > + n = write(STDOUT_FILENO, buf, nr); > + if (n != nr) { > + fprintf(stderr, "Can't write to stdout\n"); > + exit(1); > + } > @@ -237,6 +238,9 @@ main(int argc, char *argv[]) > //say_logger_init(NULL, S_DEBUG, 0, "plain", 0); > memory_init(); > > + snprintf(popen_child_path, sizeof(popen_child_path), > + "%s/test/unit/popen-child", getenv("BUILDDIR")); > + It works for in-source and out-of-source build when is run under test-run. It is okay in fact. However I think it is convenient to have a default that, say, allows to run ./test/unit/popen.test from a build directory w/o manual setting of BUILDDIR. It seems logical to set BUILDDIR to '.' by default (if getenv() returns NULL).