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.

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

Conversation

adisuissa
Copy link
Contributor

Commit Message: googletest: upgrade to a newer version
Additional Description:
Upgrading googletest dependency to a newer version (3 years forward).
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"

Risk Level: low - tests only
Testing: Updates
Docs Changes:
Release Notes: N/A
Platform Specific Features: N/A

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: #33219 was opened by adisuissa.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Mar 29, 2024
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 @moderation

🐱

Caused by: #33219 was opened by adisuissa.

see: more, trace.

# 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.

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).

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

Copy link

github-actions bot commented May 1, 2024

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 github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 1, 2024
@phlax
Copy link
Member

phlax commented May 1, 2024

@adisuissa wondering what the blocker was on this - and thinking to no-stalebot if its not immediately resolvable - this dep hasnt been updated in a very long time

@adisuissa
Copy link
Contributor Author

@adisuissa wondering what the blocker was on this - and thinking to no-stalebot if its not immediately resolvable - this dep hasnt been updated in a very long time

Apologies, as I didn't have enough bandwidth to follow up on this.
The main issue was broken gcc builds. I'll try to follow up on this next week (I should get more time).
Feel free to reassign, if you have more bandwidth at the moment.

@phlax phlax added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels May 1, 2024
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 no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants