Detect and report "Rotten Green Tests"

[pogo59]

Rotten Green Tests are tests that have assertions that did not execute, even though they were contained in a test method that was executed.

Use --gtest_treat_rotten_as_pass to report these but not cause them to fail the test execution.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[derekmauro]

This is one of the more interesting pull requests I've seen. However, we do need the CLA before we can discuss it.

[pogo59]

I am actively trying to get the CLA issue resolved. If someone on the Google side can tell me who the Sony/SIE contact is, that would help immensely.

[pogo59]

I'm told I've been added to the CLA, so we should be okay to go now?

[pogo59]

I've added the cla: yes and cla: no labels as instructed on the troubleshooting page, and my corporate contact says I should be on the list. But I'm still failing the CLA check.
The troubleshooting page says "direct the Google project maintainer to go/cla#troubleshoot" so @derekmauro if you could do that and tell me specifically what's missing, that would be lovely.

[pogo59]

@derekmauro According to https://cla.developers.google.com/clas, paul.robinson@sony.com is on the corporate CLA https://cla.developers.google.com/about/android-legacy-corporate
I don't know why the workflow check is failing, that would seem to be a problem on your side?

[derekmauro]

Looks like your CLA is passing now.

I want to be upfront with you here. In principle, I like this a lot. It might be hard to merge, but I do think it is doable.

What you need to understand is that my mandate as a GoogleTest maintainer is to primarily support Google's use cases. For everyone else, while we hope GoogleTest is useful, it unfortunately means that use cases for everyone else don't get as high of a priority. In my personal view I'm not 100% happy with this stance, but such is life.

GoogleTest is developed with our internal monorepo as the source of truth. The Software Engineering at Google book talks about the approach. Changes first get merged into the monorepo, then get re-exported to GitHub. Unfortunately, as an open source contributor, you don't get visibility into the monorepo.

This also means that we have put in place what we call the "Churn Policy". This policy basically says that if you want to make a change, the people making the change must do the work to move their internal users to new versions themselves, or do the update in place, in backward-compatible fashion. https://abseil.io/resources/swe-book/html/ch01.html#scale_and_efficiency talks about this in more detail.

As a consequence, we can't enable detection of rotten green tests by default, as much as I'd like to. We have (I'm guessing) millions of tests written using the GoogleTest framework. You can bet that a non-trivial number of them would start failing if this were enabled. For better or worse, the engineering culture here is that if your change breaks a distant test, your change almost certainly gets rolled back, even if it exposed a bug in the downstream code. Personally I dislike this - if my code has a bug, I want to know. But the reality of the situation is that most build-cops are not equipped to quickly understand why their tests are failing.

So to set exceptions, it is going to require a lot of my time to get this merged internally.

Now I haven't taken a hard look at the implementation, but here is what needs to happen initially.

• This has to be backwards compatible, at least for now. That means this must be disabled by default. We can look into turning it on by default at a later time.
• It also looks like you have set this up to work with CMake, but not Bazel. Bazel is our primary build system, and it must be supported.

Let me know if you have questions, and thanks for submitting this for consideration.

[pogo59]

Thanks Derek! Very happy to have the bureaucratic stuff over with.

It was pretty clear from the googletest commit history that this is an internal thing that Google chooses to make available. I agree with you that this will likely find a bunch of things in Google's internal tests, and I have heard from other Googlers about the "it's on you" policy. I have exactly the same situation with LLVM, my main project--I have not been able to fix all the rotten tests there. So, I am on board with having RGT detection turned off by default.

There's already a global toggle to let you compile-out RGT support, but that's too big a hammer, I think. (I had to invent that because at some point older version of gcc weren't able to build it.) There's a command-line option to control rotten=pass/fail, which I can fiddle the LLVM test runner to set to pass, but maybe that's tricky for how Google works. A separate toggle to control the default pass/fail behavior would be worthwhile. Then you can have a default mode that reports rotten assertions but doesn't fail the test program. I don't know how you have things set up internally but maybe this would let you enable RGT incrementally as you fix tests.

I'll have a go at the Bazel stuff. I've never used it before, and the syntax looks a little funky, but with any luck it shouldn't be too hard. It wasn't clear whether contributors were on the hook for that part, but it's fine to have me do it.

Question for you. I don't have access to a Mac, so I haven't tested RGT in that environment at all. If Macs use Clang to build, there's a decent chance that it will Just Work, but no way to know until somebody tries it. Maybe you can do that after I get the Bazel stuff done?

FTR, this is a side project for me so it might take a little while to deliver an update.
Also FYI, I've submitted a paper describing this work to the ESEC/FSE 2023 conference. Don't worry, I didn't say it was in GoogleTest, just that I had submitted it to you. I've assumed that the best reference to cite would be the github URL; if there's something better, let me know.

[pogo59]

I've pushed an update to make the default for --gtest_treat_rotten_as_pass configurable, and default to true so existing test suites won't suddenly start failing.

Filed #4254 regarding my failure to get bazel to build googletest, which is kind of a prerequisite to providing BUILD file patches for the new files.

[pogo59]

bazel build //:gtest --cxxopt=-std=c++14 (as suggested in the other issue) completes with no errors.
Not clear how to build/run the googletest tests under bazel, though.

[pogo59]

bazel build //googletest/test:gtest_all_tests --cxxopt=-std=gnu++14 looks like it is compiling everything okay, but failing to link because it can't find my new APIs. They're all decorated with GTEST_API_. The top-level BUILD.bazel file specifies "googletest/src/*.cc" which seems like it ought to include my new files. Am I missing something about how to add a new file to the googletest library? I looked at a few commits that added new files and didn't see anything build-related in them.

[pogo59]

Found the build problem--bazel builds modules individually, but CMake uses gtest-all.cc, which allowed some sloppiness to creep in. That's fixed.

Speculatively tried bazel test //:gtest which complained "No test targets were found." Then tried bazel test //googletest/test:gtest_all_test which didn't complain, but it is clearly not running my new tests, despite changes to the BUILD.bazel file. Please advise how to run the tests under bazel.

[derekmauro]

Found the build problem--bazel builds modules individually, but CMake uses gtest-all.cc, which allowed some sloppiness to creep in. That's fixed.

Speculatively tried bazel test //:gtest which complained "No test targets were found." Then tried bazel test //googletest/test:gtest_all_test which didn't complain, but it is clearly not running my new tests, despite changes to the BUILD.bazel file. Please advise how to run the tests under bazel.

bazel test ... will run the tests. If you have docker installed, you can also run ci/linux-presubmit.sh.

[pogo59]

Hi Derek, good news and bad news. The good news is, I got my new tests to build and run under Bazel. You might want to look at the BUILD.bazel change, where I had to list a test source explicitly, because it doesn't have gtest_*.cc as a dependency. It looks like the existing BUILD.bazel ought to have gtest_*.cc in the dependencies for gtest_all_test.cc, because some of its components have that filename pattern. (I didn't do that, because bazel is still a bit of a mystery to me.) But that isn't the bad news.

The bad news is: I think the current implementation of RGT will be useless under Bazel. This is because the implementation needs to parse the test source, at runtime. It records the __FILE__ macro at compile time. Unfortunately, the __FILE__ macro reflects only what's provided on the command line, and bazel passes a relative path to gcc. Under bazel, the test execution CWD is not the same as the compilation directory, and therefore the relative path name from __FILE__ isn't enough to find the source code. Without the source to parse, it can't associate assertions with Test methods, which means it can't constrain reports only to those Test methods that actually passed, which makes for an unacceptably high false-positive rate. So, no parse = no report.

I've only figured this out now because I developed RGT using CMake to generate makefiles, and it seems those pass absolute paths to the compiler. Under CMake, __FILE__ is an absolute path, so as long as the test is run on the same system where it was built, RGT will be able to find the source, and do all its reporting correctly.

If there's a magic option to bazel to have it pass absolute paths to the compiler, then it could still work. I did skim the bazel command-line reference and didn't see anything that looked like this.

So, lacking a magic bazel option, if we want to proceed with RGT, we need to come up with a tactic that doesn't involve parsing source at runtime.

Now, the reason RGT parses the source is because I couldn't come up with any other way to associate assertions with their containing Test methods reliably at compile-time. There's a way to do it for the TEST and TEST_F macros, because those generate a test_info symbol, and the assertion macros can capture that in order to identify the containing Test method. That's actually what I did originally. But, none of the other TEST* macros work that way, which is why I went to parsing the source.

So, I could go back to capturing test_info but have RGT work only for TEST and TEST_F. Better than not having RGT at all, but it's obviously an incomplete solution. And if we come up with something that allows the assertions to statically identify the containing TEST_P, TYPED_TEST, and TYPED_TEST_P, well, now we're talking!

Let me know how you want to proceed with this.

[pogo59]

See #4552 for a revised patch that works in Google's Bazel environment, but is less effective.