Re: [PATCH v3 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Sat, Feb 13, 2021 at 12:09:13AM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Create t0052-simple-ipc.sh with unit tests for the "simple-ipc" mechanism.
> 
> Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc"
> functions.
> 
> When the tool is invoked with "run-daemon", it runs a server to listen
> for "simple-ipc" connections on a test socket or named pipe and
> responds to a set of commands to exercise/stress the communication
> setup.
> 
> When the tool is invoked with "start-daemon", it spawns a "run-daemon"
> command in the background and waits for the server to become ready
> before exiting.  (This helps make unit tests in t0052 more predictable
> and avoids the need for arbitrary sleeps in the test script.)
> 
> The tool also has a series of client "send" commands to send commands
> and data to a server instance.
> 
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---

> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
> new file mode 100644
> index 000000000000..92aa7f843cfa
> --- /dev/null
> +++ b/t/helper/test-simple-ipc.c

[...]

> +/*
> + * This is "application callback" that sits on top of the "ipc-server".
> + * It completely defines the set of command verbs supported by this

Please avoid the noiseword "verbs" and just call them commands; a few
of these commands are not even verbs.

> + * application.
> + */
> +static int test_app_cb(void *application_data,
> +		       const char *command,
> +		       ipc_server_reply_cb *reply_cb,
> +		       struct ipc_server_reply_data *reply_data)
> +{
> +	/*
> +	 * Verify that we received the application-data that we passed
> +	 * when we started the ipc-server.  (We have several layers of
> +	 * callbacks calling callbacks and it's easy to get things mixed
> +	 * up (especially when some are "void*").)
> +	 */
> +	if (application_data != (void*)&my_app_data)
> +		BUG("application_cb: application_data pointer wrong");
> +
> +	if (!strcmp(command, "quit")) {
> +		/*
> +		 * The client sent a "quit" command.  This is an async
> +		 * request for the server to shutdown.
> +		 *
> +		 * We DO NOT send the client a response message
> +		 * (because we have nothing to say and the other
> +		 * server threads have not yet stopped).
> +		 *
> +		 * Tell the ipc-server layer to start shutting down.
> +		 * This includes: stop listening for new connections
> +		 * on the socket/pipe and telling all worker threads
> +		 * to finish/drain their outgoing responses to other
> +		 * clients.
> +		 *
> +		 * This DOES NOT force an immediate sync shutdown.
> +		 */
> +		return SIMPLE_IPC_QUIT;
> +	}
> +
> +	if (!strcmp(command, "ping")) {
> +		const char *answer = "pong";
> +		return reply_cb(reply_data, answer, strlen(answer));
> +	}
> +
> +	if (!strcmp(command, "big"))
> +		return app__big_command(reply_cb, reply_data);
> +
> +	if (!strcmp(command, "chunk"))
> +		return app__chunk_command(reply_cb, reply_data);
> +
> +	if (!strcmp(command, "slow"))
> +		return app__slow_command(reply_cb, reply_data);
> +
> +	if (starts_with(command, "sendbytes "))
> +		return app__sendbytes_command(command, reply_cb, reply_data);
> +
> +	return app__unhandled_command(command, reply_cb, reply_data);
> +}

[...]

> +int cmd__simple_ipc(int argc, const char **argv)
> +{
> +	const char *path = "ipc-test";

Since the path of the socket used in the tests is hardcoded, we could
use it in the tests as well to check its presence/absence.

[...]

> diff --git a/t/t0052-simple-ipc.sh b/t/t0052-simple-ipc.sh
> new file mode 100755
> index 000000000000..e36b786709ec
> --- /dev/null
> +++ b/t/t0052-simple-ipc.sh
> @@ -0,0 +1,134 @@

[...]

> +# Sending a "quit" message to the server causes it to start an "async
> +# shutdown" -- queuing shutdown events to all socket/pipe thread-pool
> +# threads.  Each thread will process that event after finishing
> +# (draining) any in-progress IO with other clients.  So when the "send
> +# quit" client command exits, the ipc-server may still be running (but
> +# it should be cleaning up).
> +#
> +# So, insert a generous sleep here to give the server time to shutdown.
> +#
> +test_expect_success '`quit` works' '
> +	test-tool simple-ipc send quit &&
> +
> +	sleep 5 &&

The server process is responsible for removing the socket, so instead
of a hard-coded 5 seconds delay the test could (semi-)busy wait in a
loop until the socket disappears like this:

diff --git a/t/t0052-simple-ipc.sh b/t/t0052-simple-ipc.sh
index 6958835454..609d8d4283 100755
--- a/t/t0052-simple-ipc.sh
+++ b/t/t0052-simple-ipc.sh
@@ -122,6 +122,13 @@ test_expect_success 'stress test threads' '
 
 test_expect_success '`quit` works' '
 	test-tool simple-ipc send quit &&
+	nr_tries_left=10 &&
+	while test -S ipc-test &&
+	      test $nr_tries_left -gt 0
+	do
+		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
+	done &&
 	test_must_fail test-tool simple-ipc is-active &&
 	test_must_fail test-tool simple-ipc send ping
 '

This way we might get away without any delay or with only a single
one-second sleep in most cases, while we could bump the timeout a bit
higher for the sake of a CI system in a particularly bad mood.

Would this work on Windows, or at least could it be tweaked to work
there?

I think this is conceptually the same as what you did at startup,
except in this example the test script waits instead of the test-tool
subcommand.  Perhaps it would be worth incorporating this wait into
the test-tool as well; or perhaps it would be simpler to do the
waiting in the test script at startup as well.

> +	test_must_fail test-tool simple-ipc is-active &&
> +	test_must_fail test-tool simple-ipc send ping
> +'
> +
> +test_done
> -- 
> gitgitgadget