Skip to content

Add quic_test_output_impl.(h|cc) to QUICHE platform implementation. #6058

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

Merged
alyssawilk merged 10 commits into from
Mar 7, 2019
Merged

Add quic_test_output_impl.(h|cc) to QUICHE platform implementation. #6058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
52b7311
Add quic_test_output_impl.(h|cc) to QUICHE platform implementation.
wu-bin Feb 25, 2019
ec02e27
Merge remote-tracking branch 'upstream/master' into quic_test_output_…
wu-bin Feb 27, 2019
fe2af18
Update per comments.
wu-bin Feb 27, 2019
1a2711b
In test QuicPlatformTest.QuicTestOutput, if QUIC_TEST_OUTPUT_DIR is not
wu-bin Feb 28, 2019
80837ae
Remove a unnecessary getenv in test QuicPlatformTest.QuicTestOutput.
wu-bin Feb 28, 2019
3cf7f7d
Merge remote-tracking branch 'upstream/master' into quic_test_output_…
wu-bin Mar 1, 2019
c65b728
Merge remote-tracking branch 'upstream/master' into quic_test_output_…
wu-bin Mar 4, 2019
c677256
Merge remote-tracking branch 'upstream/master' into quic_test_output_…
wu-bin Mar 6, 2019
e268e03
Add EXPECT_LOG(_NOT)_CONTAINS to QuicPlatformTest.QuicTestOutput.
wu-bin Mar 6, 2019
f0d1018
Merge remote-tracking branch 'upstream/master' into quic_test_output_…
wu-bin Mar 7, 2019
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 bazel/envoy_build_system.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,6 +679,7 @@ def envoy_select_boringssl(if_fips, default = None):
"//conditions:default": default or [],
})

# Selects the part of QUICHE that does not yet work with the current CI.
def envoy_select_quiche(xs, repository = ""):
return select({
repository + "//bazel:enable_quiche": xs,
Expand Down
2 changes: 1 addition & 1 deletion bazel/external/quiche.BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ cc_library(
"quiche/quic/platform/api/quic_string_piece.h",
"quiche/quic/platform/api/quic_string_utils.h",
"quiche/quic/platform/api/quic_test.h",
"quiche/quic/platform/api/quic_test_output.h",
"quiche/quic/platform/api/quic_text_utils.h",
"quiche/quic/platform/api/quic_uint128.h",
"quiche/quic/platform/api/quic_thread.h",
Expand All @@ -185,7 +186,6 @@ cc_library(
# "quiche/quic/platform/api/quic_test.h",
# "quiche/quic/platform/api/quic_test_loopback.h",
# "quiche/quic/platform/api/quic_test_mem_slice_vector.h",
# "quiche/quic/platform/api/quic_test_output.h",
],
visibility = ["//visibility:public"],
deps = [
Expand Down
4 changes: 4 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ envoy_cc_library(
"quic_text_utils_impl.cc",
] + envoy_select_quiche([
"quic_hostname_utils_impl.cc",
"quic_test_output_impl.cc",
]),
hdrs = [
"quic_cert_utils_impl.h",
Expand All @@ -113,15 +114,18 @@ envoy_cc_library(
"quic_text_utils_impl.h",
] + envoy_select_quiche([
"quic_hostname_utils_impl.h",
"quic_test_output_impl.h",
]),
external_deps = [
"quiche_quic_platform_base",
"abseil_str_format",
"abseil_synchronization",
"abseil_time",
"ssl",
],
visibility = ["//visibility:public"],
deps = envoy_select_quiche([
"//source/common/filesystem:filesystem_lib",
"//source/common/http:utility_lib",
]),
)
Expand Down
73 changes: 73 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
// NOLINT(namespace-envoy)

// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "extensions/quic_listeners/quiche/platform/quic_test_output_impl.h"

#include <cstdlib>

#include "common/filesystem/filesystem_impl.h"

#include "absl/time/clock.h"
#include "absl/time/time.h"
#include "fmt/printf.h"
#include "gtest/gtest.h"
#include "quiche/quic/platform/api/quic_logging.h"

namespace quic {
namespace {

void QuicRecordTestOutputToFile(const std::string& filename, QuicStringPiece data) {
const char* output_dir_env = std::getenv("QUIC_TEST_OUTPUT_DIR");
if (output_dir_env == nullptr) {
QUIC_LOG(WARNING) << "Could not save test output since QUIC_TEST_OUTPUT_DIR is not set";
return;
}

std::string output_dir = output_dir_env;
if (output_dir.empty()) {
QUIC_LOG(WARNING) << "Could not save test output since QUIC_TEST_OUTPUT_DIR is empty";
return;
}

if (output_dir.back() != '/') {
output_dir += '/';
}

Envoy::Filesystem::InstanceImpl file_system;
Copy link
Contributor

@mpwarres mpwarres Feb 26, 2019

Choose a reason for hiding this comment

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

It would be nice if the Filesystem::Instance could be obtained from Api::Api::fileSystem(), but I can't think of a way to do that, since we don't have access to the Api::Api instance here, and there's no good way to pass it down explicitly.

What you have here looks like best immediate solution to me, but leaving this comment in case Envoy folks have any other ideas. I do wonder if this will come up in other places as well though, e.g. if other platform impls need to make use of functionality abstracted under Api::Api. Maybe we need a singleton (ick) or other means to fetch the Api::Api when it can't be passed explicitly?

Copy link
Contributor

@alyssawilk alyssawilk Feb 27, 2019

Choose a reason for hiding this comment

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

I'd love @mattklein123 take on this one.
I had a similar issue with something the server owned lately and I reaaaally wanted to make it a singleton too, but then again I love singletons about as much as Matt and Josh dislike them.

Copy link
Contributor Author

@wu-bin wu-bin Feb 27, 2019

Choose a reason for hiding this comment

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

The code in this PR is purely for tests, IMHO, making test code more testable via a mock is going a little bit too far. Does it make any sense?

Copy link
Contributor

@mpwarres mpwarres Feb 27, 2019

Choose a reason for hiding this comment

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

Fair enough, I'm fine with not using a mock and checking for the file directly.
I do think we will need a solution to the general Api::Api issue, but maybe we should deal with that separately from this PR? If we do that, we'll need to remember to circle back and update quic_test_output_impl.cc to not hardcode FilesystemImpl later on.

Copy link
Member

@mattklein123 mattklein123 Feb 27, 2019

Choose a reason for hiding this comment

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

I think we are going to have to end up plumbing an Api into this platform abstraction somehow. Can the platform abstraction have shared state that is initialized? If we have that it should be quite easy to pass in an Api. cc @jmarantz

Copy link
Contributor Author

@wu-bin wu-bin Feb 28, 2019

Choose a reason for hiding this comment

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

@mpwarres Agree we need a solution for the general Api issue, I think we can circle back when we have a concrete issue related to it. I still think quic_test_output_impl.cc can continue to write directly to real files. I found that test function TestEnvironment::writeStringToFileForTest is already doing something similar.

@mattklein123 The code in this PR is test only, all it does is to allow some unit tests to save some test output to somewhere(in this case, a file). We have a test of this test-code in quic_platform_test.cc(QuicPlatformTest.QuicTestOutput), I think the point of plumbing Api into here is to test this test-code better, which seems a little overkill?

@mattklein123 Most platform abstractions, including this one, do not have shared states. I won't be surprised if some of them end up needing a shared Api, I think we can deal with that when the need arises.

Copy link
Member

@mattklein123 mattklein123 Feb 28, 2019

Choose a reason for hiding this comment

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

If this is only used in test code, can we put it in the test directory to make that clear? Can you have a test platform split out?

Copy link
Contributor Author

@wu-bin wu-bin Feb 28, 2019

Choose a reason for hiding this comment

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

@mattklein123 I have tried moving it to the test directory previously, but gave up due to some complications. The issue is quic_test_output.h, which is part of upstream QUICHE, and built in cc_library "quic_platform_base" in bazel/external/quiche.BUILD, wants to include 'extensions/quic_listeners/quiche/platform/quic_test_output_impl.h', this path does not work if quic_test_output_impl.h is in the test directory.

Copy link
Contributor

@mpwarres mpwarres Mar 5, 2019

Choose a reason for hiding this comment

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

Re: @mattklein123's earlier question, we could add shared state to the platform implementation that is initialized early on via an Init() call, which IIUC would amount to a singleton maintained within the QUICHE platform impl. If that seems reasonable, we can add that, though I think our preference would be to do it separate from this PR, if that's ok.

if (!file_system.directoryExists(output_dir)) {
QUIC_LOG(ERROR) << "Directory does not exist while writing test output: " << output_dir;
return;
}

const std::string output_path = output_dir + filename;
Envoy::Filesystem::FilePtr file = file_system.createFile(output_path);
if (!file->open().rc_) {
QUIC_LOG(ERROR) << "Failed to open test output file: " << output_path;
return;
}

if (file->write(data).rc_ != static_cast<ssize_t>(data.size())) {
QUIC_LOG(ERROR) << "Failed to write to test output file: " << output_path;
} else {
QUIC_LOG(INFO) << "Recorded test output into " << output_path;
}

file->close();
}
} // namespace

void QuicRecordTestOutputImpl(QuicStringPiece identifier, QuicStringPiece data) {
const testing::TestInfo* test_info = testing::UnitTest::GetInstance()->current_test_info();

std::string timestamp = absl::FormatTime("%Y%m%d%H%M%S", absl::Now(), absl::LocalTimeZone());

std::string filename = fmt::sprintf("%s.%s.%s.%s.qtr", test_info->name(),
test_info->test_case_name(), identifier.data(), timestamp);

QuicRecordTestOutputToFile(filename, data);
}

} // namespace quic
15 changes: 15 additions & 0 deletions source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#pragma once

// NOLINT(namespace-envoy)
//
// This file is part of the QUICHE platform implementation, and is not to be
// consumed or referenced directly by other Envoy code. It serves purely as a
// porting layer for QUICHE.

#include "quiche/quic/platform/api/quic_string_piece.h"

namespace quic {

void QuicRecordTestOutputImpl(QuicStringPiece identifier, QuicStringPiece data);

} // namespace quic
18 changes: 18 additions & 0 deletions test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "test/extensions/transport_sockets/tls/ssl_test_utility.h"
#include "test/test_common/environment.h"
#include "test/test_common/logging.h"

#include "gmock/gmock.h"
Expand All @@ -22,6 +23,7 @@
#include "quiche/quic/platform/api/quic_stack_trace.h"
#include "quiche/quic/platform/api/quic_string.h"
#include "quiche/quic/platform/api/quic_string_piece.h"
#include "quiche/quic/platform/api/quic_test_output.h"
#include "quiche/quic/platform/api/quic_thread.h"
#include "quiche/quic/platform/api/quic_uint128.h"

Expand Down Expand Up @@ -426,6 +428,22 @@ TEST(QuicPlatformTest, QuicCertUtils) {
OPENSSL_free(static_cast<void*>(der));
}

TEST(QuicPlatformTest, QuicTestOutput) {
QuicLogThresholdSaver saver;

Envoy::TestEnvironment::setEnvVar("QUIC_TEST_OUTPUT_DIR", "/tmp", /*overwrite=*/false);

// Set log level to INFO to see the test output path in log.
quic::GetLogger().set_level(quic::INFO);

EXPECT_LOG_NOT_CONTAINS("warn", "",
quic::QuicRecordTestOutput("quic_test_output.1", "output 1 content\n"));
EXPECT_LOG_NOT_CONTAINS("error", "",
quic::QuicRecordTestOutput("quic_test_output.2", "output 2 content\n"));
EXPECT_LOG_CONTAINS("info", "Recorded test output into",
quic::QuicRecordTestOutput("quic_test_output.3", "output 3 content\n"));
}

} // namespace
} // namespace Quiche
} // namespace QuicListeners
Expand Down