> On 26 Aug 2016, at 22:03, Junio C Hamano <[email protected]> wrote:
>
> [email protected] writes:
>
>> From: Lars Schneider <[email protected]>
>>
>> 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.
Thanks,
Lars
