Hello!

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

>
> > On 21 Apr 2022, at 02:37, Maxim Dounin <mdounin@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@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/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org