threads / patch / 41298

patcht6302: drop unnecessary GPG requirement

Subject: [PATCH] t6302: drop unnecessary GPG requirement

## tl;dr

5 messages between Jan 31, 2016 and Feb 2, 2016. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Eric Sunshine· Jan 31, 2016, 20:19 UTC · lore

These tests are concerned specifically with filtering, sorting, formatting behavior of git-for-each-ref, yet if GPG is not present, the entire test script is skipped even though none of the tests depend upon or care whether the tags are signed. This unnecessary dependency upon GPG may prevent these tests from being more widely run, so drop it.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

I noticed this while reviewing[1] v3 of Karthik's "optimize ref-filter.c:populate_value()" series when I tried to run tests the series added but couldn't due to missing GPG.

[1]: http://article.gmane.org/gmane.comp.version-control.git/284766
t/t6302-for-each-ref-filter.sh | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)
Show changes to t/t6302-for-each-ref-filter.sh +2 −9
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..dea2a9e 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -3,13 +3,6 @@
 test_description='test for-each-refs usage of ref-filter APIs'
 
 . ./test-lib.sh
-. "$TEST_DIRECTORY"/lib-gpg.sh
-
-if ! test_have_prereq GPG
-then
-	skip_all="skipping for-each-ref tests, GPG not available"
-	test_done
-fi
 
 test_expect_success 'setup some history and refs' '
 	test_commit one &&
@@ -17,8 +10,8 @@ test_expect_success 'setup some history and refs' '
 	test_commit three &&
 	git checkout -b side &&
 	test_commit four &&
-	git tag -s -m "A signed tag message" signed-tag &&
-	git tag -s -m "Annonated doubly" double-tag signed-tag &&
+	git tag -m "A signed tag message" signed-tag &&
+	git tag -m "Annonated doubly" double-tag signed-tag &&
 	git checkout master &&
 	git update-ref refs/odd/spot master
 '

-- 
2.7.0.333.g9c3d022
Junio C Hamano· Feb 1, 2016, 02:07 UTC · re: Eric Sunshine · lore

Re: [PATCH] t6302: drop unnecessary GPG requirement

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
> These tests are concerned specifically with filtering, sorting,
> formatting behavior of git-for-each-ref, yet if GPG is not present, the
> entire test script is skipped even though none of the tests depend upon
> or care whether the tags are signed. This unnecessary dependency upon
> GPG may prevent these tests from being more widely run, so drop it.

It is conceivable, if not highly plausible, that a change to the code that does the filtering and formatting can become buggy because payload with GPG signature looks somewhat differently from what is in an annotated but not signed tag. Even if "these are specifically filtering..." is true, including tests for signed tags is valuable.

It seems that we are not currently testing with unsigned but annotated tags, and tests that use them would be good, too.

Would it make sense to introduce a helper function specific to this script to be used to prepare the expected output, to replace cat <<, that goes like this?

    test_prepare_expect () {
	if test_have_prereq GPG
        then
		cat
	else
        	sed -e '/signed/d'
	fi
    }
And then use it like this?
    test_expect_success 'check "%(contents:lines=99999)"' '
        test_prepare_expect <<-\EOF &&
	master |three
        ...
        signed-tag |A signed tag message
        ...
        EOF
	git for-each-ref --format=... >actual &&
        test_cmp expect actual
    '
The setup part would need to make GPG conditional, e.g.
test_expect_success 'setup some history and refs' '
	test_commit one &&
	test_commit two &&
	test_commit three &&
	git checkout -b side &&
	test_commit four &&
	git tag -m "An annotated tag" annotated-tag &&
        git tag -m "Annotated doubly" doubly-annotated-tag annotated-tag &&
	if test_have_prereq GPG
	then
                git tag -s -m "A signed tag message" signed-tag &&
                git tag -s -m "Annonated doubly" doubly-signed-tag signed-tag
	fi &&
	git checkout master &&
	git update-ref refs/odd/spot master
'
Perhaps?
Johannes Schindelin· Feb 1, 2016, 07:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] t6302: drop unnecessary GPG requirement

Hi Eric & Junio,
On Sun, 31 Jan 2016, Junio C Hamano wrote:
Show 16 quoted lines
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > These tests are concerned specifically with filtering, sorting,
> > formatting behavior of git-for-each-ref, yet if GPG is not present,
> > the entire test script is skipped even though none of the tests depend
> > upon or care whether the tags are signed. This unnecessary dependency
> > upon GPG may prevent these tests from being more widely run, so drop
> > it.
> 
> [...]
>
> Would it make sense to introduce a helper function specific to this
> script to be used to prepare the expected output, to replace cat <<,
> that goes like this?
> 
> [...]

An even easier solution might be to *not* set up the signed tags in the 'setup' part, but only in the respective test case, and delete them right away after said test case?

Something like this (I even tested this with and without the GPG prereq):
-- snipsnap --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: [PATCH] Do not make t6302 depend on gpg wholesale

There is but a single test case, in fact, that depends on gpg. Let's just make the other test cases independent of gpg and add the GPG prereq to said single test case.

Noticed by Eric Sunshine.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6302-for-each-ref-filter.sh | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)
Show changes to t/t6302-for-each-ref-filter.sh +4 −29
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index fe4796c..e3a5636 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -5,20 +5,12 @@ test_description='test for-each-refs usage of ref-filter APIs'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-if ! test_have_prereq GPG
-then
-	skip_all="skipping for-each-ref tests, GPG not available"
-	test_done
-fi
-
 test_expect_success 'setup some history and refs' '
 	test_commit one &&
 	test_commit two &&
 	test_commit three &&
 	git checkout -b side &&
 	test_commit four &&
-	git tag -s -m "A signed tag message" signed-tag &&
-	git tag -s -m "Annonated doubly" double-tag signed-tag &&
 	git checkout master &&
 	git update-ref refs/odd/spot master
 '
@@ -33,7 +25,10 @@ test_expect_success 'filtering with --points-at' '
 	test_cmp expect actual
 '
 
-test_expect_success 'check signed tags with --points-at' '
+test_expect_success GPG 'check signed tags with --points-at' '
+	git tag -s -m "A signed tag message" signed-tag side &&
+	git tag -s -m "Annonated doubly" double-tag signed-tag &&
+	test_when_finished git tag -d signed-tag &&
 	sed -e "s/Z$//" >expect <<-\EOF &&
 	refs/heads/side Z
 	refs/tags/four Z
@@ -58,9 +53,7 @@ test_expect_success 'filtering with --merged' '
 test_expect_success 'filtering with --no-merged' '
 	cat >expect <<-\EOF &&
 	refs/heads/side
-	refs/tags/double-tag
 	refs/tags/four
-	refs/tags/signed-tag
 	EOF
 	git for-each-ref --format="%(refname)" --no-merged=master >actual &&
 	test_cmp expect actual
@@ -71,9 +64,7 @@ test_expect_success 'filtering with --contains' '
 	refs/heads/master
 	refs/heads/side
 	refs/odd/spot
-	refs/tags/double-tag
 	refs/tags/four
-	refs/tags/signed-tag
 	refs/tags/three
 	refs/tags/two
 	EOF
@@ -90,10 +81,8 @@ test_expect_success 'left alignment is default' '
 	refname is refs/heads/master  |refs/heads/master
 	refname is refs/heads/side    |refs/heads/side
 	refname is refs/odd/spot      |refs/odd/spot
-	refname is refs/tags/double-tag|refs/tags/double-tag
 	refname is refs/tags/four     |refs/tags/four
 	refname is refs/tags/one      |refs/tags/one
-	refname is refs/tags/signed-tag|refs/tags/signed-tag
 	refname is refs/tags/three    |refs/tags/three
 	refname is refs/tags/two      |refs/tags/two
 	EOF
@@ -106,10 +95,8 @@ test_expect_success 'middle alignment' '
 	| refname is refs/heads/master |refs/heads/master
 	|  refname is refs/heads/side  |refs/heads/side
 	|   refname is refs/odd/spot   |refs/odd/spot
-	|refname is refs/tags/double-tag|refs/tags/double-tag
 	|  refname is refs/tags/four   |refs/tags/four
 	|   refname is refs/tags/one   |refs/tags/one
-	|refname is refs/tags/signed-tag|refs/tags/signed-tag
 	|  refname is refs/tags/three  |refs/tags/three
 	|   refname is refs/tags/two   |refs/tags/two
 	EOF
@@ -122,10 +109,8 @@ test_expect_success 'right alignment' '
 	|  refname is refs/heads/master|refs/heads/master
 	|    refname is refs/heads/side|refs/heads/side
 	|      refname is refs/odd/spot|refs/odd/spot
-	|refname is refs/tags/double-tag|refs/tags/double-tag
 	|     refname is refs/tags/four|refs/tags/four
 	|      refname is refs/tags/one|refs/tags/one
-	|refname is refs/tags/signed-tag|refs/tags/signed-tag
 	|    refname is refs/tags/three|refs/tags/three
 	|      refname is refs/tags/two|refs/tags/two
 	EOF
@@ -140,10 +125,8 @@ test_expect_success 'alignment with format quote' "
 	|'      '\''master| A U Thor'\''      '|
 	|'       '\''side| A U Thor'\''       '|
 	|'     '\''odd/spot| A U Thor'\''     '|
-	|'        '\''double-tag| '\''        '|
 	|'       '\''four| A U Thor'\''       '|
 	|'       '\''one| A U Thor'\''        '|
-	|'        '\''signed-tag| '\''        '|
 	|'      '\''three| A U Thor'\''       '|
 	|'       '\''two| A U Thor'\''        '|
 	EOF
@@ -156,10 +139,8 @@ test_expect_success 'nested alignment with quote formatting' "
 	|'         master               '|
 	|'           side               '|
 	|'       odd/spot               '|
-	|'     double-tag               '|
 	|'           four               '|
 	|'            one               '|
-	|'     signed-tag               '|
 	|'          three               '|
 	|'            two               '|
 	EOF
@@ -172,10 +153,8 @@ test_expect_success 'check `%(contents:lines=1)`' '
 	master |three
 	side |four
 	odd/spot |three
-	double-tag |Annonated doubly
 	four |four
 	one |one
-	signed-tag |A signed tag message
 	three |three
 	two |two
 	EOF
@@ -188,10 +167,8 @@ test_expect_success 'check `%(contents:lines=0)`' '
 	master |
 	side |
 	odd/spot |
-	double-tag |
 	four |
 	one |
-	signed-tag |
 	three |
 	two |
 	EOF
@@ -204,10 +181,8 @@ test_expect_success 'check `%(contents:lines=99999)`' '
 	master |three
 	side |four
 	odd/spot |three
-	double-tag |Annonated doubly
 	four |four
 	one |one
-	signed-tag |A signed tag message
 	three |three
 	two |two
 	EOF

-- 
2.7.0.windows.1.7.g55a05c8
Junio C Hamano· Feb 1, 2016, 18:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t6302: drop unnecessary GPG requirement

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> An even easier solution might be to *not* set up the signed tags in the
> 'setup' part, but only in the respective test case, and delete them right
> away after said test case?

After reading your patch, I do not find it an "easier solution", at least with the definition of the word "solution" I would use. It stops testing signed or doubly signed tags everywhere, assuming that future regressions can ever break only --points-at tests and no other tests around signed tags. The easiest solution along that same line of thought taken to the extreme would be to say test_done without having any test ;-)

The "filter entries about signed tags from the expected output" approach I suggested would not work if the expected output files are not line oriented and lines that would appear only when signed tags are tested cannot be easily filtered out (like with the sed script in the message you are responding to), but I think for the existing test cases (with or without additions of annotated but unsigned tags) it would work.

Show 7 quoted lines
> Something like this (I even tested this with and without the GPG prereq):
> ...
> -test_expect_success 'check signed tags with --points-at' '
> +test_expect_success GPG 'check signed tags with --points-at' '
> +	git tag -s -m "A signed tag message" signed-tag side &&
> +	git tag -s -m "Annonated doubly" double-tag signed-tag &&
> +	test_when_finished git tag -d signed-tag &&
Interestingly, double-tag is not removed here.
Johannes Schindelin· Feb 2, 2016, 07:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] t6302: drop unnecessary GPG requirement

Hi Junio,
On Mon, 1 Feb 2016, Junio C Hamano wrote:
Show 11 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > An even easier solution might be to *not* set up the signed tags in the
> > 'setup' part, but only in the respective test case, and delete them right
> > away after said test case?
> 
> After reading your patch, I do not find it an "easier solution", at
> least with the definition of the word "solution" I would use.  It
> stops testing signed or doubly signed tags everywhere, assuming that
> future regressions can ever break only --points-at tests and no
> other tests around signed tags.
True.
Show 9 quoted lines
> > Something like this (I even tested this with and without the GPG prereq):
> > ...
> > -test_expect_success 'check signed tags with --points-at' '
> > +test_expect_success GPG 'check signed tags with --points-at' '
> > +	git tag -s -m "A signed tag message" signed-tag side &&
> > +	git tag -s -m "Annonated doubly" double-tag signed-tag &&
> > +	test_when_finished git tag -d signed-tag &&
> 
> Interestingly, double-tag is not removed here.
Whooopsie.
I retract the patch in any case ;-)

Ciao, Dscho

← back to recent threads