Updating googletest to v1.10.0

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Updating googletest to v1.10.0

Fangrui Song via cfe-dev
Hi all,

On a Phabricator review (https://reviews.llvm.org/D78704) the `GTEST_SKIP` was
mentioned as a way to express requirements for a test. In that patch, I wanted
to skip the test if clang was not built with Z3.

I think this `GTEST_SKIP` macro express the intention much better than the
following code that I saw throughout the codebase:

```
#define CHECK_UNSUPPORTED() \
  do { \
    if (isUnsupportedOSOrEnvironment()) \
      return; \
  } while (0);

```


There are several places where we might benefit from the `GTEST_SKIP` macro:
- llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29
- llvm/unittests/Support/ThreadPool.cpp:81
- llvm/unittests/Support/MemoryTest.cpp:89

It is possible that the upgrade would introduce other benefits as well - which
I'm not aware of.

Should we upgrade googletest to version 1.10.0?

/CC Chandler

Balazs.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Updating googletest to v1.10.0

Fangrui Song via cfe-dev
On Fri, Apr 24, 2020 at 11:17 AM Balázs Benics via cfe-dev <[hidden email]> wrote:
Hi all,

On a Phabricator review (https://reviews.llvm.org/D78704) the `GTEST_SKIP` was
mentioned as a way to express requirements for a test. In that patch, I wanted
to skip the test if clang was not built with Z3.

I think this `GTEST_SKIP` macro express the intention much better than the
following code that I saw throughout the codebase:

```
#define CHECK_UNSUPPORTED() \
  do { \
    if (isUnsupportedOSOrEnvironment()) \
      return; \
  } while (0);

```


There are several places where we might benefit from the `GTEST_SKIP` macro:
- llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29
- llvm/unittests/Support/ThreadPool.cpp:81
- llvm/unittests/Support/MemoryTest.cpp:89

It is possible that the upgrade would introduce other benefits as well - which
I'm not aware of.
I know there are some minor features unavailable in our old gtest that I've wanted to use and worked around.
Unfortunately I can't remember what these cases were, but I think there are at least minor benefits.
 
Should we upgrade googletest to version 1.10.0?
+1 from me.

A thing to be aware of for anyone doing the upgrade: we have a few customizations e.g.
llvm/utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h, and also small (I hope clearly marked) modifications within gtest itself.

Some of these you'll notice (replacing them with clean sources will break the build), but others affect assertion failure message quality, and I'm not sure if they themselves are well tested.
It's probably worthwhile to trawl the history of the gtest directories to find these...

 

/CC Chandler

Balazs.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Updating googletest to v1.10.0

Fangrui Song via cfe-dev
On Fri, 24 Apr 2020 at 14:01, Sam McCall via cfe-dev
<[hidden email]> wrote:

>
> On Fri, Apr 24, 2020 at 11:17 AM Balázs Benics via cfe-dev <[hidden email]> wrote:
>>
>> Hi all,
>>
>> On a Phabricator review (https://reviews.llvm.org/D78704) the `GTEST_SKIP` was
>> mentioned as a way to express requirements for a test. In that patch, I wanted
>> to skip the test if clang was not built with Z3.
>>
>> I think this `GTEST_SKIP` macro express the intention much better than the
>> following code that I saw throughout the codebase:
>>
>> ```
>> #define CHECK_UNSUPPORTED() \
>>   do { \
>>     if (isUnsupportedOSOrEnvironment()) \
>>       return; \
>>   } while (0);
>> ```
>>
>>
>> There are several places where we might benefit from the `GTEST_SKIP` macro:
>> - llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29
>> - llvm/unittests/Support/ThreadPool.cpp:81
>> - llvm/unittests/Support/MemoryTest.cpp:89
>>
>> It is possible that the upgrade would introduce other benefits as well - which
>> I'm not aware of.
>
> I know there are some minor features unavailable in our old gtest that I've wanted to use and worked around.
> Unfortunately I can't remember what these cases were, but I think there are at least minor benefits.

One thing I remember running into was the inability to add "override"
specifiers to mocked methods, which tends to trigger a torrent of
-Winconsistent-missing-override warnings. New googletests solve that

>
>>
>> Should we upgrade googletest to version 1.10.0?
>
> +1 from me.
+1 as well, and I also volunteer to help with any problems within lldb tests.

pl
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev