Re: [PATCH v3 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Feb 13, 2021, 09:30 UTC
- Message-ID
- <20210213093052.GJ1015009@szeder.dev>
- In-Reply-To
- <1e5c856ade8557d7514d9bee1c58bf978aba062c.1613174954.git.gitgitgadget@gmail.com>
On Sat, Feb 13, 2021 at 12:09:13AM +0000, Jeff Hostetler via GitGitGadget wrote:
Show 22 quoted lines
> 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> > ---
Show 5 quoted lines
> 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.
Show 55 quoted lines
> + * 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.
[...]
Show 6 quoted lines
> 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 @@
[...]
Show 13 quoted lines
> +# 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