Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/31/2017 12:33 AM, Christian Couder wrote:

+test_expect_success 'refresh_index() invalidates fsmonitor cache' '
+   git config core.fsmonitor true &&
+   git config core.untrackedcache true &&
+   clean_repo &&
+   git status &&
+   test_path_is_missing marker &&
+   dirty_repo &&
+   write_script .git/hooks/query-fsmonitor<<-\EOF &&
+   :>marker
+   EOF
+   git add . &&
+   git commit -m "to reset" &&
+   git status &&
+   test_path_is_file marker &&


Ok so "marker" is there now.


+   git reset HEAD~1 &&
+   git status >output &&
+   test_path_is_file marker &&


You already checked that "marker" exists 3 lines above, and as far as
I can see nothing could remove this file since the previous test, as
the hook can only create it.
So I wonder if something is missing or if this test is redundant.


Testing it each time ensures it is being created when it is supposed to be
(ie when the test believes it is using the query-fsmonitor hook) and that it
isn't when it isn't supposed to be (ie when the hook should not be called).


I would agree with that if the "marker" file was removed after the
previous "test_path_is_file marker", but I don't see any "clean_repo"
or "rm marker" call that removes it.



Got it.  I've added a call to "rm -f marker" to ensure the marker isn't 
left over from the previous status command.  I found another instance of 
where this could happen in "status doesn't detect unreported 
modifications" and fixed it as well.  Thanks!

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Tue, May 30, 2017 at 11:21 PM, Ben Peart  wrote:
>
> On 5/30/2017 9:18 AM, Christian Couder wrote:
>>
>> On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:
>>
>>> +   printf "untracked\0"
>>> +   printf "dir1/untracked\0"
>>> +   printf "dir2/untracked\0"
>>> +   printf "modified\0"
>>> +   printf "dir1/modified\0"
>>> +   printf "dir2/modified\0"
>>> +   printf "new\0""
>>> +   printf "dir1/new\0"
>>> +   printf "dir2/new\0"
>>
>> Maybe something like the following to save a few lines and remove some
>> redundancies:
>>
>> printf "%s\0" untracked dir1/untracked dir2/untracked \
>>   modified dir1/modified dir2/modified \
>>   new dir1/new dir2/new
>>
>> or perhaps:
>>
>> for f in untracked modified new
>> do
>>printf "%s\0" "$f" "dir1/$f" "dir2/$f"
>> done
>
> That is a clever solution that certainly is fewer lines of code. However, I
> have to read the loop and think through the logic to figure out what it is
> doing vs the existing implementation where what it is doing is apparent from
> just glancing at the code.  I was also trying to maintain consistency with
> the other status test code in t7508-status.sh

Ok fair enough.

>>> +   EOF
>>> +   : >tracked &&
>>> +   : >modified &&
>>> +   mkdir dir1 &&
>>> +   : >dir1/tracked &&
>>> +   : >dir1/modified &&
>>> +   mkdir dir2 &&
>>> +   : >dir2/tracked &&
>>> +   : >dir2/modified &&
>>> +   git add . &&
>>> +   test_tick &&
>>> +   git commit -m initial &&
>>> +   dirty_repo
>>> +'
>>> +
>>> +cat >.gitignore <<\EOF
>>> +.gitignore
>>> +expect*
>>> +output*
>>> +marker*
>>> +EOF
>>
>> This could be part of the previous setup test.
>
> I had followed the pattern in t7508-status.sh with this but I can move it in
> if that is the preferred model.

Yeah, I think it is preferred these days to have all the setup code
inside tests.

>>> +   git config core.fsmonitor true  &&
>>> +   git config core.untrackedcache true &&
>>> +   git -c core.fsmonitor=false -c core.untrackedcache=true status
>>> >expect &&
>>
>> I don't understand why there is " -c core.untrackedcache=true" in the
>> above command as you already set core.untrackedcache to true on the
>> previous line.
>
> Defensive programming. :) The global setting was to ensure it was set when
> the test sub-functions clean and dirty were called and the command line
> settings were used to make it explicit what was being tested.  I can remove
> them if it is causing confusion.

I think it is a bit confusing indeed.

>>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
>>> +   git config core.fsmonitor true &&
>>> +   git config core.untrackedcache true &&
>>> +   clean_repo &&
>>> +   git status &&
>>> +   test_path_is_missing marker &&
>>> +   dirty_repo &&
>>> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
>>> +   :>marker
>>> +   EOF
>>> +   git add . &&
>>> +   git commit -m "to reset" &&
>>> +   git status &&
>>> +   test_path_is_file marker &&

Ok so "marker" is there now.

>>> +   git reset HEAD~1 &&
>>> +   git status >output &&
>>> +   test_path_is_file marker &&
>>
>> You already checked that "marker" exists 3 lines above, and as far as
>> I can see nothing could remove this file since the previous test, as
>> the hook can only create it.
>> So I wonder if something is missing or if this test is redundant.
>
> Testing it each time ensures it is being created when it is supposed to be
> (ie when the test believes it is using the query-fsmonitor hook) and that it
> isn't when it isn't supposed to be (ie when the hook should not be called).

I would agree with that if the "marker" file was removed after the
previous "test_path_is_file marker", but I don't see any "clean_repo"
or "rm marker" call that removes it.

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 6:37 PM, Junio C Hamano wrote:

Ben Peart  writes:


I did a quick search through the existing test scripts and the
majority do not link commands together with && when they are in a sub
function like this.  I find not having them linked together is easier
to write, maintain and is more readable.


I had an impression that it is true only when the series of commands
are not about Git.  When testing git commands, we should expect any
of them to be broken (by somebody else ;-) and prepare to notice it.



Fixed

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Junio C Hamano]

Ben Peart  writes:

> I did a quick search through the existing test scripts and the
> majority do not link commands together with && when they are in a sub
> function like this.  I find not having them linked together is easier
> to write, maintain and is more readable.

I had an impression that it is true only when the series of commands
are not about Git.  When testing git commands, we should expect any
of them to be broken (by somebody else ;-) and prepare to notice it.

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 9:18 AM, Christian Couder wrote:

On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:

[...]


diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
new file mode 100755
index 00..395db46d55
--- /dev/null
+++ b/t/t7519-status-fsmonitor.sh
@@ -0,0 +1,158 @@
+#!/bin/sh
+
+test_description='git status with file system watcher'
+
+. ./test-lib.sh
+
+clean_repo () {
+   git reset --hard HEAD
+   git clean -fd
+   rm -f marker
+}


Maybe link all the commands in the function with "&&".


+dirty_repo () {
+   : >untracked
+   : >dir1/untracked
+   : >dir2/untracked
+   echo 1 >modified
+   echo 2 >dir1/modified
+   echo 3 >dir2/modified
+   echo 4 >new
+   echo 5 >dir1/new
+   echo 6 >dir2/new
+   git add new
+   git add dir1/new
+   git add dir2/new
+}


Idem.



I did a quick search through the existing test scripts and the majority 
do not link commands together with && when they are in a sub function 
like this.  I find not having them linked together is easier to write, 
maintain and is more readable.



+# The test query-fsmonitor hook proc will output a marker file we can use to
+# ensure the hook was actually used to generate the correct results.
+
+test_expect_success 'setup' '
+   mkdir -p .git/hooks &&
+   write_script .git/hooks/query-fsmonitor<<-\EOF &&
+   if [ $1 -ne 1 ]
+   then
+   echo -e "Unsupported query-fsmonitor hook version.\n" >&2
+   exit 1;
+   fi
+   : >marker
+   printf "untracked\0"
+   printf "dir1/untracked\0"
+   printf "dir2/untracked\0"
+   printf "modified\0"
+   printf "dir1/modified\0"
+   printf "dir2/modified\0"
+   printf "new\0""
+   printf "dir1/new\0"
+   printf "dir2/new\0"


Maybe something like the following to save a few lines and remove some
redundancies:

printf "%s\0" untracked dir1/untracked dir2/untracked \
  modified dir1/modified dir2/modified \
  new dir1/new dir2/new

or perhaps:

for f in untracked modified new
do
   printf "%s\0" "$f" "dir1/$f" "dir2/$f"
done



That is a clever solution that certainly is fewer lines of code. 
However, I have to read the loop and think through the logic to figure 
out what it is doing vs the existing implementation where what it is 
doing is apparent from just glancing at the code.  I was also trying to 
maintain consistency with the other status test code in t7508-status.sh



+   EOF
+   : >tracked &&
+   : >modified &&
+   mkdir dir1 &&
+   : >dir1/tracked &&
+   : >dir1/modified &&
+   mkdir dir2 &&
+   : >dir2/tracked &&
+   : >dir2/modified &&
+   git add . &&
+   test_tick &&
+   git commit -m initial &&
+   dirty_repo
+'
+
+cat >.gitignore <<\EOF
+.gitignore
+expect*
+output*
+marker*
+EOF


This could be part of the previous setup test.



I had followed the pattern in t7508-status.sh with this but I can move 
it in if that is the preferred model.



+# Status is well tested elsewhere so we'll just ensure that the results are
+# the same when using core.fsmonitor. First call after turning on the option
+# does a complete scan so need to do two calls to ensure we test the new
+# codepath.
+
+test_expect_success 'status with core.untrackedcache true' '


If this test is using untracked cache, it should perhaps first check
that untracked cache can be used on the current file system.
t7063-status-untracked-cache.sh does that with the following:

test_lazy_prereq UNTRACKED_CACHE '
 { git update-index --test-untracked-cache; ret=$?; } &&
 test $ret -ne 1
'

if ! test_have_prereq UNTRACKED_CACHE; then
 skip_all='This system does not support untracked cache'
 test_done
fi



Good point. I'll change it so that untracked cache is only turned on if 
it is available and that the one test that requires it is skipped if it 
isn't available.



+   git config core.fsmonitor true  &&
+   git config core.untrackedcache true &&
+   git -c core.fsmonitor=false -c core.untrackedcache=true status >expect 
&&


I don't understand why there is " -c core.untrackedcache=true" in the
above command as you already set core.untrackedcache to true on the
previous line.



Defensive programming. :) The global setting was to ensure it was set 
when the test sub-functions clean and dirty were called and the command 
line settings were used to make it explicit what was being tested.  I 
can remove them if it is causing confusion.



+   clean_repo &&
+   git status &&
+   test_path_is_missing marker &&
+   dirty_repo &&
+   git status >output &&
+   test_path_is_file marker &&
+   test_i18ncmp expect output
+'
+
+


Spurious new line.



Fixed


+test_expect_success 'status with core.untrackedcache false' '
+   git config core.fsmonitor 

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Thu, May 25, 2017 at 8:36 PM, Ben Peart  wrote:

[...]

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> new file mode 100755
> index 00..395db46d55
> --- /dev/null
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -0,0 +1,158 @@
> +#!/bin/sh
> +
> +test_description='git status with file system watcher'
> +
> +. ./test-lib.sh
> +
> +clean_repo () {
> +   git reset --hard HEAD
> +   git clean -fd
> +   rm -f marker
> +}

Maybe link all the commands in the function with "&&".

> +dirty_repo () {
> +   : >untracked
> +   : >dir1/untracked
> +   : >dir2/untracked
> +   echo 1 >modified
> +   echo 2 >dir1/modified
> +   echo 3 >dir2/modified
> +   echo 4 >new
> +   echo 5 >dir1/new
> +   echo 6 >dir2/new
> +   git add new
> +   git add dir1/new
> +   git add dir2/new
> +}

Idem.

> +# The test query-fsmonitor hook proc will output a marker file we can use to
> +# ensure the hook was actually used to generate the correct results.
> +
> +test_expect_success 'setup' '
> +   mkdir -p .git/hooks &&
> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +   if [ $1 -ne 1 ]
> +   then
> +   echo -e "Unsupported query-fsmonitor hook version.\n" >&2
> +   exit 1;
> +   fi
> +   : >marker
> +   printf "untracked\0"
> +   printf "dir1/untracked\0"
> +   printf "dir2/untracked\0"
> +   printf "modified\0"
> +   printf "dir1/modified\0"
> +   printf "dir2/modified\0"
> +   printf "new\0""
> +   printf "dir1/new\0"
> +   printf "dir2/new\0"

Maybe something like the following to save a few lines and remove some
redundancies:

   printf "%s\0" untracked dir1/untracked dir2/untracked \
 modified dir1/modified dir2/modified \
 new dir1/new dir2/new

or perhaps:

   for f in untracked modified new
   do
  printf "%s\0" "$f" "dir1/$f" "dir2/$f"
   done

> +   EOF
> +   : >tracked &&
> +   : >modified &&
> +   mkdir dir1 &&
> +   : >dir1/tracked &&
> +   : >dir1/modified &&
> +   mkdir dir2 &&
> +   : >dir2/tracked &&
> +   : >dir2/modified &&
> +   git add . &&
> +   test_tick &&
> +   git commit -m initial &&
> +   dirty_repo
> +'
> +
> +cat >.gitignore <<\EOF
> +.gitignore
> +expect*
> +output*
> +marker*
> +EOF

This could be part of the previous setup test.

> +# Status is well tested elsewhere so we'll just ensure that the results are
> +# the same when using core.fsmonitor. First call after turning on the option
> +# does a complete scan so need to do two calls to ensure we test the new
> +# codepath.
> +
> +test_expect_success 'status with core.untrackedcache true' '

If this test is using untracked cache, it should perhaps first check
that untracked cache can be used on the current file system.
t7063-status-untracked-cache.sh does that with the following:

test_lazy_prereq UNTRACKED_CACHE '
{ git update-index --test-untracked-cache; ret=$?; } &&
test $ret -ne 1
'

if ! test_have_prereq UNTRACKED_CACHE; then
skip_all='This system does not support untracked cache'
test_done
fi

> +   git config core.fsmonitor true  &&
> +   git config core.untrackedcache true &&
> +   git -c core.fsmonitor=false -c core.untrackedcache=true status 
> >expect &&

I don't understand why there is " -c core.untrackedcache=true" in the
above command as you already set core.untrackedcache to true on the
previous line.

> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   git status >output &&
> +   test_path_is_file marker &&
> +   test_i18ncmp expect output
> +'
> +
> +

Spurious new line.

> +test_expect_success 'status with core.untrackedcache false' '
> +   git config core.fsmonitor true &&
> +   git config core.untrackedcache false &&
> +   git -c core.fsmonitor=false -c core.untrackedcache=false status 
> >expect &&

Again core.untrackedcache is already set to false on the previous line.

> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   git status >output &&
> +   test_path_is_file marker &&
> +   test_i18ncmp expect output
> +'
> +
> +# Ensure commands that call refresh_index() to move the index back in time
> +# properly invalidate the fsmonitor cache
> +
> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> +   git config core.fsmonitor true &&
> +   git config core.untrackedcache true &&
> +   clean_repo &&
> +   git status &&
> +   test_path_is_missing marker &&
> +   dirty_repo &&
> +   write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +   :>marker
> +   EOF
> +   git add . &&
> +   git commit -m "to reset" &&
> +   git status &&
> +