unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
* [PATCH 0/3] t/www_listing: update for Grokmirror v2
@ 2021-02-21 21:46 Kyle Meyer
  2021-02-21 21:46 ` [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip Kyle Meyer
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Kyle Meyer @ 2021-02-21 21:46 UTC (permalink / raw)
  To: meta

The test suite is failing on my end due to incompatibilities with
Grokmirror v2:

  ERROR: Section [core] must exist in: /tmp/pi-www_listing-4290-4AZck2/repos.conf
         Perhaps this is a grokmirror-1.x config file?

Patch 3 switches the generated manifest to be compatible with v2,
skipping the tests for older Grokmirror versions.

  [1/3] t/www_listing: correct the number of tests for grok-pull skip
  [2/3] t/www_listing: reword grok-pull skip message
  [3/3] t/www_listing: require grok-pull version 2 or later

 t/www_listing.t | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)


base-commit: dda6a67ce8865641edad51f83aab2eb2fc27bda8
-- 
2.30.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip
  2021-02-21 21:46 [PATCH 0/3] t/www_listing: update for Grokmirror v2 Kyle Meyer
@ 2021-02-21 21:46 ` Kyle Meyer
  2021-02-21 22:02   ` Eric Wong
  2021-02-21 21:46 ` [PATCH 2/3] t/www_listing: reword grok-pull skip message Kyle Meyer
  2021-02-21 21:46 ` [PATCH 3/3] t/www_listing: require grok-pull version 2 or later Kyle Meyer
  2 siblings, 1 reply; 7+ messages in thread
From: Kyle Meyer @ 2021-02-21 21:46 UTC (permalink / raw)
  To: meta

---

  I'm not positive that I'm understanding skip's $how_many argument or
  that it matters much.

 t/www_listing.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index 63ef2eacfd407d51..cdf2a1847a945225 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -121,7 +121,7 @@ SKIP: {
 	tiny_test($json, $host, $port);
 
 	my $grok_pull = which('grok-pull') or
-		skip('skipping grok-pull integration test', 2);
+		skip('skipping grok-pull integration test', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] t/www_listing: reword grok-pull skip message
  2021-02-21 21:46 [PATCH 0/3] t/www_listing: update for Grokmirror v2 Kyle Meyer
  2021-02-21 21:46 ` [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip Kyle Meyer
@ 2021-02-21 21:46 ` Kyle Meyer
  2021-02-21 21:46 ` [PATCH 3/3] t/www_listing: require grok-pull version 2 or later Kyle Meyer
  2 siblings, 0 replies; 7+ messages in thread
From: Kyle Meyer @ 2021-02-21 21:46 UTC (permalink / raw)
  To: meta

Make it clear that this skip is because grok-pull isn't available at
all because the next commit will add another skip for older versions
of Grokmirror.
---
 t/www_listing.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index cdf2a1847a945225..bf35530f3494c016 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -121,7 +121,7 @@ SKIP: {
 	tiny_test($json, $host, $port);
 
 	my $grok_pull = which('grok-pull') or
-		skip('skipping grok-pull integration test', 12);
+		skip('grok-pull not available', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] t/www_listing: require grok-pull version 2 or later
  2021-02-21 21:46 [PATCH 0/3] t/www_listing: update for Grokmirror v2 Kyle Meyer
  2021-02-21 21:46 ` [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip Kyle Meyer
  2021-02-21 21:46 ` [PATCH 2/3] t/www_listing: reword grok-pull skip message Kyle Meyer
@ 2021-02-21 21:46 ` Kyle Meyer
  2021-02-21 22:20   ` Eric Wong
  2 siblings, 1 reply; 7+ messages in thread
From: Kyle Meyer @ 2021-02-21 21:46 UTC (permalink / raw)
  To: meta

The grok-pull-based tests in www_listing are incompatible with
Grokmirror v2 in two ways: the generated configuration format and the
expected exit codes.  Update the tests to work with v2, and skip them
for earlier versions.

This was tested with the latest release of Grokmirror, v2.0.7.  Note
that the "pull" and "fsck" sections are required even though they're
empty.
---

  Another option would be to generate an appropriate v1 or v2
  configuration based on which Grokmirror version is detected.  I'm
  not sure that's worth the trouble though.

 t/www_listing.t | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index bf35530f3494c016..6a2892de9b827fe6 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -122,22 +122,27 @@ SKIP: {
 
 	my $grok_pull = which('grok-pull') or
 		skip('grok-pull not available', 12);
+	my ($grok_version) = (xqx([$grok_pull, "--version"])
+			=~ /(\d+)\.(?:\d+)(?:\.(\d+))?/);
+	$grok_version >= 2 or
+		skip('grok-pull v2 or later not available', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
 	print $fh <<"" or die;
-# You can pull from multiple grok mirrors, just create
-# a separate section for each mirror. The name can be anything.
-[test]
-site = http://$host:$port
-manifest = http://$host:$port/manifest.js.gz
+[core]
 toplevel = $tmpdir/mirror
-mymanifest = $tmpdir/local-manifest.js.gz
+manifest = $tmpdir/local-manifest.js.gz
+[remote]
+site = http://$host:$port
+manifest = \${site}/manifest.js.gz
+[pull]
+[fsck]
 
 	close $fh or die;
 
 	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
-	is($? >> 8, 127, 'grok-pull exit code as expected');
+	is($? >> 8, 0, 'grok-pull exit code as expected');
 	for (qw(alt bare v2/git/0.git v2/git/1.git v2/git/2.git)) {
 		ok(-d "$tmpdir/mirror/$_", "grok-pull created $_");
 	}
@@ -146,18 +151,19 @@ mymanifest = $tmpdir/local-manifest.js.gz
 	# /$INBOX/v2/manifest.js.gz
 	open $fh, '>', "$tmpdir/per-inbox.conf" or die;
 	print $fh <<"" or die;
-# You can pull from multiple grok mirrors, just create
-# a separate section for each mirror. The name can be anything.
-[v2]
-site = http://$host:$port
-manifest = http://$host:$port/v2/manifest.js.gz
+[core]
 toplevel = $tmpdir/per-inbox
-mymanifest = $tmpdir/per-inbox-manifest.js.gz
+manifest = $tmpdir/per-inbox-manifest.js.gz
+[remote]
+site = http://$host:$port
+manifest = \${site}/v2/manifest.js.gz
+[pull]
+[fsck]
 
 	close $fh or die;
 	ok(mkdir("$tmpdir/per-inbox"), 'prepare single-v2-inbox mirror');
 	xsys($grok_pull, '-c', "$tmpdir/per-inbox.conf");
-	is($? >> 8, 127, 'grok-pull exit code as expected');
+	is($? >> 8, 0, 'grok-pull exit code as expected');
 	for (qw(v2/git/0.git v2/git/1.git v2/git/2.git)) {
 		ok(-d "$tmpdir/per-inbox/$_", "grok-pull created $_");
 	}
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip
  2021-02-21 21:46 ` [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip Kyle Meyer
@ 2021-02-21 22:02   ` Eric Wong
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Wong @ 2021-02-21 22:02 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta

Kyle Meyer <kyle@kyleam.com> wrote:
> ---
> 
>   I'm not positive that I'm understanding skip's $how_many argument or
>   that it matters much.

It doesn't seem to matter, anymore.

In the old days, Test::More required explicit plan test counts
for everything, but we've always relied on done_testing() (no
args) in our code base with no explicit plan.

I've given up maintaining counts inside test_lei; and
just using `1'.  I suppose it's safe to do for the rest
of our tests, but I'm not against this patch, either.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] t/www_listing: require grok-pull version 2 or later
  2021-02-21 21:46 ` [PATCH 3/3] t/www_listing: require grok-pull version 2 or later Kyle Meyer
@ 2021-02-21 22:20   ` Eric Wong
  2021-02-22 14:05     ` Konstantin Ryabitsev
  0 siblings, 1 reply; 7+ messages in thread
From: Eric Wong @ 2021-02-21 22:20 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta, Konstantin Ryabitsev

Kyle Meyer <kyle@kyleam.com> wrote:
> The grok-pull-based tests in www_listing are incompatible with
> Grokmirror v2 in two ways: the generated configuration format and the
> expected exit codes.  Update the tests to work with v2, and skip them
> for earlier versions.
> 
> This was tested with the latest release of Grokmirror, v2.0.7.  Note
> that the "pull" and "fsck" sections are required even though they're
> empty.
> ---
> 
>   Another option would be to generate an appropriate v1 or v2
>   configuration based on which Grokmirror version is detected.  I'm
>   not sure that's worth the trouble though.

Ugh, some of these incompatible changes to grokmirror are really
annoying and will break existing scripts when I upgrade.
(and I suspect this affects other people, too).

+Cc Konstantin

In any case, I suppose the damage is done and losing some
coverage for now on my Debian stable systems is fine...

>  t/www_listing.t | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/t/www_listing.t b/t/www_listing.t
> index bf35530f3494c016..6a2892de9b827fe6 100644
> --- a/t/www_listing.t
> +++ b/t/www_listing.t
> @@ -122,22 +122,27 @@ SKIP: {
>  
>  	my $grok_pull = which('grok-pull') or
>  		skip('grok-pull not available', 12);
> +	my ($grok_version) = (xqx([$grok_pull, "--version"])
> +			=~ /(\d+)\.(?:\d+)(?:\.(\d+))?/);
> +	$grok_version >= 2 or
> +		skip('grok-pull v2 or later not available', 12);
>  
>  	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
>  	open $fh, '>', "$tmpdir/repos.conf" or die;
>  	print $fh <<"" or die;
> -# You can pull from multiple grok mirrors, just create
> -# a separate section for each mirror. The name can be anything.
> -[test]
> -site = http://$host:$port
> -manifest = http://$host:$port/manifest.js.gz
> +[core]
>  toplevel = $tmpdir/mirror
> -mymanifest = $tmpdir/local-manifest.js.gz
> +manifest = $tmpdir/local-manifest.js.gz
> +[remote]
> +site = http://$host:$port
> +manifest = \${site}/manifest.js.gz
> +[pull]
> +[fsck]
>  
>  	close $fh or die;
>  
>  	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
> -	is($? >> 8, 127, 'grok-pull exit code as expected');
> +	is($? >> 8, 0, 'grok-pull exit code as expected');

In particular, I'm relying on this exit code in at least one of
my scripts.  Now I'll have to RTFM to figure out if I should be
testing any other exit codes or something else...

Anyways, applied and pushed as
44138460e53f90426476fa0c323fc15ef17568df for now;
but not happy this was needed.

Thanks.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] t/www_listing: require grok-pull version 2 or later
  2021-02-21 22:20   ` Eric Wong
@ 2021-02-22 14:05     ` Konstantin Ryabitsev
  0 siblings, 0 replies; 7+ messages in thread
From: Konstantin Ryabitsev @ 2021-02-22 14:05 UTC (permalink / raw)
  To: Eric Wong; +Cc: Kyle Meyer, meta

On Sun, Feb 21, 2021 at 10:20:13PM +0000, Eric Wong wrote:
> > This was tested with the latest release of Grokmirror, v2.0.7.  Note
> > that the "pull" and "fsck" sections are required even though they're
> > empty.

Hmm... That grok-pull requires the [fsck] section is a bug that I introduced
in one of the last versions. :/ I'll put in a fix that doesn't require it.

> >   Another option would be to generate an appropriate v1 or v2
> >   configuration based on which Grokmirror version is detected.  I'm
> >   not sure that's worth the trouble though.
> 
> Ugh, some of these incompatible changes to grokmirror are really
> annoying and will break existing scripts when I upgrade.
> (and I suspect this affects other people, too).

Grokmirror-1.x is not maintained any more and 2.x is a much better codebase
anyway.

It was a difficult call to introduce a config change that intentionally
breaks upgrade path from 1.x to 2.x, but the priority was to avoid a much
bigger problem by sneaking this upgrade on an unsuspecting repo admin.
Grokmirror-2 completely refactors how backend repositories are organized, so
the first grok-fsck run on large repo collections with a lot of alternates
could have had potentially led to a disastrous outcome resulting in repository
corruption. Not a problem for repos on lore.kernel.org where there are no
forks, but very much a problem for git.kernel.org and source.codeaurora.org
mirrors (especially the latter).

So, I had to make the choice between annoyance and potential corruption. :)
The breaking nature of the change is documented in
https://git.sr.ht/~monsieuricon/grokmirror/tree/master/item/UPGRADING.rst

> >  	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
> > -	is($? >> 8, 127, 'grok-pull exit code as expected');
> > +	is($? >> 8, 0, 'grok-pull exit code as expected');
> 
> In particular, I'm relying on this exit code in at least one of
> my scripts.  Now I'll have to RTFM to figure out if I should be
> testing any other exit codes or something else...

I'm pretty sure I did this to make grok-pull work better with systemd, sorry.

-K

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-02-22 14:05 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-21 21:46 [PATCH 0/3] t/www_listing: update for Grokmirror v2 Kyle Meyer
2021-02-21 21:46 ` [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip Kyle Meyer
2021-02-21 22:02   ` Eric Wong
2021-02-21 21:46 ` [PATCH 2/3] t/www_listing: reword grok-pull skip message Kyle Meyer
2021-02-21 21:46 ` [PATCH 3/3] t/www_listing: require grok-pull version 2 or later Kyle Meyer
2021-02-21 22:20   ` Eric Wong
2021-02-22 14:05     ` Konstantin Ryabitsev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).