Update googletest to 1.15.2 #36699

asedeno · 2024-10-18T19:53:20Z

This picks up nearly four years of changes in googletest.

googletest has updated to abseil flags¹.

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 documentation², 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 manner³.
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 re2⁴. The switch to re2 means
that . does not match newline by default. This prefixes regex that need it with (?s)
to change that behavior⁵.

Risk Level: low — test only
Testing: Updates

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]>

repokitteh-read-only · 2024-10-18T19:53:25Z

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: #36699 was opened by asedeno.

see: more, trace.

repokitteh-read-only · 2024-10-18T19:53:30Z

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: #36699 was opened by asedeno.

see: more, trace.

asedeno · 2024-10-18T19:59:28Z

cc: @phlax @adisuissa @yanavlasov
x-ref #33219

adisuissa

Thanks for pursuing this!
I cannot recall which CI test was problematic, but I hope we can get this solved in this PR.

test/fuzz/fuzz_runner.cc

adisuissa · 2024-10-18T20:24:52Z

test/integration/protocol_integration_test.cc

@@ -4769,7 +4769,8 @@ class AllowForceFail : public Api::OsSysCallsImpl {
  bool fail_ = false;
 };

-TEST_P(ProtocolIntegrationTest, HandleUpstreamSocketCreationFail) {
+TEST_P(ProtocolIntegrationDeathTest, HandleUpstreamSocketCreationFail) {
+  GTEST_FLAG_SET(death_test_style, "threadsafe");


Why is this needed here (and in some of the other tests)? I thought it is set once in the test_runner.

I did not go digging into why. All I can tell you is that anywhere I set it explicitly, the tests failed without it.

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

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

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]>

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

asedeno · 2024-10-19T03:19:16Z

Fun. Coverage tests are failing because the disk is full. From df-post:

Filesystem      Size  Used Avail Use% Mounted on                                                                                     
/dev/root        73G   73G   16M 100% /                                                                                                            
tmpfs           7.9G  172K  7.9G   1% /dev/shm                                                                                                     
tmpfs           3.2G  1.1M  3.2G   1% /run                                                                                                         
tmpfs           5.0M     0  5.0M   0% /run/lock                                                                                                    
/dev/sdb15      105M  6.1M   99M   6% /boot/efi                                                                                                    
/dev/sda1        74G  4.1G   66G   6% /mnt                                                                                                         
tmpfs           1.6G   12K  1.6G   1% /run/user/1001

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

asedeno · 2024-10-19T14:12:46Z

clang and gcc are being killed building some targets. We can iterate and keep bumping up the rbe_pools for targets one by one, but I don't like it.

phlax · 2024-10-21T11:04:35Z

Fun. Coverage tests are failing because the disk is full

hmm - i added the diskspace-hack (ie delete some of the default installed cruft before running) so i would have hoped this didnt happen - lmc again ...

clang and gcc are being killed building some targets. We can iterate and keep bumping up the rbe_pools for targets one by one, but I don't like it.

i have some tooling to identify which jobs need more mem by profiling a complete run with everything running on max mem workers, but tbh, with most of the problematic tasks already identified, doing what you are doing is probably more reliable and less time consuming

phlax · 2024-10-21T11:08:59Z

so diskspace hack is running and frees up a fair amount of space but evidently not enough

Disk space before cruft removal
Filesystem      Size  Used Avail Use% Mounted on
/dev/root        73G   58G   16G  80% /
tmpfs           7.9G  172K  7.9G   1% /dev/shm
tmpfs           3.2G  1.1M  3.2G   1% /run
tmpfs           5.0M     0  5.0M   0% /run/lock
/dev/sda15      105M  6.1M   99M   6% /boot/efi
/dev/sdb1        74G  4.1G   66G   6% /mnt
tmpfs           1.6G   12K  1.6G   1% /run/user/1001
Removing: /opt/hostedtoolcache ...
Removing: /usr/local/lib/android ...
Removing: /usr/local/.ghcup ...
Disk after cruft removal
Filesystem      Size  Used Avail Use% Mounted on
/dev/root        73G   36G   37G  50% /
tmpfs           7.9G  172K  7.9G   1% /dev/shm
tmpfs           3.2G  1.1M  3.2G   1% /run
tmpfs           5.0M     0  5.0M   0% /run/lock
/dev/sda15      105M  6.1M   99M   6% /boot/efi
/dev/sdb1        74G  4.1G   66G   6% /mnt
tmpfs           1.6G   12K  1.6G   1% /run/user/1001

i guess we just need to rm -rf even more paths to free space - i can raise a PR for that now

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

asedeno · 2024-10-22T03:27:52Z

In the last Mobile/ASAN run, //test/common:internal_engine_test failed to compile and it's already set to run on 4core.

phlax · 2024-10-22T08:42:41Z

mobile/test/common/BUILD

@@ -18,6 +18,7 @@ envoy_cc_test(
 envoy_cc_test(
    name = "internal_engine_test",
    srcs = ["internal_engine_test.cc"],
+    rbe_pool = "2core",


mobile CI doesnt use these pools

for ref here are the mobile pools:

https://envoy.cluster.engflow.com/restatus

and the envoy ones:

https://mordenite.cluster.engflow.com/restatus

also mobile is not set up to use rbe_pool so it will either break stuff or at best be ignored

it is, however, OOMing

ERROR: /source/mobile/test/common/BUILD:18:14: Compiling test/common/internal_engine_test.cc failed: (Killed): clang-14 failed: error executing command (from target //test/common:internal_engine_test)

which means there is a bigger issue in terms of mobile workers and resources - probably you will need to raise this on the related channel

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

asedeno · 2024-11-11T20:40:45Z

Still OOMing. Waiting on further resources.

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

phlax · 2024-11-15T11:42:13Z

mobile/test/common/extensions/filters/http/platform_bridge/BUILD

@@ -31,6 +31,7 @@ envoy_extension_cc_test(
        "platform_bridge_filter_integration_test.cc",
    ],
    extension_names = ["envoy.filters.http.platform_bridge"],
+    rbe_pool = "6gig",


this wont work out of the box - what it actually needs to be is:

exec_properties = {"Pool": "6gig"}

probably it makes sense to set up a macro - so that you can set it as rbe_pool - see eg

envoy/bazel/envoy_library.bzl

Lines 117 to 120 in 4c09752

exec_properties = exec_properties | select({

repository + "//bazel:engflow_rbe": {"Pool": rbe_pool} if rbe_pool else {},

"//conditions:default": {},

})

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

github-actions · 2024-12-15T20:01:15Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2024-12-23T00:04:01Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

asedeno added 7 commits October 18, 2024 15:43

Update googletest to 1.15.2

aa0f563

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

Use googletest's new(ish) FLAGS macros.

4574384

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

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Oct 18, 2024

repokitteh-read-only bot assigned mattklein123 Oct 18, 2024

adisuissa reviewed Oct 18, 2024

View reviewed changes

asedeno added 2 commits October 18, 2024 16:40

googletest: load deps from googletest

b682527

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

Use GMOCK_FLAG_SET

9c6c7b0

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

asedeno force-pushed the update_googletest branch from 0ced0af to 9c6c7b0 Compare October 18, 2024 21:11

asedeno added 3 commits October 18, 2024 19:35

buffer_accounting_integration_test.cc: move deathtest alias and insta…

41ab261

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

more resources for tests that fail to build

452d443

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

asedeno force-pushed the update_googletest branch from e30755f to 452d443 Compare October 19, 2024 02:54

more resources for tests that fail to build

7b9e1a0

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

asedeno mentioned this pull request Oct 21, 2024

Print disk usage information in ci logs #36701

Closed

asedeno added 4 commits October 21, 2024 14:51

more resources for tests that fail to build

d9f0f95

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

Merge branch 'main' into update_googletest

70380f4

more resources for tests that fail to build

aa8fd76

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

more resources for tests that fail to build

1ae9162

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

more resources for tests that fail to build

3782561

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

phlax reviewed Oct 22, 2024

View reviewed changes

asedeno added 9 commits October 22, 2024 07:34

Mobile build pool doesn't have 2core/4core

7b83911

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

more resources for tests that fail to build

5e0f84f

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

more resources for tests that fail to build

81863c8

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

Merge branch 'main' into update_googletest

a789e52

Merge branch 'main' into update_googletest

cb7205d

one more rbe_pool 2core -> 6gig

7eced89

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

Merge branch 'main' into update_googletest

6625d9b

Merge branch 'main' into update_googletest

d30def7

Merge branch 'main' into update_googletest

5588d1e

more resources for tests that fail to build

aa2b11d

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

phlax reviewed Nov 15, 2024

View reviewed changes

phlax assigned phlax and adisuissa Nov 15, 2024

asedeno added 6 commits November 15, 2024 10:20

define engflow_rbe in mobile RBE builds to make rbe_pool work

d021654

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

more resources for tests that fail to build

300cae1

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

more resources for tests that fail to build

225a29e

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

mobile rbe_pool support for linux-asan only for now

bfd66b9

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

more resources for tests that fail to build

6b081b6

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

More DeathTest updates

3d04c4a

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

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 15, 2024

github-actions bot closed this Dec 23, 2024

This was referenced May 1, 2025

deps: bump up googletest to v1.17.0 #39301

Closed

Update googletest to v1.17 #39305

Merged

Update googletest to 1.17.0 #39313

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update googletest to 1.15.2 #36699

Update googletest to 1.15.2 #36699

asedeno commented Oct 18, 2024 •

edited by phlax

Loading

repokitteh-read-only bot commented Oct 18, 2024

repokitteh-read-only bot commented Oct 18, 2024

asedeno commented Oct 18, 2024

adisuissa left a comment

adisuissa Oct 18, 2024

asedeno Oct 18, 2024

asedeno commented Oct 19, 2024

asedeno commented Oct 19, 2024

phlax commented Oct 21, 2024

phlax commented Oct 21, 2024

asedeno commented Oct 22, 2024

phlax Oct 22, 2024

phlax Oct 22, 2024

asedeno commented Nov 11, 2024

phlax Nov 15, 2024

github-actions bot commented Dec 15, 2024

github-actions bot commented Dec 23, 2024

	exec_properties = exec_properties \| select({
	repository + "//bazel:engflow_rbe": {"Pool": rbe_pool} if rbe_pool else {},
	"//conditions:default": {},
	})

Update googletest to 1.15.2 #36699

Update googletest to 1.15.2 #36699

Conversation

asedeno commented Oct 18, 2024 • edited by phlax Loading

Footnotes

repokitteh-read-only bot commented Oct 18, 2024

repokitteh-read-only bot commented Oct 18, 2024

asedeno commented Oct 18, 2024

adisuissa left a comment

Choose a reason for hiding this comment

adisuissa Oct 18, 2024

Choose a reason for hiding this comment

asedeno Oct 18, 2024

Choose a reason for hiding this comment

asedeno commented Oct 19, 2024

asedeno commented Oct 19, 2024

phlax commented Oct 21, 2024

phlax commented Oct 21, 2024

asedeno commented Oct 22, 2024

phlax Oct 22, 2024

Choose a reason for hiding this comment

phlax Oct 22, 2024

Choose a reason for hiding this comment

asedeno commented Nov 11, 2024

phlax Nov 15, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 15, 2024

github-actions bot commented Dec 23, 2024

asedeno commented Oct 18, 2024 •

edited by phlax

Loading