I am working on building and running LLVM unit tests, which are built on top of GoogleTest, for the Fuchsia platform. I noticed that the GoogleTest source under third-party/unittest is still on v1.10.0, which lacks some bug fixes related to the Fuchsia platform.
Instead of backporting these fixes to LLVM’s GoogleTest fork, I am wondering if it is better to take this opportunity to update it to the latest release version, which is v1.14.0. This is 3 years newer than what LLVM currently has. Rolling GoogleTest to v1.14.0 will bring a lot of updates and bug fixes, especially the C++ 17 support introduced in the v1.11.0 release.
In addition, it would be great if we can modularize the LLVM’s GoogleTest fork so that third-party/unittest is just a vanilla copy of GoogleTest (with the LLVM-specific custom code in a separate directory) and a script that could be used to update it in the future similar to how we manage Google Benchmark in third-party/benchmark.
No objection from me. Please make sure to test the new googletest on a wide range of platforms (including Windows). It would probably be worth when doing this testing to make sure that the same number of tests were run as before the upgrade, just in case something funky’s going on with the behaviour of the gtest macros.
If you can manage that, it would be great, although I’m not 100% convinced it would be feasible. Some of the customizations are doing things like adding overloads to accept LLVM internal types such as StringRef. But the closer we can get to a straight copy of upstream, the better.
I’ve been working on an enhancement to Google Test for some time so I’m moderately familiar with it; please add me as a reviewer (my Phab handle is probinson). But also please make a note here when you post the patch, as I’m more likely to notice it here than as a Phab email.
Not as it is right now. There is a bunch of LLVM customizations to the in-tree copy of googletest. Some can be dropped now, some can be worked around, but some would probably require rewriting lots of tests. It’d be good to have at least a clear diff with upstream and work on reducing/isolating it.
llvm::StringRef does not exist in upstream googletest, or in any non-LLVM client of googletest, so merging a change to support it would be problematic.
If possible, we should use googletest’s existing local customization hooks to add this support locally.
(Edit) Having looked at the patch now, that is mainly what we did. Googletest buries those customization headers deep in its tree so trying to do something like a git submodule is unlikely to work.
LLVM may not be extending gtest in the proper way, it may never have been doing so. That’s water under the bridge at this point.
Continuing to hammer this point in this tone is not going to help matters and only serves to discourage those who might generously offer their time to improve it.
Luckily the thread author has posted a PR already. If you don’t think that’s enough, you could submit a follow up to improve the situation, I’m sure you’ll have plenty of reviewers
Reviewing the patch, it looks like there are two problems with using a completely unmodified upstream googletest.
LLVM has its own stream implementation, which requires a more intrusive change than the usual googletest customization scheme allows. It’s relatively minimal and it’s possible upstream googletest could be persuaded to support it.
Googletest’s own scheme for supporting customizations involves changing header files deep inside its source tree. There is literally nothing we can do about that.
That would be really great.