threads / patch / 25458

patchMake test script annotate-tests.sh handle missing authors

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

## tl;dr

5 messages between Oct 16, 2010 and Oct 16, 2010. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Kevin Ballard· Oct 16, 2010, 05:50 UTC · lore

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(-)

Show changes to t/annotate-tests.sh +3 −0

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

Jakub Narebski· Oct 16, 2010, 07:34 UTC · re: Kevin Ballard · lore

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

Kevin Ballard <kevin@sb.org> writes:

Show 17 quoted lines

> 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;

Show 6 quoted lines

>  		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

Kevin Ballard· Oct 16, 2010, 10:43 UTC · re: Jakub Narebski · lore

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

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

Show 23 quoted lines

> 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

Kevin Ballard· Oct 16, 2010, 11:09 UTC · re: Kevin Ballard · lore

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

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(-)

Show changes to t/annotate-tests.sh +6 −6

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

Jakub Narebski· Oct 16, 2010, 12:22 UTC · re: Kevin Ballard · lore

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

On Sat, 16 Oct 2010, Kevin Ballard wrote:

Show 38 quoted lines

> 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