Update googletest to the version with GTEST_SKIP

[pbaughman]

We would like to use GTEST_SKIP, introduced in October 2018 in some ROS2 gtests. The ament googletest fork looks like it was was taken from 1b07766 which is about 7 months before version 1.8.1 but doesn't appear to correspond to a tagged release

To make this PR, I rebased the osrf version of googletest onto commit 00938b2. This keeps the four OSRF commits that turn this into a ros2 package at the head. I'm able to rebuild ros2 crystal with this version of googletest and can still run all the tests

I'm not sure what the policy or process is to update ament/googletest, but @wjwwood suggested opening a PR to kick off a discussion.

Let me know if there's a better way to do this or if you'd prefer something different.

[pbaughman]

On closer inspection, this version adds the GTEST_SKIP macro, but does not indicate the test was skipped in the resulting XML. . . I know that gtest master gets the XML right, but updating to that gives a whole bunch of deprecation warnings too when I build. I will search a little harder to see if there's as version that gets the XML working, but without depreciation warnings

Edit Ok, it looks like we would need to rebase onto https://github.com/google/googletest/commit/c6cb7e033591528a5fe2c63157a0d8ce927740dc to get the right XML output. That commit was December 13 2018. Unfortunately, it looks like they messed with it again on January 3 2019 and again on March 22 2019 so it's not the most stable thing. I worry about breaking colcon test-result now. . .

Summary

If I rebase onto 00938b2b228f3b70d3d9e51f29a1505bdad43f1e, then the resulting XML for a skipped test looks like this and you can't tell the test was skipped. This is a non-starter:

<testcase name="frob_a_foo" status="run" time="0.01" classname="test_foo" /> 

If I rebase onto c6cb7e033591528a5fe2c63157a0d8ce927740dc, then the resulting XML for a skipped test looks like this:

<testcase name="frob_a_foo" status="skipped" time="0.01" classname="test_foo" />

If I rebase onto 3a460a26b7a91abf87af7f31b93d29f930e25c82 (which looks like a whitespace-only change for the part we care about), then the resulting XML for a skipped test looks the same:

<testcase name="frob_a_foo" status="skipped" time="0.011" classname="test_foo" />

Finally, if I rebase onto 5b752b1947bbb4df571848a1afad00f9b06f30e0, I start to get warnings about things being deprecated:

~/ws/install/src/gtest_vendor/include/gtest/gtest-typed-test.h:230:38: warning: ‘constexpr bool
testing::internal::TypedTestCaseIsDeprecated()’ is deprecated: TYPED_TEST_CASE is deprecated,
please use TYPED_TEST_SUITE [-Wdeprecated-declarations]                                    
   static_assert(::testing::internal::TypedTestCaseIsDeprecated(), ""); \                                                                                      

and my XML looks like this:

<testcase name="frob_a_foo" status="run" result="skipped" time="0.01" classname="test_foo" />

XML for a disabled test looks like this:

<testcase name="DISABLED_frob_a_bar" status="notrun" result="suppressed" time="0" classname="test_foo" />                                                                                      

colcon test-result continues to work pretty much correctly. It doesn't tally the number of skipped tests ever - only the number of disabled tests which it calls 'skipped' in text printed to the console. It uses the summary number from the testsuite element and doesn't care about the status field of a testcase so none of these changes affect it.

Conclusion

I'd like to update ament_gtest to be based on either c6cb7e033591528a5fe2c63157a0d8ce927740dc or 3a460a26b7a91abf87af7f31b93d29f930e25c82

[wjwwood]

Note that we don't get notifications when you update comments.

[clalancette]

In general, I'm totally fine with an update to googletest (and the method you did, where you fast forward and then re-apply our changes is perfect). As long as this gets green CI, I'm fine with the update.

[pbaughman]

@clalancette Let me re-do my branch to be based off of c6cb7e0 instead of 00938b2b. It might be worth having an admin rebase and force-push to the ament/googletest repo instead of doing a merge so that your commits and dirk's commits stay at the head of the repo. That's really your call though

[clalancette]

@clalancette Let me re-do my branch to be based off of c6cb7e0 instead of 00938b2. It might be worth having an admin rebase and force-push to the ament/googletest repo instead of doing a merge so that your commits and dirk's commits stay at the head of the repo. That's really your call though

Yeah, we've done that before when we've updated vendored git repositories before, so I'm guessing we'll do the same here.

[dirk-thomas]

:+1: for the general idea of pull in a newer state of googletest.

I know that gtest master gets the XML right, but updating to that gives a whole bunch of deprecation warnings too when I build.

It would be good to pull in head in the future once these deprecation warnings have been addressed upstream.

[pbaughman]

It looks like the PR got closed when I force-pushed to my fork, but https://github.com/pbaughman/googletest is in the 'correct' final state for what I'm proposing. The top 4 commits are from you guys, and the 5th commit is c6cb7e0 from googletest

[pbaughman]

@dirk-thomas @clalancette Is there anything I can do to help test this? We build most of ROS2 from source and I've already updated our version and all the tests still pass. I don't think I'm able to run your CI though

[wjwwood]

Please have a look at https://github.com/ament/googletest/issues/5.