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

Update to gtest 1.11 #21

Merged
merged 684 commits into from
Jun 27, 2023
Merged

Update to gtest 1.11 #21

merged 684 commits into from
Jun 27, 2023

Conversation

clalancette
Copy link

This matches the version that is in Ubuntu 22.04, and it should fix some warnings when building with Clang.

Draft for now while I get some feedback on it.

suertreus and others added 30 commits September 29, 2020 20:47
…be_unintialized

PiperOrigin-RevId: 334391149
Update comment to suggest using SetUpTestSuite and TearDownTestSuite.

PiperOrigin-RevId: 334430329
Fix undefined pointer comparison

PiperOrigin-RevId: 334436975
Update faq.md on underscore to mention `DISABLED_` prefix.

PiperOrigin-RevId: 334507963
…k from A1, A2, ... to TArg1, TArg2,... to avoid clash with legacy header files
Make the code Python3 compliant.

PiperOrigin-RevId: 336144198
Improve lookup of operator<< for user types

Without this fix, trying to use this class with googletest

  struct Foo {};

  template <typename OutputStream>
  OutputStream& operator<<(OutputStream& os, const Foo&) {
    os << "TemplatedStreamableInFoo";
    return os;
  }

results in an ambiguity error between the class' operator<< and the
operator<< in gtest-printers.h removed in this CL.

This fix also enables implicit conversions to happen, so that e.g.
we will find the base class operator<< if a subclass has no
operator<< of its own.

PiperOrigin-RevId: 336261221
Add helper methos to internal FlatTuple. Refactor constructors.

PiperOrigin-RevId: 336306681
Suggest using generic lambdas for composing macros.

Long chains of macros hurt legibility; generic lambdas are an easy way to abbreviate them, but are not an obvious solution to casual users.

Compare:
EXPECT_THAT(f(), ElementsAre(
    Property(&MyClass::foo, Property(&OtherClass::bar, Contains("x"))),
    Property(&MyClass::foo, Property(&OtherClass::bar, Contains("y"))));
to:
EXPECT_THAT(f(), ElementsAre(HasFooBar("x"), HasFooBar("y")));
PiperOrigin-RevId: 336870137
Use absl::StrCat in MATCHER_P example for consistency with https://abseil.io/tips/3

PiperOrigin-RevId: 336878481
Fixes AppVeyor by upgrading to Bazel 3.6.0

PiperOrigin-RevId: 336887434
Fix -Wmismatched-tags error with struct tuple_size vs class tuple_size

PiperOrigin-RevId: 336930166
Prefer using over typedef.

PiperOrigin-RevId: 337080404
Disable -Wmismatched-tags warning for struct/class tuple_size

PiperOrigin-RevId: 337087493
Removing a semicolon that triggers a lint error in sample code.

PiperOrigin-RevId: 337095451
Stop using master.zip to make the build reproducible

PiperOrigin-RevId: 337102716
Disable warnings on code that intentionally tests a suboptimal syntax

PiperOrigin-RevId: 337138442
Add ::testing::FieldsAre matcher for objects that support get<> and structured bindings.
PiperOrigin-RevId: 337165285
…plate-args-AX-to-TArgX

PiperOrigin-RevId: 337217118
Fixes build warnings from previous CL
Add CMake to internal presubmit to prevent these

PiperOrigin-RevId: 337325504
Fix some issues when running fuse_gmock_files.

The module path should be updated before importing `fuse_gtest_files`, since
the script may not run from the googletest repo root. We also need a non-frozen
set in order to track progress.

PiperOrigin-RevId: 337380466
Fix typo in the "Assertion Placement" section

PiperOrigin-RevId: 337435223
Replace "definedin in" by "defined in" in files:
- googletest/src/gtest.cc
- googletest/test/googletest-output-test-golden-lin.txt
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Author

clalancette commented Nov 7, 2022

CI just to see if things are happy with this:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Author

Another try:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

This updating is holding back MoveIt2 from using -Wpedantic. I can confirm that this works and lets us finally enable that warning.

@clalancette
Copy link
Author

Unfortunately it is still causing CI to be yellow, and I haven't had time to go back and look at why. If you have some time to look at that and propose possible fixes, that would be helpful.

@ChrisThrasher
Copy link

https://ci.ros2.org/job/ci_linux/17846/gcc/new/source.3769323b-695d-4413-a06b-0a8173357ff9/#316

The error seems to be that there's some new code in v1.11 that triggers -Wmaybe-uninitialized. I think this was fixed in v1.12. Because GTest headers should be treated as system headers and thus no warnings get emitted in them, is this something we need to worry about?

@clalancette
Copy link
Author

The error seems to be that there's some new code in v1.11 that triggers -Wmaybe-uninitialized. I think this was fixed in v1.12. Because GTest headers should be treated as system headers and thus no warnings get emitted in them, is this something we need to worry about?

You are right, nowadays we are treating them as system headers, so this probably isn't a problem anymore.

That said, this change is not compiling anymore locally for me. I had to upgrade a bunch of our packages to C++17 (which we honestly should have done a while ago). I'm slowly getting those PRs in, but this will take some time. Once we have those in, I'll rebase this and try CI again.

@ChrisThrasher
Copy link

That said, this change is not compiling anymore locally for me. I had to upgrade a bunch of our packages to C++17

Did this update accidentally start using C++17 features? The latest release only claims to require C++14. What specifically fails to compile for you?

@clalancette
Copy link
Author

What specifically fails to compile for you?

When I go to compile this PR against the rest of the ROS 2 core, rcutils fails to build:

$ colcon build --event-handlers console_direct+ --packages-up-to rcutils
...
Starting >>> rcutils
...
/usr/bin/ld: CMakeFiles/test_allocator.dir/test/test_allocator.cpp.o: in function `TestAllocatorFixture_test_default_allocator_normal_Test::TestBody()':
test_allocator.cpp:(.text+0x38c): undefined reference to `osrf_testing_tools_cpp::memory_tools::on_unexpected_malloc(std::variant<std::function<void (osrf_testing_tools_cpp::memory_tools::MemoryToolsService&)>, std::function<void ()>, decltype(nullptr)>)'
/usr/bin/ld: test_allocator.cpp:(.text+0x3c2): undefined reference to `osrf_testing_tools_cpp::memory_tools::on_unexpected_realloc(std::variant<std::function<void (osrf_testing_tools_cpp::memory_tools::MemoryToolsService&)>, std::function<void ()>, decltype(nullptr)>)'
/usr/bin/ld: test_allocator.cpp:(.text+0x3f8): undefined reference to `osrf_testing_tools_cpp::memory_tools::on_unexpected_calloc(std::variant<std::function<void (osrf_testing_tools_cpp::memory_tools::MemoryToolsService&)>, std::function<void ()>, decltype(nullptr)>)'
/usr/bin/ld: test_allocator.cpp:(.text+0x42e): undefined reference to `osrf_testing_tools_cpp::memory_tools::on_unexpected_free(std::variant<std::function<void (osrf_testing_tools_cpp::memory_tools::MemoryToolsService&)>, std::function<void ()>, decltype(nullptr)>)'
collect2: error: ld returned 1 exit status
gmake[2]: *** [CMakeFiles/test_allocator.dir/build.make:101: test_allocator] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:455: CMakeFiles/test_allocator.dir/all] Error 2

@ChrisThrasher
Copy link

ChrisThrasher commented Feb 1, 2023

osrf_testing_tools_cpp is using std::variant from C++17 but isn't specifying that it requires C++17. That may be related to this error you're getting although I'd expect that to manifest at compile time, not link time.

https://github.com/osrf/osrf_testing_tools_cpp/blob/master/osrf_testing_tools_cpp/CMakeLists.txt#L9

@clalancette
Copy link
Author

OK, we've now updated osrf_testing_tools_cpp to use C++17. Let's give CI another shot here:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

A few things:

  1. This needs a rebase or merge to take changes to the package manifest from rolling
  2. The CMakeLists.txt.upstream should be updated within the googletest and googlemock subdirectories - this appears to have been lost in the merge.
  3. The version snapshot list in the package manifests immediately prior to the version number should be updated with the new google/googletest ref

@cottsay
Copy link

cottsay commented Apr 6, 2023

I'm not sure why, but GitHub failed to back-link from the other PR.

This is the bug fix in osrf_testing_tools_cpp for the variant issue: osrf/osrf_testing_tools_cpp#77

@clalancette
Copy link
Author

clalancette commented Jun 23, 2023

I think I've now fixed all of the issues pointed out by @cottsay. With the other fixes that are in place, let's see how CI does now:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Author

All right, with ros2/rclcpp#2227 now merged, I'm going to mark this PR as ready for review and run another CI on it.

@clalancette clalancette marked this pull request as ready for review June 26, 2023 17:00
@clalancette
Copy link
Author

clalancette commented Jun 26, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link

@cottsay cottsay left a comment

Choose a reason for hiding this comment

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

I reviewed the diff and it aligns with my attempt to do the same.

Thanks!

@clalancette
Copy link
Author

CI is green, and this is approved, so merging. Note that I am going to create a merge commit here.

@clalancette clalancette merged commit ad343ef into rolling Jun 27, 2023
@clalancette clalancette deleted the clalancette/update-to-gtest-1.11 branch June 27, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Current Gmock generates a lot of warning on Ubuntu 22.04 with Clang