On 4/1/2021 11:41 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@xxxxxxxxxxxxx>
It might be nice to summarize the testing strategy here. Are these just
the basics? Is this a full list of every conceivable client/server
interaction? Do some platforms need special tests?
> +# Ask the fsmonitor daemon to insert a little delay before responding to
> +# client commands like `git status` and `git fsmonitor--daemon --query` to
> +# allow recent filesystem events to be received by the daemon. This helps
> +# the CI/PR builds be more stable.
> +#
> +# An arbitrary millisecond value.
> +#
> +GIT_TEST_FSMONITOR_CLIENT_DELAY=1000
> +export GIT_TEST_FSMONITOR_CLIENT_DELAY
As I mentioned before, this seems like it is hiding a bug, especially
because of a full second delay. But even a 1 millisecond delay seems
like it is incorrect to assume this feature works correctly if the
test requires this delay.
If there is a specific interaction that has issues, then it might be
valid to insert this delay in a specific test or two.
> +git version --build-options | grep "feature:" | grep "fsmonitor--daemon" || {
> + skip_all="The built-in FSMonitor is not supported on this platform"
> + test_done
> +}
I see some precedent of this pattern, but it might be nice to instead
register a prereq and then test for the prereq here in the test script.
> +kill_repo () {
Perhaps "kill_repo_daemon" might be more specific?
> + r=$1
> + git -C $r fsmonitor--daemon --stop >/dev/null 2>/dev/null
> + rm -rf $1
> + return 0
> +}
> +
> +start_daemon () {
> + case "$#" in
> + 1) r="-C $1";;
> + *) r="";
> + esac
> +
> + git $r fsmonitor--daemon --start || return $?
> + git $r fsmonitor--daemon --is-running || return $?
Perhaps add 'test_when_finished kill_repo "$r"' as a line here so
consumers don't need to do it themselves.
> + return 0
> +}
> +
> +test_expect_success 'explicit daemon start and stop' '
> + test_when_finished "kill_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 --is-running
> +'
This is an example of a test that could have been created as early as
patch 09/23.
> +test_expect_success 'implicit daemon start' '
> + test_when_finished "kill_repo test_implicit" &&
> +
> + git init test_implicit &&
> + test_must_fail git -C test_implicit fsmonitor--daemon --is-running &&
> +
> + # 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" \
> + git -C test_implicit fsmonitor--daemon --query 0 >actual &&
> + nul_to_q <actual >actual.filtered &&
> + grep "builtin:" actual.filtered &&
> +
> + # confirm that a daemon was started in the background.
> + #
> + # since the mechanism for starting the background daemon is platform
> + # dependent, just confirm that the foreground command received a
> + # response from the daemon.
> +
> + grep :\"query/response-length\" .git/trace &&
> +
> + git -C test_implicit fsmonitor--daemon --is-running &&
> + git -C test_implicit fsmonitor--daemon --stop &&
> + test_must_fail git -C test_implicit fsmonitor--daemon --is-running
> +'
> +
> +test_expect_success 'implicit daemon stop (delete .git)' '
> + test_when_finished "kill_repo test_implicit_1" &&
> +
> + git init test_implicit_1 &&
> +
> + start_daemon test_implicit_1 &&
> +
> + # deleting the .git directory will implicitly stop the daemon.
> + rm -rf test_implicit_1/.git &&
> +
> + # Create an empty .git directory so that the following Git command
> + # will stay relative to the `-C` directory. Without this, the Git
> + # command will (override the requested -C argument) and crawl out
Why the parentheses here?
> + # to the containing Git source tree. This would make the test
> + # result dependent upon whether we were using fsmonitor on our
> + # development worktree.
> +
> + sleep 1 &&
I can understand this sleep, as we are waiting for a background process
to end in response to a directory being deleted.
I'm surprised this works on Windows! I recall having issues deleting
repos that are being watched by Watchman.
> + mkdir test_implicit_1/.git &&
> +
> + test_must_fail git -C test_implicit_1 fsmonitor--daemon --is-running
> +'
> +
> +test_expect_success 'implicit daemon stop (rename .git)' '
> + test_when_finished "kill_repo test_implicit_2" &&
> +
> + git init test_implicit_2 &&
> +
> + start_daemon test_implicit_2 &&
> +
> + # renaming the .git directory will implicitly stop the daemon.
> + mv test_implicit_2/.git test_implicit_2/.xxx &&
> +
> + # Create an empty .git directory so that the following Git command
> + # will stay relative to the `-C` directory. Without this, the Git
> + # command will (override the requested -C argument) and crawl out
> + # to the containing Git source tree. This would make the test
> + # result dependent upon whether we were using fsmonitor on our
> + # development worktree.
> +
> + sleep 1 &&
> + mkdir test_implicit_2/.git &&
> +
> + test_must_fail git -C test_implicit_2 fsmonitor--daemon --is-running
> +'
> +
> +test_expect_success 'cannot start multiple daemons' '
> + test_when_finished "kill_repo test_multiple" &&
> +
> + git init test_multiple &&
> +
> + start_daemon test_multiple &&
> +
> + test_must_fail git -C test_multiple fsmonitor--daemon --start 2>actual &&
> + grep "fsmonitor--daemon is already running" actual &&
> +
> + git -C test_multiple fsmonitor--daemon --stop &&
> + test_must_fail git -C test_multiple fsmonitor--daemon --is-running
> +'
The tests above seem like they could be inserted as soon as the
platform-specific listeners are created. None of this requires the
linked-list of batched updates or cookie file checks.
> +test_expect_success 'setup' '
> + >tracked &&
> + >modified &&
> + >delete &&
> + >rename &&
> + mkdir dir1 &&
> + >dir1/tracked &&
> + >dir1/modified &&
> + >dir1/delete &&
> + >dir1/rename &&
> + mkdir dir2 &&
> + >dir2/tracked &&
> + >dir2/modified &&
> + >dir2/delete &&
> + >dir2/rename &&
> + mkdir dirtorename &&
> + >dirtorename/a &&
> + >dirtorename/b &&
> +
> + cat >.gitignore <<-\EOF &&
> + .gitignore
> + expect*
> + actual*
> + EOF
> +
> + git -c core.useBuiltinFSMonitor= add . &&
> + test_tick &&
> + git -c core.useBuiltinFSMonitor= commit -m initial &&
> +
> + git config core.useBuiltinFSMonitor true
> +'
Now we are getting into the meat of the interactions with Git
features. I can understand these not being ready until all of
the previous product patches are in place.
> +test_expect_success 'update-index implicitly starts daemon' '
> + test_must_fail git fsmonitor--daemon --is-running &&
> +
> + GIT_TRACE2_EVENT="$PWD/.git/trace_implicit_1" \
> + git update-index --fsmonitor &&
> +
> + git fsmonitor--daemon --is-running &&
> + test_might_fail git fsmonitor--daemon --stop &&
Should this be a "test_when_finished kill_repo ." at the
beginning of the test?
> +
> + grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_1
> +'
> +
> +test_expect_success 'status implicitly starts daemon' '
> + test_must_fail git fsmonitor--daemon --is-running &&
> +
> + GIT_TRACE2_EVENT="$PWD/.git/trace_implicit_2" \
> + git status >actual &&
> +
> + git fsmonitor--daemon --is-running &&
> + test_might_fail git fsmonitor--daemon --stop &&
> +
> + grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_2
> +'
> +
> +edit_files() {
> + echo 1 >modified
> + echo 2 >dir1/modified
> + echo 3 >dir2/modified
> + >dir1/untracked
> +}
> +
> +delete_files() {
> + rm -f delete
> + rm -f dir1/delete
> + rm -f dir2/delete
> +}
> +
> +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
> +}
> +
> +file_to_directory() {
> + rm -f delete
> + mkdir delete
> + echo 1 >delete/new
> +}
> +
> +directory_to_file() {
> + rm -rf dir1
> + echo 1 >dir1
> +}
> +
> +verify_status() {
> + git status >actual &&
> + GIT_INDEX_FILE=.git/fresh-index git read-tree master &&
> + GIT_INDEX_FILE=.git/fresh-index git -c core.useBuiltinFSMonitor= status >expect &&
> + test_cmp expect actual &&
> + echo HELLO AFTER &&
> + cat .git/trace &&
> + echo HELLO AFTER
> +}
> +
> +# The next few test cases confirm that our fsmonitor daemon sees each type
> +# of OS filesystem notification that we care about. At this layer we just
> +# ensure we are getting the OS notifications and do not try to confirm what
> +# is reported by `git status`.
> +#
> +# We run a simple query after modifying the filesystem just to introduce
> +# a bit of a delay so that the trace logging from the daemon has time to
> +# get flushed to disk.
> +#
> +# We `reset` and `clean` at the bottom of each test (and before stopping the
> +# daemon) because these commands might implicitly restart the daemon.
> +
> +clean_up_repo_and_stop_daemon () {
> + git reset --hard HEAD
> + git clean -fd
> + git fsmonitor--daemon --stop
> + rm -f .git/trace
> +}
> +
> +test_expect_success 'edit some files' '
> + test_when_finished "clean_up_repo_and_stop_daemon" &&
Do you need the quotes here?
> +
> + (
> + GIT_TRACE_FSMONITOR="$PWD/.git/trace" &&
Use "$(pwd)/.git/trace". There are some strange things with $PWD
especially on Windows.
> + export GIT_TRACE_FSMONITOR &&
> +
> + start_daemon
> + ) &&
> +
> + edit_files &&
> +
> + git fsmonitor--daemon --query 0 >/dev/null 2>&1 &&
> +
> + grep "^event: dir1/modified$" .git/trace &&
> + grep "^event: dir2/modified$" .git/trace &&
> + grep "^event: modified$" .git/trace &&
> + grep "^event: dir1/untracked$" .git/trace
> +'
> +
> +test_expect_success 'create some files' '
> + test_when_finished "clean_up_repo_and_stop_daemon" &&
> +
> + (
> + GIT_TRACE_FSMONITOR="$PWD/.git/trace" &&
> + export GIT_TRACE_FSMONITOR &&
> +
> + start_daemon
> + ) &&
> +
> + create_files &&
> +
> + git fsmonitor--daemon --query 0 >/dev/null 2>&1 &&
> +
> + grep "^event: dir1/new$" .git/trace &&
> + grep "^event: dir2/new$" .git/trace &&
> + grep "^event: new$" .git/trace
> +'
I wonder if we can scan the trace for the number of events
and ensure we have the right count, to ensure we aren't getting
_extra_ events that we don't want?
The rest of the tests seem similarly structured and testing
important cases. I'll delay thinking of new tests until I see
the rest of the tests you are adding.
Thanks,
-Stolee
