[PATCH] Make test script annotate-tests.sh handle missing authors

[PATCH] Make test script annotate-tests.sh handle missing authors

[Kevin Ballard]

Initialize the %count hash to contain all the expected authors already.
This allows the script to print an error if an expected author was omitted
entirely from the blame/annotate output.

Signed-off-by: Kevin Ballard <kevin@sb.org>
---
I discovered this omission when trying to write a test for a change to
git-blame that I will be submitting shortly. Without this change, if an
author never showed up in the blame output, it would errneously consider
that to be ok. It still ignores authors that were never specified as
expected in the first place, but I wasn't so sure that was an error.

Also, I'm not a Perl programmer, so it's possible there's a better idiom
for this sort of thing.

 t/annotate-tests.sh |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 396b965..4e37a66 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -9,6 +9,9 @@ check_count () {
 	cat .result | perl -e '
 		my %expect = (@ARGV);
 		my %count = ();
+		while (my ($author, $count) = each %expect) {
+			$count{$author} = 0;
+		}
 		while (<STDIN>) {
 			if (/^[0-9a-f]+\t\(([^\t]+)\t/) {
 				my $author = $1;
-- 
1.7.3.1.186.gc0af9.dirty

Re: [PATCH] Make test script annotate-tests.sh handle missing authors

[Jakub Narebski]

Kevin Ballard <kevin@sb.org> writes:

> Also, I'm not a Perl programmer, so it's possible there's a better idiom
> for this sort of thing.
> 
>  t/annotate-tests.sh |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> index 396b965..4e37a66 100644
> --- a/t/annotate-tests.sh
> +++ b/t/annotate-tests.sh
> @@ -9,6 +9,9 @@ check_count () {
>  	cat .result | perl -e '
>  		my %expect = (@ARGV);
>  		my %count = ();
> +		while (my ($author, $count) = each %expect) {
> +			$count{$author} = 0;
> +		}


First, it is a very bad practice to have variables of different type
named the same way, here %count (hash) and $count (scalar, unused).

Perl idiom way would be

  -		my %count = ();
  +		my %count = map { $_ => 0 } keys %expect;

>  		while (<STDIN>) {
>  			if (/^[0-9a-f]+\t\(([^\t]+)\t/) {
>  				my $author = $1;
> -- 
> 1.7.3.1.186.gc0af9.dirty
> 

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] Make test script annotate-tests.sh handle missing authors

[Kevin Ballard]

On Oct 16, 2010, at 12:34 AM, Jakub Narebski wrote:

> Kevin Ballard <kevin@sb.org> writes:
> 
>> Also, I'm not a Perl programmer, so it's possible there's a better idiom
>> for this sort of thing.
>> 
>> t/annotate-tests.sh |    3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>> 
>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>> index 396b965..4e37a66 100644
>> --- a/t/annotate-tests.sh
>> +++ b/t/annotate-tests.sh
>> @@ -9,6 +9,9 @@ check_count () {
>> 	cat .result | perl -e '
>> 		my %expect = (@ARGV);
>> 		my %count = ();
>> +		while (my ($author, $count) = each %expect) {
>> +			$count{$author} = 0;
>> +		}
> 
> 
> First, it is a very bad practice to have variables of different type
> named the same way, here %count (hash) and $count (scalar, unused).

Thanks for the pointer, but $count is already used in the while loop below:

		while (my ($author, $count) = each %count) {
			my $ok;
			if ($expect{$author} != $count) {
				$bad = 1;
				$ok = "bad";
			}
			else {
				$ok = "good";
			}
			print STDERR "Author $author (expected $expect{$author}, attributed $count) $ok\n";
		}

I'll go and change the one in my new while loop though, to use the Perl idiom way you listed below.

> Perl idiom way would be
> 
>  -		my %count = ();
>  +		my %count = map { $_ => 0 } keys %expect;

-Kevin Ballard

[PATCH v2] Update test script annotate-tests.sh to handle missing/extra authors

[Kevin Ballard]

The current script used by annotate-tests.sh (used by t8001 and t8002) fails
to emit a warning if any of the expected authors never show up in the output
or if authors that show up in the output were never specified as expected.
Update the script to fail in both of these scenarios.

Helped-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Kevin Ballard <kevin@sb.org>
---
 t/annotate-tests.sh |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 396b965..141b60c 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh
@@ -8,27 +8,27 @@ check_count () {
 	$PROG file $head >.result || return 1
 	cat .result | perl -e '
 		my %expect = (@ARGV);
-		my %count = ();
+		my %count = map { $_ => 0 } keys %expect;
 		while (<STDIN>) {
 			if (/^[0-9a-f]+\t\(([^\t]+)\t/) {
 				my $author = $1;
 				for ($author) { s/^\s*//; s/\s*$//; }
-				if (exists $expect{$author}) {
-					$count{$author}++;
-				}
+				$count{$author}++;
 			}
 		}
 		my $bad = 0;
 		while (my ($author, $count) = each %count) {
 			my $ok;
-			if ($expect{$author} != $count) {
+			my $value = 0;
+			$value = $expect{$author} if defined $expect{$author};
+			if ($value != $count) {
 				$bad = 1;
 				$ok = "bad";
 			}
 			else {
 				$ok = "good";
 			}
-			print STDERR "Author $author (expected $expect{$author}, attributed $count) $ok\n";
+			print STDERR "Author $author (expected $value, attributed $count) $ok\n";
 		}
 		exit($bad);
 	' "$@"
-- 
1.7.3.1.208.g53ea2

Re: [PATCH] Make test script annotate-tests.sh handle missing authors

[Jakub Narebski]

On Sat, 16 Oct 2010, Kevin Ballard wrote:
> On Oct 16, 2010, at 12:34 AM, Jakub Narebski wrote:
>> Kevin Ballard <kevin@sb.org> writes:
>> 
>>> Also, I'm not a Perl programmer, so it's possible there's a better idiom
>>> for this sort of thing.
>>> 
>>> t/annotate-tests.sh |    3 +++
>>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
>>> index 396b965..4e37a66 100644
>>> --- a/t/annotate-tests.sh
>>> +++ b/t/annotate-tests.sh
>>> @@ -9,6 +9,9 @@ check_count () {
>>> 	cat .result | perl -e '
>>> 		my %expect = (@ARGV);
>>> 		my %count = ();
>>> +		while (my ($author, $count) = each %expect) {
>>> +			$count{$author} = 0;
>>> +		}
>> 
>> 
>> First, it is a very bad practice to have variables of different type
>> named the same way, here %count (hash) and $count (scalar, unused).
> 
> Thanks for the pointer, but $count is already used in the while loop below:
> 
> 		while (my ($author, $count) = each %count) {
> 			my $ok;
> 			if ($expect{$author} != $count) {
> 				$bad = 1;
> 				$ok = "bad";
> 			}
> 			else {
> 				$ok = "good";
> 			}
> 			print STDERR "Author $author (expected $expect{$author}, attributed $count) $ok\n";
> 		}

Hmmm... the %count hash should probably be named %actual (to complement
%expect), or %attributed (like in output).

-- 
Jakub Narebski
Poland