Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
Eric Sunshine writes: > On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano wrote: >> Jonathan Nieder writes: >> >>> My current thinking is "no" --- the patch has as a justification "Now >>> we can test these aspects of .mailmap handling directly with a >>> low-level tool instead of using the tool most people will use, so do >>> so", which sounds an awful lot like "Reduce test coverage of commonly >>> used tools, because we can". >> >> Yes, that was exactly my reaction that prompted my response. > > Does any of my follow-up commentary result in a different > reaction? Not really. While I _do_ think direct testing has merits, I think that should be done by adding direct tests, not by removing the tests that are meant to protect higher level _users_ of the underlying mechanism from breakages. After all, their breakages may not come from new breakages of the lower level mechanism (i.e. the mailmap.c code) but the way these higher level code makes calls into the mechanism, and direct test of the lower level mechanism will not protect them from the latter kind of breakages. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano wrote: > Jonathan Nieder writes: > >> My current thinking is "no" --- the patch has as a justification "Now >> we can test these aspects of .mailmap handling directly with a >> low-level tool instead of using the tool most people will use, so do >> so", which sounds an awful lot like "Reduce test coverage of commonly >> used tools, because we can". > > Yes, that was exactly my reaction that prompted my response. Does any of my follow-up commentary result in a different reaction? If not, I'll drop patches 3 &4 in the re-roll. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
Jonathan Nieder writes: > My current thinking is "no" --- the patch has as a justification "Now > we can test these aspects of .mailmap handling directly with a > low-level tool instead of using the tool most people will use, so do > so", which sounds an awful lot like "Reduce test coverage of commonly > used tools, because we can". Yes, that was exactly my reaction that prompted my response. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
On Thu, Jul 11, 2013 at 8:55 PM, Jonathan Nieder wrote: > Eric Sunshine wrote: >> On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano wrote: >>> Eric Sunshine writes: > With the introduction of check-mailmap, it is now possible to check .mailmap functionality directly rather than indirectly as a side-effect of other commands (such as git-shortlog), therefore, do so. >>> >>> Does this patch mean that we will now ignore future breakages in >>> shortlog and blame if their mailmap integration becomes buggy? >>> >>> I am not convinced it is a good idea if that is what is going on. >> >> I meant to mention in the cover letter that this patch was open for >> debate, however, it does not eliminate all testing of these other >> commands. >> >> The tests in which git-check-mailmap is substituted for git-shortlog >> all worked against a simplistic two-commit repository. Those tests >> were checking the low-level mailmap functionality under various >> conditions and configurations; they were not especially checking any >> particular behavior of git-shortlog. >> >> There still remain a final eight tests which cover git-shortlog, >> git-log, and git-blame. These tests do check mailmap-related behavior >> of those commands, and they do so using a more involved seven-commit >> repository with "complex" mappings, so we have not necessarily lost >> any checks of mailmap integration for those commands. >> >> Would this patch become more palatable if I stated the above in the >> commit message? > > My current thinking is "no" --- the patch has as a justification "Now > we can test these aspects of .mailmap handling directly with a > low-level tool instead of using the tool most people will use, so do > so", which sounds an awful lot like "Reduce test coverage of commonly > used tools, because we can". > > But I imagine the actual motivation was something other than "because > we can". The motivation is as stated in the subject: Direct rather than indirect testing of mailmap functionality. The tests in question are properly crafted to check only low-level mailmap (not git-shortlog) functionality under various conditions and configurations, yet the mental load on the reader is high because he has to keep in mind the authors and commits in the repository underlying git-shortlog's output. When testing via git-check-mailmap, mental load is low: there is a direct correlation between the "Name " which goes in and that which comes out. What is being tested is the same, but it's easier to reason about and understand the the tests when done directly. When converting the tests, I also discovered an additional, perhaps more important motivation. Most of the tests check only that name remapping functions correctly, however, a couple also attempt to check address remapping (b...@company.xx => b...@company.xy). Even though the tests succeed, they don't actually prove that address remapping was correct (or occurred at all) since git-shortlog does not display the email address. git-check-mailmap always displays the email address, thus the check actually tests the intended email address remapping. > For example, maybe the idea is that after this patch, it > should be easier to make cosmetic improvements to the shortlog, log, > and blame output and only have to change those final 8 tests that are > adequately covering the output? If you have such plans and this patch > makes them easier, it could sound like a good patch as a stepping > stone toward that. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
Eric Sunshine wrote: > On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano wrote: >> Eric Sunshine writes: >>> With the introduction of check-mailmap, it is now possible to check >>> .mailmap functionality directly rather than indirectly as a side-effect >>> of other commands (such as git-shortlog), therefore, do so. >> >> Does this patch mean that we will now ignore future breakages in >> shortlog and blame if their mailmap integration becomes buggy? >> >> I am not convinced it is a good idea if that is what is going on. > > I meant to mention in the cover letter that this patch was open for > debate, however, it does not eliminate all testing of these other > commands. > > The tests in which git-check-mailmap is substituted for git-shortlog > all worked against a simplistic two-commit repository. Those tests > were checking the low-level mailmap functionality under various > conditions and configurations; they were not especially checking any > particular behavior of git-shortlog. > > There still remain a final eight tests which cover git-shortlog, > git-log, and git-blame. These tests do check mailmap-related behavior > of those commands, and they do so using a more involved seven-commit > repository with "complex" mappings, so we have not necessarily lost > any checks of mailmap integration for those commands. > > Would this patch become more palatable if I stated the above in the > commit message? My current thinking is "no" --- the patch has as a justification "Now we can test these aspects of .mailmap handling directly with a low-level tool instead of using the tool most people will use, so do so", which sounds an awful lot like "Reduce test coverage of commonly used tools, because we can". But I imagine the actual motivation was something other than "because we can". For example, maybe the idea is that after this patch, it should be easier to make cosmetic improvements to the shortlog, log, and blame output and only have to change those final 8 tests that are adequately covering the output? If you have such plans and this patch makes them easier, it could sound like a good patch as a stepping stone toward that. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano wrote: > Eric Sunshine writes: > >> With the introduction of check-mailmap, it is now possible to check >> .mailmap functionality directly rather than indirectly as a side-effect >> of other commands (such as git-shortlog), therefore, do so. > > Does this patch mean that we will now ignore future breakages in > shortlog and blame if their mailmap integration becomes buggy? > > I am not convinced it is a good idea if that is what is going on. I meant to mention in the cover letter that this patch was open for debate, however, it does not eliminate all testing of these other commands. The tests in which git-check-mailmap is substituted for git-shortlog all worked against a simplistic two-commit repository. Those tests were checking the low-level mailmap functionality under various conditions and configurations; they were not especially checking any particular behavior of git-shortlog. There still remain a final eight tests which cover git-shortlog, git-log, and git-blame. These tests do check mailmap-related behavior of those commands, and they do so using a more involved seven-commit repository with "complex" mappings, so we have not necessarily lost any checks of mailmap integration for those commands. Would this patch become more palatable if I stated the above in the commit message? -- ES -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
Eric Sunshine writes: > With the introduction of check-mailmap, it is now possible to check > .mailmap functionality directly rather than indirectly as a side-effect > of other commands (such as git-shortlog), therefore, do so. Does this patch mean that we will now ignore future breakages in shortlog and blame if their mailmap integration becomes buggy? I am not convinced it is a good idea if that is what is going on. > > Signed-off-by: Eric Sunshine > --- > t/t4203-mailmap.sh | 133 > ++--- > 1 file changed, 45 insertions(+), 88 deletions(-) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 8645492..48a000b 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' ' > ' > > cat >expect <<\EOF > -A U Thor (1): > - initial > - > -nick1 (1): > - second > - > +A U Thor > +nick1 > EOF > > test_expect_success 'No mailmap' ' > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Repo Guy (1): > - initial > - > -nick1 (1): > - second > - > +Repo Guy > +nick1 > EOF > > test_expect_success 'default .mailmap' ' > echo "Repo Guy " > .mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > # Using a mailmap file in a subdirectory of the repo here, but > # could just as well have been a file outside of the repository > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy > +Internal Guy > EOF > test_expect_success 'mailmap.file set' ' > mkdir -p internal_mailmap && > echo "Internal Guy " > internal_mailmap/.mailmap && > git config mailmap.file internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -External Guy (1): > - initial > - > -Internal Guy (1): > - second > - > +External Guy > +Internal Guy > EOF > test_expect_success 'mailmap.file override' ' > echo "External Guy " >> internal_mailmap/.mailmap && > git config mailmap.file internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Repo Guy (1): > - initial > - > -nick1 (1): > - second > - > +Repo Guy > +nick1 > EOF > > test_expect_success 'mailmap.file non-existent' ' > rm internal_mailmap/.mailmap && > rmdir internal_mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy > +Internal Guy > EOF > > test_expect_success 'name entry after email entry' ' > mkdir -p internal_mailmap && > echo " " >internal_mailmap/.mailmap && > echo "Internal Guy " >>internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy > +Internal Guy > EOF > > test_expect_success 'name entry after email entry, case-insensitive' ' > mkdir -p internal_mailmap && > echo " " >internal_mailmap/.mailmap && > echo "Internal Guy " >>internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -A U Thor (1): > - initial > - > -nick1 (1): > - second > - > +A U Thor > +nick1 > EOF > test_expect_success 'No mailmap files, but configured' ' > rm -f .mailmap internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin actual && > test_cmp expect actual > ' > > @@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' ' > > test_expect_success 'mailmap.blob set' ' > cat >expect <<-\EOF && > - Blob Guy (1): > - second > - > - Repo Guy (1): > - initial > - > + Repo Guy > + Blob Guy > EOF > - git -c mailmap.blob=map:just-bugs shortlog HEAD >actual && > + git -c mailmap.blob=map:just-bugs check-mailmap --stdin \ > + actual && > test_cmp expect actual > ' > > test_expect_success 'mailmap.blob overrides .mailmap' ' > cat >expect <<-\EOF && > - Blob Guy (2): > - initial > - second > - > + Blob Guy > + Blob Guy > EOF > - git -c mailmap.blob=map:both shortlog HEAD >actual && > + git -c mailmap.blob=map:both check-mailmap --stdin \ > +
[PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly
With the introduction of check-mailmap, it is now possible to check .mailmap functionality directly rather than indirectly as a side-effect of other commands (such as git-shortlog), therefore, do so. Signed-off-by: Eric Sunshine --- t/t4203-mailmap.sh | 133 ++--- 1 file changed, 45 insertions(+), 88 deletions(-) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 8645492..48a000b 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' ' ' cat >expect <<\EOF -A U Thor (1): - initial - -nick1 (1): - second - +A U Thor +nick1 EOF test_expect_success 'No mailmap' ' - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -Repo Guy (1): - initial - -nick1 (1): - second - +Repo Guy +nick1 EOF test_expect_success 'default .mailmap' ' echo "Repo Guy " > .mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' # Using a mailmap file in a subdirectory of the repo here, but # could just as well have been a file outside of the repository cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy +Internal Guy EOF test_expect_success 'mailmap.file set' ' mkdir -p internal_mailmap && echo "Internal Guy " > internal_mailmap/.mailmap && git config mailmap.file internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -External Guy (1): - initial - -Internal Guy (1): - second - +External Guy +Internal Guy EOF test_expect_success 'mailmap.file override' ' echo "External Guy " >> internal_mailmap/.mailmap && git config mailmap.file internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -Repo Guy (1): - initial - -nick1 (1): - second - +Repo Guy +nick1 EOF test_expect_success 'mailmap.file non-existent' ' rm internal_mailmap/.mailmap && rmdir internal_mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy +Internal Guy EOF test_expect_success 'name entry after email entry' ' mkdir -p internal_mailmap && echo " " >internal_mailmap/.mailmap && echo "Internal Guy " >>internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy +Internal Guy EOF test_expect_success 'name entry after email entry, case-insensitive' ' mkdir -p internal_mailmap && echo " " >internal_mailmap/.mailmap && echo "Internal Guy " >>internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' cat >expect <<\EOF -A U Thor (1): - initial - -nick1 (1): - second - +A U Thor +nick1 EOF test_expect_success 'No mailmap files, but configured' ' rm -f .mailmap internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin actual && test_cmp expect actual ' @@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' ' test_expect_success 'mailmap.blob set' ' cat >expect <<-\EOF && - Blob Guy (1): - second - - Repo Guy (1): - initial - + Repo Guy + Blob Guy EOF - git -c mailmap.blob=map:just-bugs shortlog HEAD >actual && + git -c mailmap.blob=map:just-bugs check-mailmap --stdin \ + actual && test_cmp expect actual ' test_expect_success 'mailmap.blob overrides .mailmap' ' cat >expect <<-\EOF && - Blob Guy (2): - initial - second - + Blob Guy + Blob Guy EOF - git -c mailmap.blob=map:both shortlog HEAD >actual && + git -c mailmap.blob=map:both check-mailmap --stdin \ + actual && test_cmp expect actual ' test_expect_success 'mailmap.file overrides mailmap.blob' ' cat >expect <<-\EOF && - Blob Guy (1): - second - - Internal Guy (1): - initial - + Internal Guy + Blob Guy EOF git \ -c mailmap.blob=map:both \ -c mailmap.file=internal.map \ - shortlog HEAD >actual && + check-mailmap --stdin actual && test_cmp expect actual ' te
