[PATCH] Tests: fixed try_run() to remove temporary directory on failure
Sergey Kandaurov
pluknet at nginx.com
Fri Sep 16 17:03:38 UTC 2022
> On 7 Sep 2022, at 05:46, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> Hello!
>
> On Tue, Sep 06, 2022 at 05:52:51PM +0400, Sergey Kandaurov wrote:
>
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet at nginx.com>
>> # Date 1662472340 -14400
>> # Tue Sep 06 17:52:20 2022 +0400
>> # Node ID 7eb87eaf06f4b9161cfe298f195947dbddedd01d
>> # Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7
>> Tests: fixed try_run() to remove temporary directory on failure.
>>
>> Keeping the file handle pointing to the "stderr" file prevented removal
>> of the temporary directory on win32 if execution aborted in run().
>> The fix is to move safe file operations surrounding run() out of the
>> eval block such that the state of file handles is consistent.
>> Fixes a1874249496d.
>
> I believe it might need some additional emphasis that this is
> about Windows only. For example:
>
> Tests: fixed try_run() to remove temporary directory on win32.
>
Agree, thx.
>>
>> diff -r 78fe648d54a7 -r 7eb87eaf06f4 lib/Test/Nginx.pm
>> --- a/lib/Test/Nginx.pm Tue Aug 30 17:24:16 2022 +0400
>> +++ b/lib/Test/Nginx.pm Tue Sep 06 17:52:20 2022 +0400
>> @@ -290,8 +290,9 @@ sub has_daemon($) {
>> sub try_run($$) {
>> my ($self, $message) = @_;
>>
>> + open OLDERR, ">&", \*STDERR;
>> +
>> eval {
>> - open OLDERR, ">&", \*STDERR;
>> open NEWERR, ">", $self->{_testdir} . '/stderr'
>> or die "Can't open stderr: $!";
>> close STDERR;
>> @@ -299,10 +300,10 @@ sub try_run($$) {
>> close NEWERR;
>>
>> $self->run();
>> + };
>>
>> - close STDERR;
>> - open STDERR, ">&", \*OLDERR;
>> - };
>> + close STDERR;
>> + open STDERR, ">&", \*OLDERR;
>>
>> return $self unless $@;
>
> Wouldn't it be better to move all stderr handling out of the eval?
>
> diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
> --- a/lib/Test/Nginx.pm
> +++ b/lib/Test/Nginx.pm
> @@ -291,14 +291,13 @@ sub try_run($$) {
> my ($self, $message) = @_;
>
> open OLDERR, ">&", \*STDERR;
> + open NEWERR, ">", $self->{_testdir} . '/stderr'
> + or die "Can't open stderr: $!";
> + close STDERR;
> + open STDERR, ">&", \*NEWERR;
> + close NEWERR;
>
> eval {
> - open NEWERR, ">", $self->{_testdir} . '/stderr'
> - or die "Can't open stderr: $!";
> - close STDERR;
> - open STDERR, ">&", \*NEWERR;
> - close NEWERR;
> -
> $self->run();
> };
>
The reason was to continue hide '/stderr' open errors in eval,
but I tend to think that it's rather better to die explicitly,
as that's not something to happen.
>
> Alternatively, it might be the right time to backout a1874249496d
> completely and rely on "-e ..." instead, given that it was
> introduced in 1.19.5 and already present in all supported
> versions.
I thought about that, looks like the time has come to cleanup things.
It doesn't break out nicely into two pieces, so comes in one change.
# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1663347154 -14400
# Fri Sep 16 20:52:34 2022 +0400
# Node ID 16c3e0b1c8878bb127425b4da873f8eba604c879
# Parent 78fe648d54a79822a72f3a5f8cc79651b3b1f5a7
Tests: updated try_run() to rely on nginx "-e".
The "-e" command line option introduced in nginx 1.19.5 is now used
to print error line on startup failures with TEST_NGINX_VERBOSE set.
This change replaces a previous approach (a1874249496d) compatible
with pre-1.19.5 nginx versions that used to redirect stderr to file.
Hence, "-e" compatibility is removed.
As a side effect, this fixes temporary directory removal on win32
left on startup failures because the "stderr" file was kept open.
diff --git a/lib/Test/Nginx.pm b/lib/Test/Nginx.pm
--- a/lib/Test/Nginx.pm
+++ b/lib/Test/Nginx.pm
@@ -48,8 +48,6 @@ sub new {
)
or die "Can't create temp directory: $!\n";
$self->{_testdir} =~ s!\\!/!g if $^O eq 'MSWin32';
- mkdir "$self->{_testdir}/logs"
- or die "Can't create logs directory: $!\n";
Test::More::BAIL_OUT("no $NGINX binary found")
unless -x $NGINX;
@@ -291,24 +289,16 @@ sub try_run($$) {
my ($self, $message) = @_;
eval {
- open OLDERR, ">&", \*STDERR;
- open NEWERR, ">", $self->{_testdir} . '/stderr'
- or die "Can't open stderr: $!";
- close STDERR;
- open STDERR, ">&", \*NEWERR;
- close NEWERR;
-
+ open OLDERR, ">&", \*STDERR; close STDERR;
$self->run();
-
- close STDERR;
open STDERR, ">&", \*OLDERR;
};
return $self unless $@;
if ($ENV{TEST_NGINX_VERBOSE}) {
- open F, '<', $self->{_testdir} . '/stderr'
- or die "Can't open stderr: $!";
+ open F, '<', $self->{_testdir} . '/error.log'
+ or die "Can't open error.log: $!";
log_core($_) while (<F>);
close F;
}
@@ -350,10 +340,8 @@ sub run(;$) {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- my @error = $self->has_version('1.19.5') ?
- ('-e', 'error.log') : ();
exec($NGINX, '-p', "$testdir/", '-c', 'nginx.conf',
- @error, @globals)
+ '-e', 'error.log', @globals)
or die "Unable to exec(): $!\n";
}
@@ -425,10 +413,8 @@ sub dump_config() {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- my @error = $self->has_version('1.19.5') ?
- ('-e', 'error.log') : ();
my $command = "$NGINX -T -p $testdir/ -c nginx.conf "
- . join(' ', @error, @globals);
+ . join(' ', '-e', 'error.log', @globals);
return qx/$command 2>&1/;
}
@@ -481,10 +467,8 @@ sub reload() {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- my @error = $self->has_version('1.19.5') ?
- ('-e', 'error.log') : ();
system($NGINX, '-p', $testdir, '-c', "nginx.conf",
- '-s', 'reload', @error, @globals) == 0
+ '-s', 'reload', '-e', 'error.log', @globals) == 0
or die "system() failed: $?\n";
} else {
@@ -506,10 +490,8 @@ sub stop() {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- my @error = $self->has_version('1.19.5') ?
- ('-e', 'error.log') : ();
system($NGINX, '-p', $testdir, '-c', "nginx.conf",
- '-s', 'quit', @error, @globals) == 0
+ '-s', 'quit', '-e', 'error.log', @globals) == 0
or die "system() failed: $?\n";
} else {
@@ -530,10 +512,8 @@ sub stop() {
my @globals = $self->{_test_globals} ?
() : ('-g', "pid $testdir/nginx.pid; "
. "error_log $testdir/error.log debug;");
- my @error = $self->has_version('1.19.5') ?
- ('-e', 'error.log') : ();
system($NGINX, '-p', $testdir, '-c', "nginx.conf",
- '-s', 'stop', @error, @globals) == 0
+ '-s', 'stop', '-e', 'error.log', @globals) == 0
or die "system() failed: $?\n";
} else {
--
Sergey Kandaurov
More information about the nginx-devel
mailing list