Re: [PATCH 2/4] msgmap: mid_insert: reraise on unexpected errors, [PATCH 3/4] t/v2writable: test ENOSPC from fast-import

["Štěpán Němec" <stepnem@smrk.net>]

Just a few nits in the new test messages, if I may:

[2/4]:
On Tue, 23 Jul 2024 21:28:35 +0000
Eric Wong wrote:

> SQLITE_CONSTRAINT is the only SQLite error we really expect under
> normal circumstances.  This avoids infinite loops when writing
> to inboxes after hitting ENOSPC.
[...]
> diff --git a/t/msgmap.t b/t/msgmap.t
[...]
> @@ -70,4 +73,17 @@ is(eval {
>  	'ok'
>  }, 'ok', 'atfork_* work on tmp_clone');
>  
> -done_testing();
> +SKIP: {
> +	my $strace = strace_inject;
> +	open my $fh, '>', my $trace = "$tmpdir/trace.out";
> +	my $rd = popen_rd([ $strace, '-p', $$, '-o', $trace,
> +		'-e', 'inject=pwrite64:error=ENOSPC'], undef, { 2 => 1 });
> +	$rd->poll_in(10) or die 'strace not ready';
> +	is eval { $d->mid_insert('this-better-trigger-ENOSPC@error') },
> +		undef, 'insert fails w/ ENOSPC';
> +	like $@, qr/ disk is full/, 'set $@ for ENOSPC';
                                     ^^^^^^^^^^^^^^^^^
> +	kill 'TERM', $rd->attached_pid;
> +	$rd->close;
> +}
> +
> +done_testing;
> diff --git a/t/v2writable.t b/t/v2writable.t
[...]
> @@ -335,4 +337,18 @@ ok($@, 'V2Writable fails on non-existent dir');
>  	is($mode, 0664, sprintf('0%03o', $mode).' is 0664');
>  }
>  
> -done_testing();
> +SKIP: {
> +	my $strace = strace_inject;
> +	my $eml = eml_load 't/plack-qp.eml';
> +	open my $fh, '>', my $trace = "$inboxdir/trace.out";
> +	my $rd = popen_rd([ $strace, '-p', $$, '-o', $trace,
> +		'-e', 'inject=pwrite64:error=ENOSPC'], undef, { 2 => 1 });
> +	$rd->poll_in(10) or die 'strace not ready';
> +	ok ! eval { $im->add($eml) }, 'v2w->add fails on ENOSPC';
> +	like $@, qr/ disk is full/, 'set $@ for ENOSPC';
                                     ^^^^^^^^^^^^^^^^^

[3/4]:
On Tue, 23 Jul 2024 21:28:36 +0000
Eric Wong wrote:

> This also ensures we can recover after the ENOSPC condition
> is resolved.
> ---
>  t/v2writable.t | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/t/v2writable.t b/t/v2writable.t
> index a062d1b3..93385456 100644
> --- a/t/v2writable.t
> +++ b/t/v2writable.t
[...]
> @@ -340,15 +341,46 @@ ok($@, 'V2Writable fails on non-existent dir');
[...]
> +	$rd->poll_in(10) or die 'strace not ready';
> +	ok !eval { $im->done }, 'done fails with ENOSPC';
> +	ok $@, '$@ set on ENOSPC';
                ^^^^^^^^^^^^^^^^
If I'm correct in assuming that no semantic nuance is
intended between the 'set $@ for ENOSPC' and '$@ set on
ENOSPC' variants, I suggest unifying on the latter (the
unequivocal passive reads clearer than the
imperative-looking 'set $@').

[...]

> +	$im->add($eml) or xbail '->add fail after fixing ENOSPC';

s/fail/fails/ ?

> +	$im->done;
> +	ok !$im->add($eml), '->add detects existing message';
> +	$im->done;
> +	is -s $fh, 0, 'nothing new fast-import stderr';

s/new/new on/ ?

Thanks.

-- 
Štěpán