From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: szeder.dev@gmail.com, ukshah2@illinois.edu,
Kevin.Willford@microsoft.com,
Derrick Stolee <dstolee@microsoft.com>,
Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/8] Improve testability with GIT_TEST_FSMONITOR
Date: Mon, 09 Dec 2019 16:09:56 +0000 [thread overview]
Message-ID: <pull.466.v2.git.1575907804.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.466.git.1574374826.gitgitgadget@gmail.com>
The GIT_TEST_FSMONITOR environment variable allows run-time specification of
the fsmonitor hook. Initially used by t7619-status-fsmonitor.sh, we can
enable it across the test suite to see how it affects Git's behavior. In
particular, we can specify the version of the hook that requests a result
from Watchman to get actual updates to the files in the repo.
In many cases, our tests are simply not ready to handle this option.
fsmonitor does not integrate well with features such as split index, bare
repos, or submodules. Other times, we need to disable it because the test is
being specific about what files Git inspects during a 'status' call.
The long-term vision is to be able to run CI builds using a file-system
watcher like Watchman to get better coverage on this feature. These patches
get us closer, but there are still some issues around overloading the
Watchman interface when the tests are run in parallel. When using "prove -j8
t[0-8]*.sh" I see some failures that do not reproduce when running the test
scripts in isolation (on Linux), but using "-j5" is enough to eliminate
failures.
Some of these changes should be backed out in favor of a deeper fix, so in
some sense these settings of GIT_TEST_FSMONITOR="" serve as TODOs for the
fsmonitor feature.
Some recent and upcoming work on fsmonitor by Utsav Shah and Kevin Willford
may help us reach the goal of running with watchman enabled in CI builds.
I'll come back around with updates to the .azure-pipelines YAML files when
we feel the feature is ready for that.
Updates in V2:
* Commit messages have been filled out completely. Most provide the same
blurb of context before briefly describing the actual change.
* Some commits were merged because they had similar causes (worktrees,
submodules).
* The test_clear_watchman function is updated and it is called by test_done
instead of the atexit helper.
Thanks, -Stolee
Derrick Stolee (8):
fsmonitor: disable in a bare repo
fsmonitor: do not output to stderr for tests
t1301-shared-repo.sh: disable FSMONITOR
t3030-merge-recursive.sh: disable fsmonitor when tweaking
GIT_WORK_TREE
tests: disable fsmonitor in submodule tests
t7063: disable fsmonitor with status cache
t7519: disable external GIT_TEST_FSMONITOR variable
test-lib: clear watchman watches at test completion
config.c | 5 +++++
t/t1301-shared-repo.sh | 1 +
t/t1510-repo-setup.sh | 1 +
t/t2400-worktree-add.sh | 2 ++
t/t3030-merge-recursive.sh | 2 ++
t/t3404-rebase-interactive.sh | 1 +
t/t3600-rm.sh | 1 +
t/t4060-diff-submodule-option-diff-format.sh | 3 +++
t/t5526-fetch-submodules.sh | 2 ++
t/t7063-status-untracked-cache.sh | 3 +++
t/t7402-submodule-rebase.sh | 3 +++
t/t7406-submodule-update.sh | 2 ++
t/t7506-status-submodule.sh | 3 +++
t/t7508-status.sh | 3 +++
t/t7519-status-fsmonitor.sh | 3 +++
t/t7519/fsmonitor-watchman | 1 -
t/test-lib-functions.sh | 15 +++++++++++++++
t/test-lib.sh | 4 ++++
18 files changed, 54 insertions(+), 1 deletion(-)
base-commit: dd0b61f577f041f1119bb3288451f8f9b7f9e3f2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-466%2Fderrickstolee%2Ftest-watchman-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-466/derrickstolee/test-watchman-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/466
Range-diff vs v1:
1: 9bf5a803e6 ! 1: 79bb4c8e7d fsmonitor: disable in a bare repo
@@ -2,7 +2,18 @@
fsmonitor: disable in a bare repo
- t0003-attributes.sh
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ If the repository is bare, then there is no working directory to
+ watch. Disable the core_fsmonitor global in this case.
+
+ Before this change, the test t0003-attributes.sh would fail with
+ GIT_TEST_FSMONITOR pointing to t/t7519/fsmonitor-watchman.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
2: 245cc1b2cc ! 2: dd492091e3 fsmonitor: do not output to stderr for tests
@@ -2,7 +2,18 @@
fsmonitor: do not output to stderr for tests
- t0003-attributes.sh
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ The test t0003-attributes.sh and others would fail when
+ GIT_TEST_FSMONITOR is pointing at t/t7519/fsmonitor-watchman because
+ it sends a message over stderr when registering the repo with
+ watchman for the first time. Remove this stderr message for the
+ test script to avoid this noise.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
3: ed40ce8e4c ! 3: f9db0c3416 t1301-shared-repo.sh: disable FSMONITOR
@@ -2,6 +2,17 @@
t1301-shared-repo.sh: disable FSMONITOR
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR
+ is set to t/t7519/fsmonitor-watchman because it changes permissions
+ in an incompatible way.
+
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
4: 9179776982 < -: ---------- t1510-repo-setup.sh: disable fsmonitor if no .git dir
5: 81c587651e < -: ---------- fsmonitor: disable fsmonitor with worktrees
6: d5f5a3e2b9 ! 4: efc16962ee t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE
@@ -2,8 +2,47 @@
t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ Worktrees use a ".git" _file_ instead of a folder to point to
+ the base repo's .git directory and the proper worktree HEAD. The
+ fsmonitor hook tries to create a JSON file inside the ".git" folder
+ which violates the expectation here. It would be better to properly
+ find a safe folder for storing this JSON file.
+
+ This is also a problem when a test script uses GIT_WORK_TREE.
+
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
+ diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
+ --- a/t/t1510-repo-setup.sh
+ +++ b/t/t1510-repo-setup.sh
+@@
+ setup_repo 29 non-existent gitfile true &&
+ mkdir -p 29/sub/sub 29/wt/sub &&
+ (
++ GIT_TEST_FSMONITOR="" &&
+ cd 29 &&
+ GIT_WORK_TREE="$here/29" &&
+ export GIT_WORK_TREE &&
+
+ diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
+ --- a/t/t2400-worktree-add.sh
+ +++ b/t/t2400-worktree-add.sh
+@@
+ #!/bin/sh
+
++GIT_TEST_FSMONITOR=""
++
+ test_description='test git worktree add'
+
+ . ./test-lib.sh
+
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
7: d0b212c9df < -: ---------- t3600-rm.sh: disable fsmonitor when deleting populated submodule
8: 36f845cb7e ! 5: a5b0bf6ac7 tests: disable fsmonitor in submodule tests
@@ -2,12 +2,43 @@
tests: disable fsmonitor in submodule tests
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
The fsmonitor feature struggles with submodules. Disable the
GIT_TEST_FSMONITOR environment variable before running tests with
a lot of submodule interactions.
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
+ diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
+ --- a/t/t3404-rebase-interactive.sh
+ +++ b/t/t3404-rebase-interactive.sh
+@@
+ '
+
+ test_expect_success 'submodule rebase setup' '
++ GIT_TEST_FSMONITOR="" &&
+ git checkout A &&
+ mkdir sub &&
+ (
+
+ diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
+ --- a/t/t3600-rm.sh
+ +++ b/t/t3600-rm.sh
+@@
+ '
+
+ test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
++ GIT_TEST_FSMONITOR="" &&
+ git reset --hard &&
+ git submodule update &&
+ git -C submod checkout HEAD^ &&
+
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
9: 141a1909a4 ! 6: 9cd4a08d82 t7063: disable fsmonitor with status cache
@@ -2,6 +2,13 @@
t7063: disable fsmonitor with status cache
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
The status cache tests use GIT_TRACE_UNTRACKED_STATS to check very
detailed statistics related to how much Git actually checked for
untracked files. The fsmonitor feature changes the expected behavior
10: cd717ef5de = 7: 215ec8688e t7519: disable external GIT_TEST_FSMONITOR variable
11: 47cecb4a83 ! 8: e51165f260 test-lib: clear watchman watches at test completion
@@ -2,6 +2,29 @@
test-lib: clear watchman watches at test completion
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ When running the test suite in parallel with 'prove -j <N>', many
+ repos are created and deleted in parallel. When GIT_TEST_FSMONITOR
+ points to t/t7519/fsmonitor-watchman, this can lead to watchman
+ tracking many different folders, overloading its watch queue.
+
+ As a test script completes, we can tell watchman to stop watching
+ the directories inside the TRASH_DIRECTORY.
+
+ This is particularly important on Windows where watchman keeps an
+ open handle on the directories it watches, preventing them from
+ being deleted. There is currently a bug in watchman [1] where this
+ handle still is not closed, but if that is updated then these tests
+ can be run on Windows as well.
+
+ [1] https://github.com/facebook/watchman/issues/764
+
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
@@ -13,17 +36,17 @@
}
+
+test_clear_watchman () {
-+ if test $GIT_TEST_FSMONITOR -ne ""
++ if test -n "$GIT_TEST_FSMONITOR"
+ then
+ watchman watch-list |
+ grep "$TRASH_DIRECTORY" |
-+ sed "s/\t\"//g" |
-+ sed "s/\",//g" >repo-list
++ sed "s/\",//g" |
++ sed "s/\"//g" >repo-list
+
-+ for repo in $(cat repo-list)
++ while read repo
+ do
+ watchman watch-del "$repo"
-+ done
++ done <repo-list
+ fi
+}
@@ -31,11 +54,13 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- # sure that the registered cleanup commands are run only once.
- test : != "$test_atexit_cleanup" || return 0
+ test_done () {
+ GIT_EXIT_OK=t
++ # If watchman is being used with GIT_TEST_FSMONITOR, then
++ # clear all watches on directories inside the TRASH_DIRECTORY.
+ test_clear_watchman
+
- setup_malloc_check
- test_eval_ "$test_atexit_cleanup"
- test_atexit_cleanup=:
+ # Run the atexit commands _before_ the trash directory is
+ # removed, so the commands can access pidfiles and socket files.
+ test_atexit_handler
--
gitgitgadget
next prev parent reply other threads:[~2019-12-09 16:10 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-21 22:20 [PATCH 00/11] Improve testability with GIT_TEST_FSMONITOR Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 01/11] fsmonitor: disable in a bare repo Derrick Stolee via GitGitGadget
2019-11-21 23:18 ` Denton Liu
2019-11-22 1:57 ` Derrick Stolee
2019-11-21 22:20 ` [PATCH 02/11] fsmonitor: do not output to stderr for tests Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 03/11] t1301-shared-repo.sh: disable FSMONITOR Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 04/11] t1510-repo-setup.sh: disable fsmonitor if no .git dir Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 05/11] fsmonitor: disable fsmonitor with worktrees Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 06/11] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 07/11] t3600-rm.sh: disable fsmonitor when deleting populated submodule Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 08/11] tests: disable fsmonitor in submodule tests Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 09/11] t7063: disable fsmonitor with status cache Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 10/11] t7519: disable external GIT_TEST_FSMONITOR variable Derrick Stolee via GitGitGadget
2019-11-21 22:20 ` [PATCH 11/11] test-lib: clear watchman watches at test completion Derrick Stolee via GitGitGadget
2019-11-22 1:06 ` SZEDER Gábor
2019-12-09 14:12 ` Derrick Stolee
2019-12-09 23:40 ` SZEDER Gábor
2019-12-10 1:43 ` Derrick Stolee
2019-12-09 16:09 ` Derrick Stolee via GitGitGadget [this message]
2019-12-09 16:09 ` [PATCH v2 1/8] fsmonitor: disable in a bare repo Derrick Stolee via GitGitGadget
2019-12-10 9:46 ` SZEDER Gábor
2019-12-09 16:09 ` [PATCH v2 2/8] fsmonitor: do not output to stderr for tests Derrick Stolee via GitGitGadget
2019-12-09 16:09 ` [PATCH v2 3/8] t1301-shared-repo.sh: disable FSMONITOR Derrick Stolee via GitGitGadget
2019-12-10 9:43 ` SZEDER Gábor
2019-12-09 16:10 ` [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE Derrick Stolee via GitGitGadget
2019-12-10 10:07 ` SZEDER Gábor
2019-12-10 13:45 ` Derrick Stolee
2019-12-10 14:57 ` SZEDER Gábor
2019-12-10 15:07 ` SZEDER Gábor
2020-01-23 15:45 ` Derrick Stolee
2019-12-09 16:10 ` [PATCH v2 5/8] tests: disable fsmonitor in submodule tests Derrick Stolee via GitGitGadget
2019-12-10 10:13 ` SZEDER Gábor
2019-12-10 13:57 ` Derrick Stolee
2019-12-09 16:10 ` [PATCH v2 6/8] t7063: disable fsmonitor with status cache Derrick Stolee via GitGitGadget
2019-12-09 16:10 ` [PATCH v2 7/8] t7519: disable external GIT_TEST_FSMONITOR variable Derrick Stolee via GitGitGadget
2019-12-09 16:10 ` [PATCH v2 8/8] test-lib: clear watchman watches at test completion Derrick Stolee via GitGitGadget
2019-12-09 22:52 ` Junio C Hamano
2019-12-10 1:49 ` Derrick Stolee
2019-12-10 5:20 ` Junio C Hamano
2019-12-10 13:51 ` Derrick Stolee
2019-12-10 14:09 ` Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.466.v2.git.1575907804.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=Kevin.Willford@microsoft.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=szeder.dev@gmail.com \
--cc=ukshah2@illinois.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.