Skip to content

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.

Sign up for GitHub

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

Closed
masaori335 wants to merge 1 commit into from
Closed

Convert HPACK regression test into unit test using Catch #5687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ proxy/http/test_proxy_http
proxy/http2/test_Huffmancode
proxy/http2/test_Http2DependencyTree
proxy/http2/test_HPACK
proxy/http2/test_HPACK_EXAMPLES
proxy/http2/hpack-tests/results
proxy/logging/test_LogUtils
proxy/logging/test_LogUtils2
Expand Down
7 changes: 0 additions & 7 deletions proxy/http2/HTTP2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,13 +820,6 @@ Http2::init()

#if TS_HAS_TESTS

void forceLinkRegressionHPACK();
void
forceLinkRegressionHPACKCaller()
{
forceLinkRegressionHPACK();
}

#include "tscore/TestBox.h"

/***********************************************************************************
Expand Down
37 changes: 30 additions & 7 deletions proxy/http2/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,19 +52,16 @@ libhttp2_a_SOURCES = \
HuffmanCodec.cc \
HuffmanCodec.h

if BUILD_TESTS
libhttp2_a_SOURCES += \
RegressionHPACK.cc
endif

check_PROGRAMS = \
test_Huffmancode \
test_Http2DependencyTree \
test_HPACK_EXAMPLES \
Copy link
Contributor Author

@masaori335 masaori335 Jul 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any name suggestion?

Copy link
Contributor

@bryancall bryancall Jul 2, 2019

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?

Copy link
Contributor Author

@masaori335 masaori335 Jul 2, 2019

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.

Copy link
Member

@maskit maskit Jul 3, 2019

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.

Copy link
Contributor Author

@masaori335 masaori335 Jul 3, 2019

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

  1. scope looks different
    test_HPACK tests only high level functions like hpack_encode_header_block/hpack_decode_header_block. OTOH, RegressionHPACK.cc tests more basic functions like encode_integer/decode_integer.

  2. test_HPACK is enough complicated because it needs to handle arguments and hpack-test-case json.

Copy link
Contributor Author

@masaori335 masaori335 Jul 3, 2019

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

Copy link
Member

@maskit maskit Jul 3, 2019

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 {}).

Copy link
Contributor Author

@masaori335 masaori335 Jul 5, 2019

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 ?

Copy link
Contributor

@bryancall bryancall Jul 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masaori335 I approved #5465

test_HPACK

TESTS = \
test_Huffmancode \
test_Http2DependencyTree \
test_HPACK_EXAMPLES \
test_HPACK

test_Huffmancode_LDADD = \
Expand Down Expand Up @@ -104,6 +101,32 @@ test_HPACK_SOURCES = \
HPACK.cc \
HPACK.h

clang-tidy-local: $(libhttp2_a_SOURCES) $(test_Huffmancode_SOURCES) \
$(test_Http2DependencyTree_SOURCES) $(test_HPACK_SOURCES)
test_HPACK_EXAMPLES_CPPFLAGS = \
$(AM_CPPFLAGS) \
-I$(abs_top_srcdir)/tests/include

test_HPACK_EXAMPLES_LDADD = \
$(top_builddir)/proxy/hdrs/libhdrs.a \
$(top_builddir)/src/tscore/libtscore.la \
$(top_builddir)/src/tscpp/util/libtscpputil.la \
$(top_builddir)/iocore/eventsystem/libinkevent.a \
$(top_builddir)/lib/records/librecords_p.a \
$(top_builddir)/mgmt/libmgmt_p.la \
$(top_builddir)/proxy/shared/libUglyLogStubs.a \
@HWLOC_LIBS@

test_HPACK_EXAMPLES_SOURCES = \
unit_tests/unit_test_main.cc \
unit_tests/test_HPACK_EXAMPLES.cc \
HuffmanCodec.cc \
HuffmanCodec.h \
HPACK.cc \
HPACK.h

clang-tidy-local: \
$(libhttp2_a_SOURCES) \
$(test_Huffmancode_SOURCES) \
$(test_Http2DependencyTree_SOURCES) \
$(test_HPACK_SOURCES) \
$(test_HPACK_EXAMPLES_SOURCES) \
$(CXX_Clang_Tidy)
Loading