-
Notifications
You must be signed in to change notification settings - Fork 844
Convert HPACK regression test into unit test using Catch #5687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| check_PROGRAMS = \ | ||
| test_Huffmancode \ | ||
| test_Http2DependencyTree \ | ||
| test_HPACK_EXAMPLES \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any name suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add it to test_HPACK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test_HPACK looks a command line tool to generate data for measuring compression rate using hpack-test-case rather than unit tests.
I'm not sure merging these make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the output as an input for the tools provided from hpack-test-case, but it's not the main goal. We rarely measure compression rate, but test if encoder and decoder works. If you look into the code, you can find that it compares the encode output with pre-encoded data and the decode output with the original plain text headers as well. I'd say the output is just compatible with other tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the measuring compression rate is NOT main purpose.
But I'm still not sure because
-
scope looks different
test_HPACK tests only high level functions likehpack_encode_header_block/hpack_decode_header_block. OTOH, RegressionHPACK.cc tests more basic functions likeencode_integer/decode_integer. -
test_HPACK is enough complicated because it needs to handle arguments and hpack-test-case json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look into the code, you can find that it compares the encode output with pre-encoded data and the decode output with the original plain text headers as well.
How can we compare encoded output with pre-encoded data? IIUC, encoded output could be different for each implementations. (If not, compression rate is same across all implementations. )
What I could found is comparing encoded and decoded header with original header.
https://github.com/apache/trafficserver/blob/master/proxy/http2/test_HPACK.cc#L262
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope looks different
I don't see what your concern is. How can it be a reason to have another executable? Those all are still HPACK. Also, as you may know, I moved functions for primitive type representation to XPACK on quit-latest branch and proposing it on master as #5465. To be honest, making this change at this time doesn't make much sense to me because this will make conflicts and it will delay QUIC merge and 9.0.
test_HPACK is enough complicated
There are many functions just for the current test cases but you don't have to read them when you add tests. How does it bother test writers? Things they have to do is the same (just add another TEST_CASE {}).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, for XPACK stuff, it looks like related tests should go to the hdrs/unit_tests/test_XPACK.cc. Let's merge #5465 first.
@bryancall @SolidWallOfCode would you review #5465 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@masaori335 I approved #5465
|
Tests for XPACK are moved to #5710. I'll move rest of tests in to test_HPACK.cc when it is converted to Catch based. |
No logical change.