From: Jeff Hostetler <git@jeffhostetler.com>
Date: Tue, 16 Feb 2021 15:53:48 GMT
Subject: Re: [PATCH v3 12/12] t0052: add simple-ipc tests and t/helper/test-simple-ipc tool
Message-ID: <01e7805a-a245-7640-42d4-5ecc01195598@jeffhostetler.com>
In-Reply-To: <20210213093052.GJ1015009@szeder.dev>



On 2/13/21 4:30 AM, SZEDER Gábor wrote:

[...]

> [...]
> 
>> +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.

Thanks for the suggestions.  Let me take another pass at
it.  I think making the "send quit" command try to wait until
the server shutdown would make it easier for all concerned.

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