> On 26 Aug 2016, at 22:03, Junio C Hamano <gits...@pobox.com> wrote:
> larsxschnei...@gmail.com writes:
>> From: Lars Schneider <larsxschnei...@gmail.com>
>> Use `test_config` to set the config, check that files are empty with
>> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
>> after ">" and "<".
> All of the above are good things to do, but the first one needs to
> be done a bit carefully.
> It is unclear in the above description if you made sure that no
> subsequent test depends on the settings left by earlier test before
> replacing "git config" with "test_config".

I assumed that I would see test failures if a subsequent test depends
on the settings left by earlier tests. I'll add this comment to the
commit message.

>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 7bac2bc..7b45136 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -13,8 +13,8 @@ EOF
>> chmod +x rot13.sh
>> test_expect_success setup '
>> -    git config filter.rot13.smudge ./rot13.sh &&
>> -    git config filter.rot13.clean ./rot13.sh &&
>> +    test_config filter.rot13.smudge ./rot13.sh &&
>> +    test_config filter.rot13.clean ./rot13.sh &&
>>      {
>>          echo "*.t filter=rot13"
> For example, after this conversion, filter.rot13.* will be reverted
> back to unconfigured once setup is done.
>> test_expect_success check '
>> -    cmp test.o test &&
>> -    cmp test.o test.t &&
>> +    test_cmp test.o test &&
>> +    test_cmp test.o test.t &&
>>      # ident should be stripped in the repository
>>      git diff --raw --exit-code :test :test.i &&
> It happens to be true that this subsequent test does not do any
> operation to cause data come from and go into the object database
> for any path that match the pattern "*.t", because for whatever
> reason the previous "setup" step happens to do more than just
> "setup".  It already exercised the filtering by doing "git add" and
> "git checkout".
> If we were writing the script t0021 from scratch today, we would
> have used test_config *AND* squashed there two tests into one
> (i.e. making it a single "the filter and ident operation" test).
> Then it is crystal clear that additional tests on commands that may
> involve filtering will always be added to that combined test
> (e.g. we may try "cat-file --filters" to ensure that rot13 filter is
> excercised).
> But because we are not doing that, it may become tempting to add
> test for a new command that pays attention to a filter to either of
> the test, and it would have worked OK if this patch weren't there.
> Such an addition will break the test, because in the second "check"
> test, the filter commands are no longer active with this patch.
> So while I do appreciate the change for the longer term, I am not
> quite happy with it.  It just looks like an invitation for future
> mistakes.

I'll follow your judgement here. However, from my point of view a future 
addition that causes test failures is no issue. Because these test failures
would be analyzed and then the tests could be refactored accordingly.

>> @@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '
>>      # delete the files and check them out again, using a smudge filter
>>      # that will count the args and echo the command-line back to us
>> -    git config filter.argc.smudge "sh ./argc.sh %f" &&
>> +    test_config filter.argc.smudge "sh ./argc.sh %f" &&
>>      rm "$normal" "$special" &&
>>      git checkout -- "$normal" "$special" &&
> This one is clearly OK.  Anything related to argc filter only
> appears in this single test so it is not just OK to use test_config,
> but it makes our intention very clear that nobody else would use the
> argc filter.  I think similar reasoning would apply to the test_config
> conversion in the remainder of the script.

OK, then I will keep these test_config's and revert the first one.

> As to other types of changes you did in this, I see no problem with
> them.


Reply via email to