From: Junio C Hamano <gitster@pobox•com>
To: larsxschneider@gmail•com
Cc: git@vger•kernel.org, peff@peff•net, sbeller@google•com,
Johannes.Schindelin@gmx•de, jnareb@gmail•com, mlbright@gmail•com
Subject: Re: [PATCH v6 09/13] convert: modernize tests
Date: Fri, 26 Aug 2016 13:03:50 -0700 [thread overview]
Message-ID: <xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160825110752.31581-10-larsxschneider@gmail.com> (larsxschneider@gmail.com's message of "Thu, 25 Aug 2016 13:07:48 +0200")
larsxschneider@gmail•com writes:
> 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".
> 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.
> @@ -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.
next prev parent reply other threads:[~2016-08-26 20:10 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-25 11:07 [PATCH v6 00/13] Git filter protocol larsxschneider
2016-08-25 11:07 ` [PATCH v6 01/13] pkt-line: rename packet_write() to packet_write_fmt() larsxschneider
2016-08-25 11:07 ` [PATCH v6 02/13] pkt-line: extract set_packet_header() larsxschneider
2016-08-25 11:07 ` [PATCH v6 03/13] pkt-line: add packet_write_fmt_gently() larsxschneider
2016-08-25 18:12 ` Stefan Beller
2016-08-25 18:47 ` Lars Schneider
2016-08-25 21:41 ` Junio C Hamano
2016-08-26 9:17 ` Lars Schneider
2016-08-26 17:10 ` Junio C Hamano
2016-08-26 17:23 ` Jeff King
2016-08-25 11:07 ` [PATCH v6 04/13] pkt-line: add packet_flush_gently() larsxschneider
2016-08-25 11:07 ` [PATCH v6 05/13] pkt-line: add packet_write_gently() larsxschneider
2016-08-25 21:50 ` Junio C Hamano
2016-08-26 9:40 ` Lars Schneider
2016-08-26 17:15 ` Junio C Hamano
2016-08-29 9:40 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams larsxschneider
2016-08-25 18:46 ` Stefan Beller
2016-08-25 19:33 ` Lars Schneider
2016-08-25 22:31 ` Junio C Hamano
2016-08-26 0:55 ` Jacob Keller
2016-08-26 17:02 ` Stefan Beller
2016-08-26 17:21 ` Jeff King
2016-08-26 17:17 ` Junio C Hamano
2016-08-25 22:27 ` Junio C Hamano
2016-08-26 10:13 ` Lars Schneider
2016-08-26 17:21 ` Junio C Hamano
2016-08-29 9:43 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size larsxschneider
2016-08-25 18:59 ` Stefan Beller
2016-08-25 19:35 ` Lars Schneider
2016-08-26 19:44 ` Junio C Hamano
2016-08-25 11:07 ` [PATCH v6 08/13] convert: quote filter names in error messages larsxschneider
2016-08-26 19:45 ` Junio C Hamano
2016-08-25 11:07 ` [PATCH v6 09/13] convert: modernize tests larsxschneider
2016-08-26 20:03 ` Junio C Hamano [this message]
2016-08-29 10:09 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 10/13] convert: generate large test files only once larsxschneider
2016-08-25 19:17 ` Stefan Beller
2016-08-25 19:54 ` Lars Schneider
2016-08-29 17:52 ` Junio C Hamano
2016-08-30 11:47 ` Lars Schneider
2016-08-30 16:55 ` Junio C Hamano
2016-08-29 17:46 ` Junio C Hamano
2016-08-30 11:41 ` Lars Schneider
2016-08-30 16:37 ` Jeff King
2016-08-25 11:07 ` [PATCH v6 11/13] convert: make apply_filter() adhere to standard Git error handling larsxschneider
2016-08-25 11:07 ` [PATCH v6 12/13] convert: add filter.<driver>.process option larsxschneider
2016-08-29 22:21 ` Junio C Hamano
2016-08-30 16:27 ` Lars Schneider
2016-08-30 18:59 ` Junio C Hamano
2016-08-30 20:38 ` Lars Schneider
2016-08-30 22:23 ` Junio C Hamano
2016-08-31 4:57 ` Torsten Bögershausen
2016-08-31 13:14 ` Jakub Narębski
2016-08-30 20:46 ` Jakub Narębski
2016-09-05 19:47 ` Lars Schneider
2016-08-25 11:07 ` [PATCH v6 13/13] read-cache: make sure file handles are not inherited by child processes larsxschneider
2016-08-29 18:05 ` Junio C Hamano
2016-08-29 19:03 ` Lars Schneider
2016-08-29 19:45 ` Junio C Hamano
2016-08-30 12:32 ` Lars Schneider
2016-08-30 14:54 ` Torsten Bögershausen
2016-09-01 17:15 ` Junio C Hamano
2016-08-29 15:39 ` [PATCH v6 00/13] Git filter protocol Lars Schneider
2016-08-29 18:09 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk2f3fgbd.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=jnareb@gmail$(echo .)com \
--cc=larsxschneider@gmail$(echo .)com \
--cc=mlbright@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sbeller@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox