From: Nikita Pettik <korablev@tarantool.org> To: Alexander Turenko <alexander.turenko@tarantool.org> Cc: tml <tarantool-patches@dev.tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process Date: Thu, 26 Mar 2020 00:59:26 +0000 [thread overview] Message-ID: <20200326005926.GA3155@tarantool.org> (raw) In-Reply-To: <20200326004737.nu4dcii7zpwesrsp@tkn_work_nb> On 26 Mar 03:47, Alexander Turenko wrote: > 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? Yep, I've already verified - patch really fixes the problem. > 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 <gorcunov@gmail.com> > > --- > > 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).
next prev parent reply other threads:[~2020-03-26 0:59 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-24 10:03 [Tarantool-patches] [PATCH 0/2] popen: fix unit test Cyrill Gorcunov 2020-03-24 10:03 ` [Tarantool-patches] [PATCH 1/2] popen: do not require space for shell args Cyrill Gorcunov 2020-03-26 0:23 ` Alexander Turenko 2020-03-24 10:03 ` [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process Cyrill Gorcunov 2020-03-26 0:47 ` Alexander Turenko 2020-03-26 0:59 ` Nikita Pettik [this message] 2020-03-26 8:31 ` [Tarantool-patches] [PATCH v2 " Cyrill Gorcunov 2020-03-26 9:04 ` Cyrill Gorcunov 2020-03-24 10:04 ` [Tarantool-patches] [PATCH 0/2] popen: fix unit test Cyrill Gorcunov 2020-03-26 0:52 ` Alexander Turenko 2020-03-26 11:57 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200326005926.GA3155@tarantool.org \ --to=korablev@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox