Test on correctness of serving short/long headers #169
Conversation
h2/test_h2_headers.py
Outdated
| 'type' : 'external', | ||
| 'binary' : 'curl', | ||
| 'cmd_args' : ( | ||
| '-k ' |
There was a problem hiding this comment.
-f key might be handy here, Normally curl return 0 if a response is received, with the key it returns non-null code on 4xx and 5xx responses, and the test will be marked as failed.
h2/test_h2_headers.py
Outdated
| encoding in HTTP/2. A request or response containing uppercase | ||
| header field names MUST be treated as malformed. | ||
| Ask curl to make an h2 request, if header field names are not converted to | ||
| lowercase then curl return code is not 0. |
There was a problem hiding this comment.
Meaningless copy-paste. Just say that it's the same case as <...> but checks cached, not forwarded responses.
h2/test_h2_headers.py
Outdated
| 'config' : NGINX_CONFIG % """ | ||
| add_header X-Extra-Data1 "q"; | ||
| add_header X-Extra-Data2 "q"; | ||
| add_header X-Extra-Data1 "q"; |
There was a problem hiding this comment.
This is not some random set of headers, but it's related to existing issue. Need to explicitly add description on the case and why do we have it here. Also tempesta-tech/tempesta#1408 tells about multiple cases, long and short ones, with cache and without. All of them must be covered.
avbelov23
changed the title
h2/test_h2_headers.py
Outdated
|
|
||
| class AddBackendShortHeaders(CurlTestBase): | ||
| ''' The test checks the correctness of forwarding short headers with | ||
| duplication |
There was a problem hiding this comment.
Seems like incomplete sentence. Need to highlight that headers in the test response must be specifically ordered.
h2/test_h2_headers.py
Outdated
| CurlTestBase.test(self) | ||
| nginx = self.get_server('nginx') | ||
| nginx.get_stats() | ||
| self.assertEqual(1, nginx.requests) |
There was a problem hiding this comment.
First I wanted to say that it's better to check counters on Tempesta side. But I see your intention, backend counters won't lie about serving from cache, while system under test possibly can. So Ok, lets leave it as is.
avbelov23
force-pushed
the
avb-1380
branch
2 times, most recently
from
59bc27e
to
79f436c
Compare
check_deps/check_cmds_nodes.py
Outdated
| }, | ||
| { | ||
| "cmd" : tf_cfg.cfg.get("Client", "h2load"), | ||
| "install" : "h2load" |
There was a problem hiding this comment.
This is package name, nghttp2-client for Debian if I'm not mistaken.
h2/test_h2_headers.py
Outdated
| } | ||
|
|
||
| def test(self): | ||
| CurlTestBase.test(self) |
There was a problem hiding this comment.
Seems like there is no need to override test() function from CurlTestBase class here.
h2/test_h2_headers.py
Outdated
|
|
||
| nginx = self.get_server('nginx') | ||
| nginx.get_stats() | ||
| self.assertEqual(1 if served_from_cache else 2, nginx.requests) |
There was a problem hiding this comment.
Add a msg attribute here to provide a good explanation what happened wrong here. It's very handy when a test fails. Saves a lot of time on debugging.
| 'type' : 'external', | ||
| 'binary' : 'h2load', | ||
| 'ssl' : True, | ||
| 'cmd_args' : ' https://${tempesta_ip} -D100' |
There was a problem hiding this comment.
Duration is configured in tests configuration file. Better to follow that value and avoid hardcoded ones. Unless hardcoded test duration if really required, e.g. to stay in sync with some OS/application timers.
There was a problem hiding this comment.
There is a dependence on time. The duration from the config may be small
tempesta-tech/tempesta#1408