[Tarantool-patches] [PATCH 2/2] test: unit/popen -- provide a child process

[Nikita Pettik]

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 at 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).