Skip to content

quiche: import some quic test utilities#7709

Merged
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
danzh2010:testutils
Jul 26, 2019
Merged

quiche: import some quic test utilities#7709
mattklein123 merged 10 commits intoenvoyproxy:masterfrom
danzh2010:testutils

Conversation

@danzh2010
Copy link
Copy Markdown
Contributor

@danzh2010 danzh2010 commented Jul 24, 2019

Bring quic/test_tools/quic_test_utils.h and quic/test_tools/crypto_test_utils.h for future testing. As crypto_test_utils.h has platform-dependent interfaces, their definitions is added in test/extensions/quic_listeners/quiche/crypto_test_utils_for_envoy.cc.

Risk Level: low, test only
Testing: no tested as the platform specific function definitions are quite straight forward
Part of: #2557

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

/assign @wu-bin @alyssawilk

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7709 (comment) was created by @danzh2010.

see: more, trace.

@danzh2010
Copy link
Copy Markdown
Contributor Author

/retest

@repokitteh-read-only
Copy link
Copy Markdown

🔨 rebuilding ci/circleci: compile_time_options (failed build)
🔨 rebuilding ci/circleci: release (failed build)
🔨 rebuilding ci/circleci: tsan (failed build)
🔨 rebuilding ci/circleci: asan (failed build)
🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7709 (comment) was created by @danzh2010.

see: more, trace.

wu-bin
wu-bin previously approved these changes Jul 25, 2019
Copy link
Copy Markdown
Contributor

@wu-bin wu-bin left a comment

Choose a reason for hiding this comment

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

LGTM. Just 2 nits.


TEST_F(QuicPlatformTest, LogEndOfLine) {
GetLogger().set_level(ERROR);
QUIC_DLOG(ERROR) << "aaaa" << std::endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this emit an extra empty new line compared to QUIC_DLOG(ERROR) << "aaaa";?

If yes, can we strip the ending new lines in quic logging impl?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

#define VALUE_BY_COMPILE_MODE(debug_mode_value, release_mode_value) debug_mode_value
#endif

TEST_F(QuicPlatformTest, LogEndOfLine) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we test a few more io manipulators? Some code may use std::hex, std::dec, std::fixed etc.

Also please change the test name to something like QuicLogIoManipulators.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
wu-bin
wu-bin previously approved these changes Jul 25, 2019
}
GetLogger().log(level_, "{}", stream_.str().c_str());
std::string content = stream_.str();
if (content.back() == '\n') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check if content is empty before calling back().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

if (content.back() == '\n') {
// strip the last trailing '\n' because spd log will add a trailing '\n' to
// the output.
content.replace(content.length() - 1, 1, 1, '\0');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can just do content.back() = '\0';

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


TEST_F(QuicPlatformTest, LogEndOfLine) {
GetLogger().set_level(ERROR);
QUIC_DLOG(ERROR) << "aaaa" << std::endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010
Copy link
Copy Markdown
Contributor Author

/assign @mattklein123

@danzh2010
Copy link
Copy Markdown
Contributor Author

ping? @mattklein123

@mattklein123 mattklein123 merged commit 6b9a116 into envoyproxy:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants