Skip to content

Update googletest to 1.17.0 #39313

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

asedeno
Copy link
Contributor

@asedeno asedeno commented May 4, 2025

This picks up about 4.5 years of changes in googletest, and attempts to revive and clean up #36699, pulling in some changes from #39301 / #39305.

googletest has updated to abseil flags1.

The most notable difference is that unrecognized flags result in a flag parsing error,
and are not returned to the user though a modified argc/argv, unless they appear
after the positional argument delimiter ("--").

For example, to pass a non-Abseil flag, you would have to do
./mytest --gtest_color=false -- --myflag=myvalue

After this upgrade, to set Envoy's CLI arguments in tests requires passing them after --.
For example,

bazel test //test/common/common:logger_test --test_arg="--" --test_arg="-l trace"

Per googletest documentation2, it is strongly recommended that death tests
have names ending in DeathTest rather than simply Test.

Some death tests behave better when run in a threadsafe manner3.
This is a trade-off that costs speed. The flag has been set in tests where it made a difference.

When building with bazel and abseil, googletest uses re24. The switch to re2 means
that . does not match newline by default. This prefixes regex that need it with (?s)
to change that behavior5.

Risk Level: low — test only
Testing: Updates

Footnotes

  1. https://github.com/google/googletest/commit/25dcdc7e8bfac8967f20fb2c0a628f5cf442188d

  2. https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-naming

  3. https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-styles

  4. https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-styles

  5. https://github.com/google/re2/wiki/syntax#flags

asedeno and others added 13 commits May 3, 2025 22:43
googletest patch: strip fuchsia out of BUILD.bazel

pulling in the fake @fuchsia_sdk from googletest left it not marked
for test only, which made the envoy CI deps check fail. Stripping it
out is a very small and simple patch, so let's do that instead.

Signed-off-by: Alejandro R. Sedeño <[email protected]>
google/googletest@25dcdc7

"""
The most notable difference is that unrecognized flags result
in a flag parsing error, and are not returned to the user though
a modified argc/argv, unless they appear after the positional
argument delimiter ("--").

For example, to pass a non-Abseil flag, you would have to do
./mytest --gtest_color=false -- --myflag=myvalue
"""

Signed-off-by: Alejandro R. Sedeño <[email protected]>
Signed-off-by: Alejandro R. Sedeño <[email protected]>
Per googletest documentation[^1], it is strongly recommended that
death tests have names ending in `DeathTest` rather than simply
`Test`. This makes those changes.

Some death tests behave better when run in a threadsafe
manner[^2]. This is a trade-off that costs speed. The flag has been
set in tests where it made a difference.

[^1]: https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-naming
[^2]: https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-styles

Signed-off-by: Alejandro R. Sedeño <[email protected]>
When building with bazel and abseil, googletest uses re2[^1]. The
switch to re2 means that `.` does not match newline by default. This
prefixes regex that need it with `(?s)` to change that behavior[^2].

[^1]: https://github.com/google/googletest/blob/main/docs/advanced.md#death-test-styles
[^2]: https://github.com/google/re2/wiki/syntax#flags

Signed-off-by: Alejandro R. Sedeño <[email protected]>
googletest has switched from custom-rolled heterogeneous comparator
functors to C++ standard ones (e.g., `std::equal_to<>`). The version
of clang we're currently using in docker finds two equally good
`operator==` overloads here, in which a cast for one operand or the
other is necessary, and having not found one unambiguously better
operator, fails to build. This has been resolved in newer versions of
clang.

Since the relevant `operator==` already does its own dynamic casting,
drop the `WhenDynamicCastTo<>` from this one `EXPECT_CALL` so it'll
build again.

Signed-off-by: Alejandro R. Sedeño <[email protected]>
This moves setting `--gmock_default_mock_behavior=2` to the head of
`args`, which makes sense in light of the googletest/absel flag
changes. This ensures that this googletest flag appears before any
`--` separating googletest flags from envoy/test flags.

Signed-off-by: Alejandro R. Sedeño <[email protected]>
Signed-off-by: Alejandro R. Sedeño <[email protected]>
…ntiation into #ifdef

Signed-off-by: Alejandro R. Sedeño <[email protected]>
Signed-off-by: Alejandro R. Sedeño <[email protected]>
Signed-off-by: Yan Avlasov <[email protected]>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #39313 was opened by asedeno.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 4, 2025
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #39313 was opened by asedeno.

see: more, trace.

@asedeno
Copy link
Contributor Author

asedeno commented May 4, 2025

I'm jumping on this bandwagon to see if my old changes plus a couple of new ones are in a decent place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps Approval required for changes to Envoy's external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants