Skip to content

googletest: upgrade to a newer version #33219

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

googletest: upgrade to a newer version #33219

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
2 changes: 1 addition & 1 deletion bazel/envoy_test.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ def envoy_cc_test(
] + envoy_pch_deps(repository, "//test:test_pch"),
# from https://github.com/google/googletest/blob/6e1970e2376c14bf658eb88f655a054030353f9f/googlemock/src/gmock.cc#L51
# 2 - by default, mocks act as StrictMocks.
args = args + ["--gmock_default_mock_behavior=2"],
args = ["--gmock_default_mock_behavior=2"] + args,
tags = coverage_tags,
local = local,
shard_count = shard_count,
Expand Down
13 changes: 0 additions & 13 deletions bazel/googletest.patch
View file
Open in desktop

This file was deleted.

6 changes: 1 addition & 5 deletions bazel/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,11 +834,7 @@ def _com_github_ncopa_suexec():
)

def _com_google_googletest():
external_http_archive(
"com_google_googletest",
patches = ["@envoy//bazel:googletest.patch"],
patch_args = ["-p1"],
)
external_http_archive("com_google_googletest")
native.bind(
name = "googletest",
actual = "@com_google_googletest//:gtest",
Expand Down
8 changes: 3 additions & 5 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,13 +913,11 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "Google Test",
project_desc = "Google's C++ test framework",
project_url = "https://github.com/google/googletest",
# Pick up fix for MOCK_METHOD compilation with clang-cl for Windows (resolved after 1.10.0)
# see https://github.com/google/googletest/issues/2490
version = "a4ab0abb93620ce26efad9de9296b73b16e88588",
sha256 = "7897bfaa5ad39a479177cfb5c3ce010184dbaee22a7c3727b212282871918751",
version = "7e33b6a1c497ced1e98fc60175aeb4678419281c",
Copy link
Member

Choose a reason for hiding this comment

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

can we pin to a release version please?

Copy link
Contributor

Choose a reason for hiding this comment

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

1.14.0 is the latest - https://github.com/google/googletest/releases/tag/v1.14.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can pin to a release version, but unfortunately not the latest (requires new absl and protobuf versions).

sha256 = "f70077cd1f90d2b901dbe0cf04b035b8ee33b8941965ec25e145216f6267ebca",
strip_prefix = "googletest-{version}",
urls = ["https://github.com/google/googletest/archive/{version}.tar.gz"],
release_date = "2020-09-10",
release_date = "2023-08-10",
use_category = ["test_only"],
cpe = "cpe:2.3:a:google:google_test:*",
license = "BSD-3-Clause",
Expand Down
4 changes: 4 additions & 0 deletions source/common/network/address_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ class Ipv4Instance : public InstanceBase {
socklen_t sockAddrLen() const override { return sizeof(sockaddr_in); }
absl::string_view addressType() const override { return "default"; }

bool operator==(const Ipv4Instance& rhs) const {
return operator==(static_cast<const Instance&>(rhs));
}

/**
* Convenience function to convert an IPv4 address to canonical string format.
* @note This works similarly to inet_ntop() but is faster.
Expand Down
2 changes: 2 additions & 0 deletions test/common/runtime/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ envoy_cc_test(
name = "runtime_flag_override_test",
srcs = ["runtime_flag_override_test.cc"],
args = [
"--",
"--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_false",
"--runtime-feature-disable-for-tests=envoy.reloadable_features.test_feature_true",
],
Expand All @@ -75,6 +76,7 @@ envoy_cc_test(
name = "runtime_flag_override_noop_test",
srcs = ["runtime_flag_override_noop_test.cc"],
args = [
"--",
"--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_true",
"--runtime-feature-disable-for-tests=envoy.reloadable_features.test_feature_false",
],
Expand Down
5 changes: 4 additions & 1 deletion test/extensions/filters/http/jwt_authn/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ envoy_extension_cc_test(
envoy_extension_cc_test(
name = "extractor_runtime_guard_test",
srcs = ["extractor_runtime_guard_test.cc"],
args = ["--runtime-feature-disable-for-tests=envoy.reloadable_features.token_passed_entirely"],
args = [
"--",
"--runtime-feature-disable-for-tests=envoy.reloadable_features.token_passed_entirely",
],
extension_names = ["envoy.filters.http.jwt_authn"],
deps = [
"//source/extensions/filters/http/jwt_authn:extractor_lib",
Expand Down
13 changes: 7 additions & 6 deletions test/extensions/filters/http/kill_request/crash_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,10 @@ TEST_P(CrashIntegrationTestAllProtocols, UnwindsTrackedObjectStack) {
// - ActiveStream
// - Http(1|2)::ConnectionImpl
// - Network::ConnectionImpl
const std::string death_string = GetParam().downstream_protocol == Http::CodecType::HTTP2
? "ActiveStream.*Http2::ConnectionImpl.*ConnectionImpl"
: "ActiveStream.*Http1::ConnectionImpl.*ConnectionImpl";
const std::string death_string =
GetParam().downstream_protocol == Http::CodecType::HTTP2
? "ActiveStream(.|\n)*Http2::ConnectionImpl(.|\n)*ConnectionImpl"
: "ActiveStream(.|\n)*Http1::ConnectionImpl(.|\n)*ConnectionImpl";
EXPECT_DEATH(sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 1024),
death_string);
}
Expand All @@ -93,7 +94,7 @@ TEST_P(CrashIntegrationTestAllProtocols, ResponseCrashDumpsTheCorrespondingReque
// Check that we dump the downstream request
EXPECT_DEATH(
sendRequestAndWaitForResponse(default_request_headers_, 0, kill_response_headers, 1024),
"Dumping corresponding downstream request.*UpstreamRequest.*request_headers:");
"Dumping corresponding downstream request(.|\n)*UpstreamRequest(.|\n)*request_headers:");
}

TEST_P(CrashIntegrationTestAllProtocols, DecodeContinueDoesNotAddTrackedObjectIfExists) {
Expand Down Expand Up @@ -164,7 +165,7 @@ TEST_P(CrashIntegrationTestAllProtocols, DecodeContinueAddsCrashContextIfNoneExi
// - ActiveStream
// - Network::ConnectionImpl
EXPECT_DEATH(sendRequestAndWaitForResponse(request_headers, 0, default_response_headers_, 1024),
"ActiveStream.*.*ConnectionImpl");
"ActiveStream(.|\n)*ConnectionImpl");
}

TEST_P(CrashIntegrationTestAllProtocols, EncodeContinueDoesNotAddTrackedObjectIfExists) {
Expand Down Expand Up @@ -231,7 +232,7 @@ TEST_P(CrashIntegrationTestAllProtocols, EncodeContinueAddsCrashContextIfNoneExi
// - ActiveStream
// - Network::ConnectionImpl
EXPECT_DEATH(sendRequestAndWaitForResponse(default_request_headers_, 0, kill_response_headers, 0),
"ActiveStream.*.*ConnectionImpl");
"ActiveStream(.|\n)*ConnectionImpl");
}

#endif
Expand Down
2 changes: 1 addition & 1 deletion test/fuzz/fuzz_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ void runCleanupHooks() {
extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv) {
// Before parsing gmock flags, set the default value of flag --gmock_verbose to "error".
// This suppresses logs from NiceMock objects, which can be noisy and provide little value.
testing::GMOCK_FLAG(verbose) = "error";
GMOCK_FLAG_SET(verbose, "error");
testing::InitGoogleMock(argc, *argv);
Envoy::Fuzz::Runner::setupEnvironment(1, *argv, spdlog::level::critical);
atexit(Envoy::Fuzz::runCleanupHooks);
Expand Down
13 changes: 6 additions & 7 deletions test/integration/buffer_accounting_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1620,11 +1620,10 @@ TEST_P(Http2DeferredProcessingIntegrationTest, CanDumpCrashInformationWhenProces
if (!deferProcessingBackedUpStreams()) {
return;
}

EXPECT_DEATH(testCrashDumpWhenProcessingBufferedData(),
"Crashing as request body over 1000!.*"
"ActiveStream.*Http2::ConnectionImpl.*Dumping current stream.*"
"ConnectionImpl::StreamImpl.*ConnectionImpl");
"Crashing as request body over 1000!(.|\n)*"
"ActiveStream(.|\n)*Http2::ConnectionImpl(.|\n)*Dumping current stream(.|\n)*"
"ConnectionImpl::StreamImpl(.|\n)*ConnectionImpl");
}

TEST_P(Http2DeferredProcessingIntegrationTest,
Expand All @@ -1635,9 +1634,9 @@ TEST_P(Http2DeferredProcessingIntegrationTest,
return;
}
EXPECT_DEATH(testCrashDumpWhenProcessingBufferedDataOfDeferredCloseStream(),
"Crashing as response body over 1000!.*"
"ActiveStream.*Http2::ConnectionImpl.*Dumping 1 Active Streams.*"
"ConnectionImpl::StreamImpl.*ConnectionImpl");
"Crashing as response body over 1000!(.|\n)*"
"ActiveStream(.|\n)*Http2::ConnectionImpl(.|\n)*Dumping 1 Active Streams(.|\n)*"
"ConnectionImpl::StreamImpl(.|\n)*ConnectionImpl");
}

#endif
Expand Down
5 changes: 2 additions & 3 deletions test/test_common/test_random_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

#include "gtest/gtest.h"

using testing::GTEST_FLAG(random_seed);

namespace Envoy {

// The purpose of using the static seed here is to use --test_arg=--gtest_random_seed=[seed]
Expand All @@ -16,7 +14,8 @@ int32_t getSeed() {
}

TestRandomGenerator::TestRandomGenerator()
: seed_(GTEST_FLAG(random_seed) == 0 ? getSeed() : GTEST_FLAG(random_seed)), generator_(seed_) {
: seed_(GTEST_FLAG_GET(random_seed) == 0 ? getSeed() : GTEST_FLAG_GET(random_seed)),
generator_(seed_) {
ENVOY_LOG_MISC(info, "TestRandomGenerator running with seed {}", seed_);
}

Expand Down
9 changes: 5 additions & 4 deletions test/test_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ bool isDeathTestChild(int argc, char** argv) {

int TestRunner::runTests(int argc, char** argv) {
const bool is_death_test_child = isDeathTestChild(argc, argv);

// Use the recommended, but not default, "threadsafe" style for the Death Tests.
Copy link
Member

Choose a reason for hiding this comment

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

this comment (ie the original) is not entirely accurate

the threadsafe mode is not default (or recommended) as it makes testing much slower - not sure if we can do anything about that, so perhaps this is a pedantic point - but if there was a way not to do this it would be better

// See: https://github.com/google/googletest/commit/84ec2e0365d791e4ebc7ec249f09078fb5ab6caa
GTEST_FLAG_SET(death_test_style, "threadsafe");

::testing::InitGoogleMock(&argc, argv);
// We hold on to process_wide to provide RAII cleanup of process-wide
// state.
Expand All @@ -88,10 +93,6 @@ int TestRunner::runTests(int argc, char** argv) {
::testing::TestEventListeners& listeners = ::testing::UnitTest::GetInstance()->listeners();
listeners.Append(new TestListener);

// Use the recommended, but not default, "threadsafe" style for the Death Tests.
// See: https://github.com/google/googletest/commit/84ec2e0365d791e4ebc7ec249f09078fb5ab6caa
::testing::FLAGS_gtest_death_test_style = "threadsafe";

// Set gtest properties
// (https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#logging-additional-information),
// they are available in the test XML.
Expand Down
Loading