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

[Christian Couder <christian.couder@gmail.com>]

On Tue, May 30, 2017 at 11:21 PM, Ben Peart <peartben@gmail.com> wrote:
>
> On 5/30/2017 9:18 AM, Christian Couder wrote:
>>
>> On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> 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.