Skip to content
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

coverage report: lower log level to trace and drop all logs #3065

Merged
merged 7 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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: 0 additions & 1 deletion source/server/options_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const HotRestartVersionCb& hot_r
}
log_levels_string +=
fmt::format("\nDefault is [{}]", spdlog::level::level_names[default_log_level]);
log_levels_string += "\n[trace] and [debug] are only available on debug builds";

const std::string log_format_string =
fmt::format("Log message format in spdlog syntax "
Expand Down
1 change: 1 addition & 0 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ envoy_cc_test_library(
"//source/common/common:thread_lib",
"//source/common/event:libevent_lib",
"//test/test_common:environment_lib",
"//test/mocks/access_log:access_log_mocks",
"//test/test_common:printers_lib",
] + select({
"//bazel:disable_signal_trace": [],
Expand Down
9 changes: 6 additions & 3 deletions test/common/compressor/zlib_compressor_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,12 @@ class ZlibCompressorImplDeathTest : public ZlibCompressorImplTest {
* compress before init.
*/
TEST_F(ZlibCompressorImplDeathTest, CompressorTestDeath) {
EXPECT_DEATH(compressorBadInitTestHelper(100, 8), std::string{"assert failure: result >= 0"});
EXPECT_DEATH(compressorBadInitTestHelper(31, 10), std::string{"assert failure: result >= 0"});
EXPECT_DEATH(unitializedCompressorTestHelper(), std::string{"assert failure: result == Z_OK"});
EXPECT_DEATH_LOG_TO_STDERR(compressorBadInitTestHelper(100, 8),
std::string{"assert failure: result >= 0"});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can drop these std:string{} ctors. They aren't needed in some cases below.

Copy link
Member

Choose a reason for hiding this comment

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

@cpakulski do you want to clean this up? Or just merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 @jmarantz I will remove string ctors. They is probably casted to char* anyways.

EXPECT_DEATH_LOG_TO_STDERR(compressorBadInitTestHelper(31, 10),
std::string{"assert failure: result >= 0"});
EXPECT_DEATH_LOG_TO_STDERR(unitializedCompressorTestHelper(),
std::string{"assert failure: result == Z_OK"});
}

/**
Expand Down
6 changes: 4 additions & 2 deletions test/common/decompressor/zlib_decompressor_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ class ZlibDecompressorImplDeathTest : public ZlibDecompressorImplTest {
* decompress before init.
*/
TEST_F(ZlibDecompressorImplDeathTest, DecompressorTestDeath) {
EXPECT_DEATH(decompressorBadInitTestHelper(100), std::string{"assert failure: result >= 0"});
EXPECT_DEATH(unitializedDecompressorTestHelper(), std::string{"assert failure: result == Z_OK"});
EXPECT_DEATH_LOG_TO_STDERR(decompressorBadInitTestHelper(100),
std::string{"assert failure: result >= 0"});
EXPECT_DEATH_LOG_TO_STDERR(unitializedDecompressorTestHelper(),
std::string{"assert failure: result == Z_OK"});
}

/**
Expand Down
17 changes: 9 additions & 8 deletions test/common/network/address_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,9 @@ TEST(AddressFromSockAddr, IPv4) {
EXPECT_EQ(1, inet_pton(AF_INET, "1.2.3.4", &sin.sin_addr));
sin.sin_port = htons(6502);

EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in) - 1), "ss_len");
EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in) + 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, sizeof(sockaddr_in) - 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, sizeof(sockaddr_in) + 1), "ss_len");

EXPECT_EQ("1.2.3.4:6502", addressFromSockAddr(ss, sizeof(sockaddr_in))->asString());

Expand All @@ -351,9 +351,9 @@ TEST(AddressFromSockAddr, IPv6) {
EXPECT_EQ(1, inet_pton(AF_INET6, "01:023::00Ef", &sin6.sin6_addr));
sin6.sin6_port = htons(32000);

EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in6) - 1), "ss_len");
EXPECT_DEATH(addressFromSockAddr(ss, sizeof(sockaddr_in6) + 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, sizeof(sockaddr_in6) - 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, sizeof(sockaddr_in6) + 1), "ss_len");

EXPECT_EQ("[1:23::ef]:32000", addressFromSockAddr(ss, sizeof(sockaddr_in6))->asString());

Expand All @@ -374,8 +374,9 @@ TEST(AddressFromSockAddr, Pipe) {

StringUtil::strlcpy(sun.sun_path, "/some/path", sizeof sun.sun_path);

EXPECT_DEATH(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH(addressFromSockAddr(ss, offsetof(struct sockaddr_un, sun_path)), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, 1), "ss_len");
EXPECT_DEATH_LOG_TO_STDERR(addressFromSockAddr(ss, offsetof(struct sockaddr_un, sun_path)),
"ss_len");

socklen_t ss_len = offsetof(struct sockaddr_un, sun_path) + 1 + strlen(sun.sun_path);
EXPECT_EQ("/some/path", addressFromSockAddr(ss, ss_len)->asString());
Expand Down
8 changes: 4 additions & 4 deletions test/common/network/connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest,

TEST_P(ConnectionImplDeathTest, BadFd) {
Event::DispatcherImpl dispatcher;
EXPECT_DEATH(ConnectionImpl(dispatcher,
std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
Network::Test::createRawBufferSocket(), false),
".*assert failure: fd\\(\\) != -1.*");
EXPECT_DEATH_LOG_TO_STDERR(
ConnectionImpl(dispatcher, std::make_unique<ConnectionSocketImpl>(-1, nullptr, nullptr),
Network::Test::createRawBufferSocket(), false),
".*assert failure: fd\\(\\) != -1.*");
}

class ConnectionImplTest : public testing::TestWithParam<Address::IpVersion> {
Expand Down
2 changes: 1 addition & 1 deletion test/common/network/listener_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ListenerImplDeathTest,
TestUtility::ipTestParamsToString);

TEST_P(ListenerImplDeathTest, ErrorCallback) {
EXPECT_DEATH(errorCallbackTest(GetParam()), ".*listener accept failure.*");
EXPECT_DEATH_LOG_TO_STDERR(errorCallbackTest(GetParam()), ".*listener accept failure.*");
}

class TestListenerImpl : public ListenerImpl {
Expand Down
1 change: 1 addition & 0 deletions test/common/singleton/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ envoy_cc_test(
srcs = ["manager_impl_test.cc"],
deps = [
"//source/common/singleton:manager_impl_lib",
"//test/test_common:utility_lib",
],
)

Expand Down
5 changes: 4 additions & 1 deletion test/common/singleton/manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include "common/singleton/manager_impl.h"

#include "test/test_common/utility.h"

#include "gmock/gmock.h"

namespace Envoy {
Expand All @@ -14,7 +16,8 @@ static void deathTestWorker() {
}

TEST(SingletonManagerImplDeathTest, NotRegistered) {
EXPECT_DEATH(deathTestWorker(), "invalid singleton name 'foo'. Make sure it is registered.");
EXPECT_DEATH_LOG_TO_STDERR(deathTestWorker(),
"invalid singleton name 'foo'. Make sure it is registered.");
}

static constexpr char test_singleton_name[] = "test_singleton";
Expand Down
5 changes: 4 additions & 1 deletion test/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,8 @@ envoy_cc_test(
name = "signals_test",
srcs = ["signals_test.cc"],
tags = ["backtrace"],
deps = ["//source/exe:sigaction_lib"],
deps = [
"//source/exe:sigaction_lib",
"//test/test_common:utility_lib",
],
)
14 changes: 8 additions & 6 deletions test/exe/signals_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "exe/signal_action.h"

#include "test/test_common/utility.h"

#include "gtest/gtest.h"

namespace Envoy {
Expand All @@ -23,7 +25,7 @@ namespace Envoy {
#ifndef ASANITIZED
TEST(Signals, InvalidAddressDeathTest) {
SignalAction actions;
EXPECT_DEATH(
EXPECT_DEATH_LOG_TO_STDERR(
[]() -> void {
// Oooooops!
volatile int* nasty_ptr = reinterpret_cast<int*>(0x0);
Expand All @@ -34,7 +36,7 @@ TEST(Signals, InvalidAddressDeathTest) {

TEST(Signals, BusDeathTest) {
SignalAction actions;
EXPECT_DEATH(
EXPECT_DEATH_LOG_TO_STDERR(
[]() -> void {
// Bus error is tricky. There's one way that can work on POSIX systems
// described below but it depends on mmaping a file. Just make it easy and
Expand All @@ -50,7 +52,7 @@ TEST(Signals, BusDeathTest) {

TEST(Signals, BadMathDeathTest) {
SignalAction actions;
EXPECT_DEATH(
EXPECT_DEATH_LOG_TO_STDERR(
[]() -> void {
// It turns out to be really hard to not have the optimizer get rid of a
// division by zero. Just raise the signal for this test.
Expand All @@ -63,7 +65,7 @@ TEST(Signals, BadMathDeathTest) {
// Unfortunately we don't have a reliable way to do this on other platforms
TEST(Signals, IllegalInstructionDeathTest) {
SignalAction actions;
EXPECT_DEATH(
EXPECT_DEATH_LOG_TO_STDERR(
[]() -> void {
// Intel defines the "ud2" opcode to be an invalid instruction:
__asm__("ud2");
Expand All @@ -74,7 +76,7 @@ TEST(Signals, IllegalInstructionDeathTest) {

TEST(Signals, AbortDeathTest) {
SignalAction actions;
EXPECT_DEATH([]() -> void { abort(); }(), "backtrace.*Abort(ed)?");
EXPECT_DEATH_LOG_TO_STDERR([]() -> void { abort(); }(), "backtrace.*Abort(ed)?");
}

TEST(Signals, RestoredPreviousHandlerDeathTest) {
Expand All @@ -86,7 +88,7 @@ TEST(Signals, RestoredPreviousHandlerDeathTest) {
// goes out of scope, NOT the default.
}
// Outer SignalAction should be active again:
EXPECT_DEATH([]() -> void { abort(); }(), "backtrace.*Abort(ed)?");
EXPECT_DEATH_LOG_TO_STDERR([]() -> void { abort(); }(), "backtrace.*Abort(ed)?");
}
#endif

Expand Down
3 changes: 2 additions & 1 deletion test/run_envoy_bazel_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ BAZEL_TEST_OPTIONS="${BAZEL_TEST_OPTIONS} -c dbg --copt=-DNDEBUG"
"${BAZEL_COVERAGE}" --batch test "${COVERAGE_TARGET}" ${BAZEL_TEST_OPTIONS} \
--cache_test_results=no --cxxopt="--coverage" --cxxopt="-DENVOY_CONFIG_COVERAGE=1" \
--linkopt="--coverage" --define ENVOY_CONFIG_COVERAGE=1 --test_output=streamed \
--strategy=Genrule=standalone --spawn_strategy=standalone --test_timeout=1000
--strategy=Genrule=standalone --spawn_strategy=standalone --test_timeout=1000 \
--test_arg="--log-path /dev/null" --test_arg="-l trace"

# The Bazel build has a lot of whack in it, in particular generated files, headers from external
# deps, etc. So, we exclude this from gcov to avoid false reporting of these files in the html and
Expand Down
16 changes: 16 additions & 0 deletions test/test_common/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,22 @@ namespace Envoy {
ADD_FAILURE() << "Unexpected exception: " << std::string(e.what()); \
}

/*
Macro to use instead of EXPECT_DEATH when stderr is produced by a logger.
It temporarily installs stderr sink and restores the original logger sink after the test
completes and sdterr_sink object goes of of scope.
EXPECT_DEATH(statement, regex) test passes when statement causes crash and produces error message
matching regex. Test fails when statement does not crash or it crashes but message does not
match regex. If a message produced during crash is redirected away from strerr, the test fails.
By installing StderrSinkDelegate, the macro forces EXPECT_DEATH to send any output produced by
statement to stderr.
*/
#define EXPECT_DEATH_LOG_TO_STDERR(statement, message) \
do { \
Logger::StderrSinkDelegate stderr_sink(Logger::Registry::getSink()); \
EXPECT_DEATH(statement, message); \
} while (false)

// Random number generator which logs its seed to stderr. To repeat a test run with a non-zero seed
// one can run the test with --test_arg=--gtest_random_seed=[seed]
class TestRandomGenerator {
Expand Down
10 changes: 10 additions & 0 deletions test/test_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "common/common/thread.h"
#include "common/event/libevent.h"

#include "test/mocks/access_log/mocks.h"
#include "test/test_common/environment.h"

#include "gmock/gmock.h"
Expand Down Expand Up @@ -31,6 +32,15 @@ class TestRunner {
Logger::Registry::initialize(TestEnvironment::getOptions().logLevel(),
TestEnvironment::getOptions().logFormat(), lock);

// Allocate fake log access manager.
testing::NiceMock<AccessLog::MockAccessLogManager> access_log_manager;
std::unique_ptr<Logger::FileSinkDelegate> file_logger;

// Redirect all logs to fake file when --log-path arg is specified in command line.
if (!TestEnvironment::getOptions().logPath().empty()) {
file_logger = std::make_unique<Logger::FileSinkDelegate>(
TestEnvironment::getOptions().logPath(), access_log_manager, Logger::Registry::getSink());
}
return RUN_ALL_TESTS();
}
};
Expand Down