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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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