mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Schlicht <...@mesosphere.io>
Subject Re: Review Request 58347: Added 'ASSERT_THREADSAFE' in libprocess.
Date Thu, 13 Apr 2017 11:20:52 GMT


> On April 11, 2017, 8:45 p.m., Neil Conway wrote:
> > I'm curious what the goal of these assertions is: to fail the test (at runtime)
if the current version of GTest is not thread-safe, right? It seems weird to be doing that
at runtime, since whether GTest is threadsafe or not seems to be a compile-time property.
> > 
> > Would it make more sense to just avoid compiling these tests if `GTEST_IS_THREADSAFE`
is not defined?

Yes, we could do something like
```
#if GTEST_IS_THREADSAFE
#define TEST_THREADSAFE(test_case_name, test_name) \
  TEST(test_case_name, test_name)
#else
#define TEST_THREADSAFE(test_case_name, test_name) \
  TEST(test_case_name, DISABLED_##test_name)
#endif
```
similar to the `TEST_TEMP_DISABLED_ON_WINDOWS` macros, or use a `THREADSAFE_` prefix in the
test name and add a filter for these tests. I prefer the second solution, because it allows
a test case to have mutiple prefixes and will implement that.


> On April 11, 2017, 8:45 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp
> > Line 294 (original), 294 (patched)
> > <https://reviews.apache.org/r/58347/diff/1/?file=1687944#file1687944line294>
> >
> >     Seems like the reference to `GTEST_IS_THREADSAFE` should be updated, here and
elsewhere.
> 
> Joseph Wu wrote:
>     One thing to note: Upgrading gtest to 1.8 will bring thread-safety support to Windows
( https://reviews.apache.org/r/58349/ ), so the macro will actually be defined.

Good point. I'll re-enable these tests.


- Jan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58347/#review171598
-----------------------------------------------------------


On April 11, 2017, 2:27 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58347/
> -----------------------------------------------------------
> 
> (Updated April 11, 2017, 2:27 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Till Toenshoff.
> 
> 
> Bugs: MESOS-7193
>     https://issues.apache.org/jira/browse/MESOS-7193
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Using 'GTEST_IS_THREADSAFE' in asserts to fail tests that need
> thread-safety early is problematic, because it may be undefined.
> Instead, a new assertion is introduced that evaluates
> 'GTEST_IS_THREADSAFE' and succeeds or fails accordingly.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/gtest.hpp 27077ac9047447fc4c52cc76ab26420e5bc79418

>   3rdparty/libprocess/src/tests/future_tests.cpp f21361ed1e354778bcd0357afb71300f05d3ecfd

>   3rdparty/libprocess/src/tests/io_tests.cpp 302fadc2a1894d3fd7c4f4975af3f2cc6c3a22de

>   3rdparty/libprocess/src/tests/limiter_tests.cpp b80b1da214f97b50aa7b61b79bbf683fd01116aa

>   3rdparty/libprocess/src/tests/metrics_tests.cpp d7fdb06060b273e16be27a263b5ee268842aa25c

>   3rdparty/libprocess/src/tests/process_tests.cpp bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115

>   3rdparty/libprocess/src/tests/reap_tests.cpp 30518dee6c2fb904a607c7a457a5ec7366aab818

> 
> 
> Diff: https://reviews.apache.org/r/58347/diff/1/
> 
> 
> Testing
> -------
> 
> libprocess-tests (using automake and CMake with macOS, Linux, Windows)
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message