Re: [PATCH v4 3/3] tests: rework mailmap tests for git log
On Thu, 11 Jul 2019 at 20:39, Ariadne Conill <ariadne@dereferenced.org> wrote:
>
> In order to prove that the --no-use-mailmap option works as expected,
> we add a test for it which runs with -c log.mailmap=true to ensure that
> the option successfully negates the configured default.
I believe that testing with `-c log.mailmap=true` is not doing much -- if we ignored that config entirely, we would still produce the wanted result. I think it's more important to test with "...=false". (Testing something like `-c log.mailmap=false -c log.mailmap=true` would basically just test our config-parsing in general, and we don't need to do that here -- there are other tests for that. Anyway, I digress.)
You or others might very well disagree with me, so feel free to wait for a while to see if others chime in. Just so you don't have to change back and forth due to my whims.
Show 24 quoted lines
> Additionally, since --use-mailmap is now the default behaviour, we
> remove mentions of --use-mailmap from the tests, since they are
> redundant. We also rework some tests to explicitly define the
> log.mailmap variable in both true and false states.
>
> Signed-off-by: Ariadne Conill <ariadne@dereferenced.org>
> ---
> t/t4203-mailmap.sh | 49 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 41 insertions(+), 8 deletions(-)
>
> diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh
> index 43b1522ea2..3d6086ff96 100755
> --- a/t/t4203-mailmap.sh
> +++ b/t/t4203-mailmap.sh
> @@ -422,8 +422,8 @@ Author: Some Dude <some@dude.xx>
> Author: A U Thor <author@example.com>
> EOF
>
> -test_expect_success 'Log output with --use-mailmap' '
> - git log --use-mailmap | grep Author >actual &&
> +test_expect_success 'Log output with mailmap enabled (default)' '
> + git log | grep Author >actual &&
> test_cmp expect actual
> '
It's a bit unfortunate that we're ignoring the exit status of `git log` since that is the exact command we want to test here. I know you're just following suit here, but if you're touching this line /anyway/, it might make sense to rewrite this into
git log ... >out &&
grep Author out >actual &&
...
Show 5 quoted lines
> -test_expect_success 'Log output with log.mailmap' '
> +test_expect_success 'Log output with log.mailmap enabled in config' '
> git -c log.mailmap=True log | grep Author >actual &&
> test_cmp expect actual
> '
Then the question is if you should change this line "while at it", or
start with a preparatory patch to first just convert all these "git log
| grep" to the pattern I showed above, then have a patch 2/2 with the
actual change you want to make. I'm sure there are different opinions
here about what is right and not. Anyway, I'm not the one to complain if
you just ignore all of these "not so optimal" lines that you're not
touching anyway.
BTW, this is a test where I wonder if it's really worth running. We basically just check that we won't choke completely on this redundant config.
Show 14 quoted lines
> +cat >expect <<\EOF
> +Author: CTO <cto@coompany.xx>
> +Author: claus <me@company.xx>
> +Author: santa <me@company.xx>
> +Author: nick2 <nick2@company.xx>
> +Author: nick2 <bugs@company.xx>
> +Author: nick1 <bugs@company.xx>
> +Author: A U Thor <author@example.com>
> +EOF
> +
> +test_expect_success 'Log output with log.mailmap disabled in config' '
> + git -c log.mailmap=False log | grep Author >actual &&
> + test_cmp expect actual
> +'
Now this test, on the other hand, I really like having!
Again, you're just following suit: Nowadays, we try to run things like "cat ...>expect ..." as part of a "test_expect_success" block. Same question about how maybe one should first convert all existing instances. And again, IMHO it's perfectly fine if you ignore the existing ones, but do it the "correct" way for the ones you're adding.
Show 22 quoted lines
> +
> cat >expect <<\EOF
> Author: Santa Claus <santa.claus@northpole.xx>
> Author: Santa Claus <santa.claus@northpole.xx>
> EOF
>
> -test_expect_success 'Grep author with --use-mailmap' '
> - git log --use-mailmap --author Santa | grep Author >actual &&
> +test_expect_success 'Grep author with mailmap enabled (default)' '
> + git log --author Santa | grep Author >actual &&
> test_cmp expect actual
> '
> cat >expect <<\EOF
> @@ -456,16 +471,34 @@ Author: Santa Claus <santa.claus@northpole.xx>
> Author: Santa Claus <santa.claus@northpole.xx>
> EOF
>
> -test_expect_success 'Grep author with log.mailmap' '
> +test_expect_success 'Grep author with log.mailmap enabled' '
> git -c log.mailmap=True log --author Santa | grep Author >actual &&
> test_cmp expect actual
> '
(Again, I kind of wonder what this buys us.)
Show 6 quoted lines
> -test_expect_success 'Only grep replaced author with --use-mailmap' '
> - git log --use-mailmap --author "<cto@coompany.xx>" >actual &&
> +test_expect_success 'Grep author with log.mailmap disabled' '
> + git -c log.mailmap=False log --author "<santa.claus@northpole.xx>" >actual &&
> + test_must_be_empty actual
> +'
> +test_expect_success 'Grep author with --no-use-mailmap' '
> + git log --no-use-mailmap --author "<santa.claus@northpole.xx>" >actual &&
> test_must_be_empty actual
> '
Show 12 quoted lines
> +test_expect_success 'Only grep replaced author with mailmap enabled' '
> + git log --author "<cto@coompany.xx>" >actual &&
> + test_must_be_empty actual
> +'
> +cat >expect <<\EOF
> +Author: santa <me@company.xx>
> +EOF
> +
> +test_expect_success 'Grep author with --no-use-mailmap + log.mailmap=True' '
> + git -c log.mailmap=True log --no-use-mailmap --author santa | grep Author >actual &&
> + test_cmp expect actual
> +'
It should be possible to drop "-c log.mailmap=true" from this.
I think you could just squash these three commits into one. It would tell a consistent story about how the specification changes, and the tests and the implementation follow suit.
One last thing: I'm kind of assuming this change of default is something that is actually wanted. I don't have strong opinions there, and maybe others disagree. I know something like this has been discussed before, and I kind of suspect the reason it hasn't been done before is that nobody has done it -- not that it isn't wanted. You could probably find more in the archives, e.g., at public-inbox.org/git.
Martin
Ariadne Conill