Hello!

On Mon, Mar 20, 2023 at 06:13:34PM +0400, Sergey Kandaurov wrote:

> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1679321601 -14400
> # Mon Mar 20 18:13:21 2023 +0400
> # Node ID f3225ad9300ee2c11c0dec54b9605e67060b7347
> # Parent 1e1d0f3874b0c5b1e399ec76b0198b5c9c265a36
> Tests: run syslog and error_log tests on win32.
>
> They are supposed to work well now, no reason to skip them.

It might be a good idea to explicitly mention why it wasn't
supposed to work.

> An exception is logging to stderr.

And why stderr is not supposed to.

(Just in case, the culprit is CreateProcess(CREATE_NO_WINDOW) used
when starting worker processes, which creates a process without a
console. This probably can be improved to preserve console if
logging to stderr is used.)

>
> diff --git a/error_log.t b/error_log.t
> --- a/error_log.t
> +++ b/error_log.t
> @@ -22,8 +22,6 @@ use Test::Nginx;
> select STDERR; $| = 1;
> select STDOUT; $| = 1;
>
> -plan(skip_all => 'win32') if $^O eq 'MSWin32';
> -
> my $t = Test::Nginx->new()->has(qw/http limit_req/)
> ->plan(25)->write_file_expand('nginx.conf', <<'EOF');
>
> @@ -110,7 +108,7 @@ EOF
> open OLDERR, ">&", \*STDERR;
> open STDERR, '>', $t->testdir() . '/stderr' or die "Can't reopen STDERR: $!";
> open my $stderr, '<', $t->testdir() . '/stderr'
> - or die "Can't open stderr file: $!";
> + or die "Can't open stderr file: $!" unless $^O eq 'MSWin32';

It shouldn't be a problem to open the file on Windows. Though
will require explicitly closing it at the end of the test, so the
test suite will be able to remove the test directory in
destructor.

(In other tests this will also require reordering of daemon
startup, to avoid leaking the descriptor to daemons.)

>
> $t->run();
>
> @@ -123,40 +121,69 @@ open STDERR, ">&", \*OLDERR;
> http_get('/');
>
> SKIP: {
> -
> skip "no --with-debug", 3 unless $t->has_module('--with-debug');
>
> http_get('/debug');
> isnt(lines($t, 'e_debug_debug.log', '[debug]'), 0, 'file debug debug');
> is(lines($t, 'e_debug_info.log', '[debug]'), 0, 'file debug info');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> isnt(lines($t, 'stderr', '[debug]'), 0, 'stderr debug');

So this won't be a SKIP, but rather TODO. Further, it will
actually work with "master_process off;".

>
> }
>
> +}
> +
> http_get('/info');
> is(lines($t, 'e_info_debug.log', '[info]'), 1, 'file info debug');
> is(lines($t, 'e_info_info.log', '[info]'), 1, 'file info info');
> is(lines($t, 'e_info_notice.log', '[info]'), 0, 'file info notice');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> is(lines($t, 'stderr', '[info]'), 1, 'stderr info');
>
> +}
> +
> http_get('/notice');
> is(lines($t, 'e_notice_info.log', '[notice]'), 1, 'file notice info');
> is(lines($t, 'e_notice_notice.log', '[notice]'), 1, 'file notice notice');
> is(lines($t, 'e_notice_warn.log', '[notice]'), 0, 'file notice warn');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> is(lines($t, 'stderr', '[notice]'), 1, 'stderr notice');
>
> +}
> +
> http_get('/warn');
> is(lines($t, 'e_warn_notice.log', '[warn]'), 1, 'file warn notice');
> is(lines($t, 'e_warn_warn.log', '[warn]'), 1, 'file warn warn');
> is(lines($t, 'e_warn_error.log', '[warn]'), 0, 'file warn error');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> is(lines($t, 'stderr', '[warn]'), 1, 'stderr warn');
>
> +}
> +
> http_get('/error');
> is(lines($t, 'e_error_warn.log', '[error]'), 1, 'file error warn');
> is(lines($t, 'e_error_error.log', '[error]'), 1, 'file error error');
> is(lines($t, 'e_error_alert.log', '[error]'), 0, 'file error alert');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> is(lines($t, 'stderr', '[error]'), 1, 'stderr error');
>
> +}
> +
> # count log messages emitted with various error_log levels
>
> http_get('/file_low');
> @@ -168,6 +195,9 @@ is(lines($t, 'e_multi.log', '[error]'),
> http_get('/file_high');
> is(lines($t, 'e_multi_high.log', '[error]'), 1, 'file high');
>
> +SKIP: {
> +skip 'win32', 3 if $^O eq 'MSWin32';
> +
> http_get('/stderr_low');
> is(lines($t, 'stderr', '[error]'), 2, 'stderr low');
>
> @@ -177,6 +207,8 @@ is(lines($t, 'stderr', '[error]'), 2, 's
> http_get('/stderr_high');
> is(lines($t, 'stderr', '[error]'), 1, 'stderr high');
>
> +}
> +
> ###############################################################################
>
> sub lines {
> diff --git a/mail_error_log.t b/mail_error_log.t
> --- a/mail_error_log.t
> +++ b/mail_error_log.t
> @@ -26,8 +26,6 @@ use Test::Nginx::IMAP;
> select STDERR; $| = 1;
> select STDOUT; $| = 1;
>
> -plan(skip_all => 'win32') if $^O eq 'MSWin32';
> -
> my $t = Test::Nginx->new()->has(qw/mail imap http rewrite/);
>
> $t->plan(30)->write_file_expand('nginx.conf', <<'EOF');
> @@ -87,7 +85,7 @@ EOF
> open OLDERR, ">&", \*STDERR;
> open STDERR, '>', $t->testdir() . '/stderr' or die "Can't reopen STDERR: $!";
> open my $stderr, '<', $t->testdir() . '/stderr'
> - or die "Can't open stderr file: $!";
> + or die "Can't open stderr file: $!" unless $^O eq 'MSWin32';
>
> $t->run_daemon(\&Test::Nginx::IMAP::imap_test_daemon);
> $t->run_daemon(\&syslog_daemon, port(8981), $t, 's_glob.log');
> @@ -117,9 +115,15 @@ isnt(lines($t, 'e_debug.log', '[debug]')
>
> isnt(lines($t, 'e_info.log', '[info]'), 0, 'file info in info');
> is(lines($t, 'e_info.log', '[debug]'), 0, 'file debug in info');
> +
> +SKIP: {
> +skip 'win32', 2 if $^O eq 'MSWin32';
> +
> isnt(lines($t, 'stderr', '[info]'), 0, 'stderr info in info');
> is(lines($t, 'stderr', '[debug]'), 0, 'stderr debug in info');
>
> +}
> +
> # multiple error_log
>
> like($t->read_file('e_glob.log'), qr!nginx/[.0-9]+!, 'error global');
> @@ -255,13 +259,16 @@ sub syslog_daemon {
> LocalAddr => "127.0.0.1:$port"
> );
>
> - open my $fh, '>', $t->testdir() . '/' . $file;
> - select $fh; $| = 1;
> + my $path = $t->testdir() . '/' . $file;
> + open my $fh, '>', $path;
> + close $fh;
>
> while (1) {
> my $buffer;
> $s->recv($buffer, 4096);
> + open $fh, '>>', $path;
> print $fh $buffer . "\n";
> + close $fh;
> }

It might be a good idea to clarify in the commit log why this is
needed. (Notably, that pseudo-processes created by Perl's fork()
emulation on win32 cannot be gracefully stopped when they are
blocked in a system call, and using kill('KILL') as nginx does
seems to leave the files open.)

> }
>
> diff --git a/stream_error_log.t b/stream_error_log.t
> --- a/stream_error_log.t
> +++ b/stream_error_log.t
> @@ -26,8 +26,6 @@ use Test::Nginx::Stream qw/ stream /;
> select STDERR; $| = 1;
> select STDOUT; $| = 1;
>
> -plan(skip_all => 'win32') if $^O eq 'MSWin32';
> -
> my $t = Test::Nginx->new()->has(qw/stream/)->plan(34);
>
> $t->write_file_expand('nginx.conf', <<'EOF');
> @@ -75,7 +73,7 @@ EOF
> open OLDERR, ">&", \*STDERR;
> open STDERR, '>', $t->testdir() . '/stderr' or die "Can't reopen STDERR: $!";
> open my $stderr, '<', $t->testdir() . '/stderr'
> - or die "Can't open stderr file: $!";
> + or die "Can't open stderr file: $!" unless $^O eq 'MSWin32';
>
> $t->run_daemon(\&stream_daemon);
> $t->run_daemon(\&syslog_daemon, port(8983), $t, 's_glob.log');
> @@ -104,9 +102,15 @@ isnt(lines($t, 'e_debug.log', '[debug]')
>
> isnt(lines($t, 'e_info.log', '[info]'), 0, 'file info in info');
> is(lines($t, 'e_info.log', '[debug]'), 0, 'file debug in info');
> +
> +SKIP: {
> +skip 'win32', 2 if $^O eq 'MSWin32';
> +
> isnt(lines($t, 'stderr', '[info]'), 0, 'stderr info in info');
> is(lines($t, 'stderr', '[debug]'), 0, 'stderr debug in info');
>
> +}
> +
> # multiple error_log
>
> like($t->read_file('e_glob.log'), qr!nginx/[.0-9]+!, 'error global');
> @@ -135,8 +139,14 @@ my $msg = 'no live upstreams while conne
>
> unlike($t->read_file('e_glob.log'), qr/$msg/ms, 'stream error in global');
> like($t->read_file('e_info.log'), qr/$msg/ms, 'stream error in info');
> +unlike($t->read_file('e_emerg.log'), qr/$msg/ms, 'stream error in emerg');
> +
> +SKIP: {
> +skip 'win32', 1 if $^O eq 'MSWin32';
> +
> like($t->read_file('stderr'), qr/$msg/ms, 'stream error in info stderr');
> -unlike($t->read_file('e_emerg.log'), qr/$msg/ms, 'stream error in emerg');
> +
> +}
>
> $msg = "bytes from/to client:5/4, bytes from/to upstream:4/5";
>
> @@ -264,13 +274,16 @@ sub syslog_daemon {
> LocalAddr => "127.0.0.1:$port"
> );
>
> - open my $fh, '>', $t->testdir() . '/' . $file;
> - select $fh; $| = 1;
> + my $path = $t->testdir() . '/' . $file;
> + open my $fh, '>', $path;
> + close $fh;
>
> while (1) {
> my $buffer;
> $s->recv($buffer, 4096);
> + open $fh, '>>', $path;
> print $fh $buffer . "\n";
> + close $fh;
> }
> }
>
> diff --git a/syslog.t b/syslog.t
> --- a/syslog.t
> +++ b/syslog.t
> @@ -25,8 +25,6 @@ use Test::Nginx;
> select STDERR; $| = 1;
> select STDOUT; $| = 1;
>
> -plan(skip_all => 'win32') if $^O eq 'MSWin32';
> -
> my $t = Test::Nginx->new()->has(qw/http limit_req/)->plan(62);
>
> $t->write_file_expand('nginx.conf', <<'EOF');
> @@ -186,7 +184,6 @@ is($lines[1], $lines[2], 'error_log many
> # error_log log levels
>
> SKIP: {
> -
> skip "no --with-debug", 1 unless $t->has_module('--with-debug');
>
> isnt(syslog_lines('/debug', '[debug]'), 0, 'debug');
> @@ -340,13 +337,16 @@ sub syslog_daemon {
> LocalAddr => "127.0.0.1:$port"
> );
>
> - open my $fh, '>', $t->testdir() . '/' . $file;
> - select $fh; $| = 1;
> + my $path = $t->testdir() . '/' . $file;
> + open my $fh, '>', $path;
> + close $fh;
>
> while (1) {
> my $buffer;
> $s->recv($buffer, 4096);
> + open $fh, '>>', $path;
> print $fh $buffer . "\n";
> + close $fh;
> }
> }
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel