Tests for PR - Fix HPACK dynamic table desync#972
Conversation
const-t
changed the title
const-t
force-pushed
the
kt-fix-hpack-desync
branch
from
8680ec1 to
7412b2a
Compare
const-t
force-pushed
the
kt-fix-hpack-desync
branch
from
7412b2a to
f3c94d9
Compare
| # and this default value for table size. | ||
| # Therefore Tempesta does not return bytes of table size in header frame. | ||
| client.update_initial_settings(header_table_size=12288) | ||
| # and we expect \x3f\xe1\x07 bytes in first header frame |
There was a problem hiding this comment.
Please move this comment.
| ) | ||
| self.assertEqual(client.h2_connection.decoder.header_table_size, 4096) | ||
|
|
||
| def test_bytes_of_table_size_in_header_frame_of_trailers(self): |
There was a problem hiding this comment.
You should rebase and rework these tests with asyncio.
| client.h2_connection.decoder.header_table_size = table_size | ||
| self.assertEqual(client.h2_connection.decoder.header_table_size, table_size) |
There was a problem hiding this comment.
You check python side here, the test works without these lines. What do you want to check here?
| client.send_bytes(client.h2_connection.data_to_send()) | ||
| client.wait_for_ack_settings() | ||
|
|
||
| client.last_response_buffer = bytes() # clearing the buffer after exchanging settings |
There was a problem hiding this comment.
Why it is necessary? The test works without the line. I think changing the buffer in the test is a bad idea. Probably we should to change it as a protected variable.
| # send the new table size | ||
| client.send_settings_frame(header_table_size=new_table_size) | ||
| # ensure that the current table size is equal to default 4096 | ||
| self.assertEqual(client.h2_connection.decoder.header_table_size, 4096) |
There was a problem hiding this comment.
Why do you change header_table_size here? Why do you send header_table_size=2048 but expected 4096? Sorry, it is difficult test and i think we need to more comments here. It is point 4 from docstring but I don't understand why do we need to change header_table_size here
And the assert self.assertEqual(client.h2_connection.decoder.header_table_size, 4096) checks python side here.
There was a problem hiding this comment.
I've added comment, we need this to be sure that size of dynamic table is 4096 at this moment, the same as for Tempesta.
| client.increment_flow_control_window(stream_id=1, flow_controlled_length=100) | ||
|
|
||
| # we are ready to receive body and trailers to that | ||
| client.valid_req_num = 1 # change the expected number of responses |
There was a problem hiding this comment.
The line is not necessary. At the moment, the value of valid_req_num is 1.
const-t
force-pushed
the
kt-fix-hpack-desync
branch
from
f3c94d9 to
0219346
Compare
We set the new size before call `connect()` to have buffer of desired size after `connect()`, otherwise it may take some time to resize buffer.
Set of tests to ensure that Tempesta sends all encoded headers even in case when stream closed by the client or Tempesta. Also changed some existent tests accordingly to the new patches where we treat stream level errors as connection errors. Also fix completely wrong test_headers_frame_for_other_stream_after_rst that checked false statement. Also fix the test test_headers_frame_for_other_stream_after_rst that incorrectly validates a false statement
const-t
force-pushed
the
kt-fix-hpack-desync
branch
from
fcb838b to
7fac9b8
Compare
2 participants
No description provided.