Re: [PATCH v6 09/13] convert: modernize tests
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2016, 20:03 UTC
- Message-ID
- <xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160825110752.31581-10-larsxschneider@gmail.com>
larsxschneider@gmail.com writes:
Show 5 quoted lines
> From: Lars Schneider <larsxschneider@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".
Show 15 quoted lines
> 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.
Show 9 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
As to other types of changes you did in this, I see no problem with them.
Thanks.