[PATCH 00 of 10] multiple headers tests

[Maxim Dounin]

Hello!

On Wed, Jun 01, 2022 at 03:11:45AM +0400, Sergey Kandaurov wrote:

> 
> > On 21 Apr 2022, at 02:37, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > Hello!
> > 
> > Tests for the multiple headers handling patch series.
> > 
> 
> What do you think about extending the tests
> for ngx_http_link_multi_headers() ?
> Searching for previous headers is a novel routine
> so it might make sense to improve its coverage.
> 
> Something like this:
> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1654038532 -14400
> #      Wed Jun 01 03:08:52 2022 +0400
> # Node ID 3c2a768ef7e3eff6c943e0fa4dd10c9ec487c973
> # Parent  83ec649296126965883e0d65c92ecd99f5397dc7
> Tests: added fastcgi tests for catched bugs in combining headers.
> 
> This improves coverage in a search for previous headers.
> 
> diff --git a/fastcgi_header_params.t b/fastcgi_header_params.t
> --- a/fastcgi_header_params.t
> +++ b/fastcgi_header_params.t
> @@ -25,7 +25,7 @@ eval { require FCGI; };
>  plan(skip_all => 'FCGI not installed') if $@;
>  plan(skip_all => 'win32') if $^O eq 'MSWin32';
>  
> -my $t = Test::Nginx->new()->has(qw/http fastcgi/)->plan(4)
> +my $t = Test::Nginx->new()->has(qw/http fastcgi/)->plan(5)
>  	->write_file_expand('nginx.conf', <<'EOF');
>  
>  %%TEST_GLOBALS%%
> @@ -58,6 +58,8 @@ EOF
>  
>  like(http_get_headers('/'), qr/SEE-THIS/,
>  	'fastcgi request with many ignored headers');
> +like(http_get_headers('/', diff => 1), qr/SEE-THIS/,
> +	'fastcgi request with many different headers');
>  
>  TODO: {
>  local $TODO = 'not yet' unless $t->has_version('1.23.0');
> @@ -94,7 +96,7 @@ like($r, qr/X-Foo: foo, bar, bazz/,
>  
>  sub http_get_headers {
>  	my ($url, %extra) = @_;
> -	return http(<<EOF, %extra);
> +	my $r = <<EOF;
>  GET $url HTTP/1.0
>  Host: localhost
>  X-Blah: ignored header
> @@ -116,8 +118,12 @@ X-Blah: ignored header
>  X-Blah: ignored header
>  X-Blah: ignored header
>  X-Blah: ignored header
> +X-Blah: ignored header
>  
>  EOF
> +	my $n = 0;
> +	$r =~ s/(?<=Blah)(?=:)/$n++/ge if $extra{diff};
> +	return http($r);
>  }

I don't think the particular change is a good idea: it interferes 
with quite different test, designed to catch issues with hiding 
multiple headers due to not enough memory allocated for the 
ignored array (4015:e0a435f5f504).  As a result, it would be 
hard to find out what is actually tested in both tests and how 
many headers are needed in test.

If you think that we need a test with multiple different headers 
(presumably with multiple list parts, so more than 20 headers as 
per ngx_list_init() for r->headers_in.headers), it probably should 
be added separately.  And it might be a good idea to actually test
headers are being correctly passed/merged/ignored across list 
parts.  Not sure we need it though.

-- 
Maxim Dounin
http://mdounin.ru/