Re: [PATCH v6 21/30] t7527: create test for fsmonitor--daemon

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/t7527-builtin-fsmonitor.sh | 511 +++++++++++++++++++++++++++++++++++
>  1 file changed, 511 insertions(+)
>  create mode 100755 t/t7527-builtin-fsmonitor.sh
>
> diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
> new file mode 100755
> index 00000000000..5f7b8e54233
> --- /dev/null
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -0,0 +1,511 @@
> +#!/bin/sh
> +
> +test_description='built-in file system watcher'
> +
> +. ./test-lib.sh
> +
> +if ! test_have_prereq FSMONITOR_DAEMON
> +then
> +	skip_all="fsmonitor--daemon is not supported on this platform"
> +	test_done
> +fi
> +
> +stop_daemon_delete_repo () {
> +	r=$1
> +	git -C $r fsmonitor--daemon stop >/dev/null 2>/dev/null

Do we really need to quiet all its output? Why not just have the
--verbose option do its thing?

> +	rm -rf $1
> +	return 0

Missing &&-chaining
> +}
> +
> +start_daemon () {
> +	case "$#" in
> +		1) r="-C $1";;
> +		*) r="";

Our usual style is not to indent these.

But maybe just use the same pattern as test_commit et al use? It's a bit
more verbose, but IMO clearer.

> +	esac
> +
> +	git $r fsmonitor--daemon start || return $?
> +	git $r fsmonitor--daemon status || return $?

Maybe don't do all this "return" and just &&-chain these instead (including the case/esac)?

> +
> +	return 0
> +}
> +
> +# Is a Trace2 data event present with the given catetory and key?
> +# We do not care what the value is.
> +#
> +have_t2_data_event () {
> +	c=$1
> +	k=$2
> +
> +	grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"'
> +}

Optional & aside: But it would be really nice to just have this amend
the test_region function into something more general, so this could be:

    test_trace2 --event data --category "$c" --key "$k"

And have "test_region" then be a thin wrapper for that, and do the
appropriate "maybe only the start one, not the end one?" logic there.

> +test_expect_success 'explicit daemon start and stop' '
> +	test_when_finished "stop_daemon_delete_repo test_explicit" &&
> +
> +	git init test_explicit &&
> +	start_daemon test_explicit &&
> +
> +	git -C test_explicit fsmonitor--daemon stop &&
> +	test_must_fail git -C test_explicit fsmonitor--daemon status
> +'
> +
> +test_expect_success 'implicit daemon start' '
> +	test_when_finished "stop_daemon_delete_repo test_implicit" &&
> +
> +	git init test_implicit &&
> +	test_must_fail git -C test_implicit fsmonitor--daemon status &&
> +
> +	# query will implicitly start the daemon.
> +	#
> +	# for test-script simplicity, we send a V1 timestamp rather than
> +	# a V2 token.  either way, the daemon response to any query contains
> +	# a new V2 token.  (the daemon may complain that we sent a V1 request,
> +	# but this test case is only concerned with whether the daemon was
> +	# implicitly started.)
> +
> +	GIT_TRACE2_EVENT="$(pwd)/.git/trace" \

Better to use $PWD than $(pwd)

> +delete_files () {
> +	rm -f delete
> +	rm -f dir1/delete
> +	rm -f dir2/delete

More missing &&-chaining.

> +}
> +
> +create_files () {
> +	echo 1 >new
> +	echo 2 >dir1/new
> +	echo 3 >dir2/new
> +}
> +
> +rename_files () {
> +	mv rename renamed
> +	mv dir1/rename dir1/renamed
> +	mv dir2/rename dir2/renamed

ditto.


> +}
> +
> +file_to_directory () {
> +	rm -f delete
> +	mkdir delete
> +	echo 1 >delete/new

ditto.


> +}
> +
> +directory_to_file () {
> +	rm -rf dir1
> +	echo 1 >dir1

ditto.
> +}
> +
> +verify_status () {

This is used by nothing? Maybe it'll be used later, but that commit
could/should add it then?

Hrm, nope, just read ahead and nothing uses it at all.

> +clean_up_repo_and_stop_daemon () {
> +	git reset --hard HEAD
> +	git clean -fd
> +	git fsmonitor--daemon stop
> +	rm -f .git/trace

Missing &&-chaining (will stop noting these now, please look through the rest...)

> +}
> +
> +test_expect_success 'edit some files' '
> +	test_when_finished clean_up_repo_and_stop_daemon &&
> +
> +	(
> +		GIT_TRACE_FSMONITOR="$(pwd)/.git/trace" &&
> +		export GIT_TRACE_FSMONITOR &&
> +
> +		start_daemon

Maybe have this "start_daemon" take an optional --trace argument or
something, allowing us to skip all these subsequent subshells.

> +	) &&
> +
> +	edit_files &&
> +
> +	test-tool fsmonitor-client query --token 0 >/dev/null 2>&1 &&

For these & the rest: just skip the quieting of the output? I.e. let the
test's --verbose do its job?


> +test_expect_success 'flush cached data' '
> +	test_when_finished "stop_daemon_delete_repo test_flush" &&
> +
> +	git init test_flush &&
> +
> +	(
> +		GIT_TEST_FSMONITOR_TOKEN=true &&
> +		export GIT_TEST_FSMONITOR_TOKEN &&
> +
> +		GIT_TRACE_FSMONITOR="$(pwd)/.git/trace_daemon" &&
> +		export GIT_TRACE_FSMONITOR &&
> +
> +		start_daemon test_flush
> +	) &&
> +
> +	# The daemon should have an initial token with no events in _0 and
> +	# then a few (probably platform-specific number of) events in _1.
> +	# These should both have the same <token_id>.
> +
> +	test-tool -C test_flush fsmonitor-client query --token "builtin:test_00000001:0" >actual_0 &&
> +	nul_to_q <actual_0 >actual_q0 &&
> +
> +	touch test_flush/file_1 &&
> +	touch test_flush/file_2 &&

I may be missinga subtlety here, but is "touch" needed v.s. ">", i.e. we
just created "test_flush", so if we create a new file it'll have the
current timestamp.

Or did it get created by the helpers?