Mike I would suggest that we want to avoid both ASSERT and CHECK macros in tests.
With ASSERT, I completely agree with you about the perils of using ASSERT that you list above, but additionally we have cases where ASSERT exits a test fixture skipping later tests that might or might not have failed. Since the CHECK macro aborts the test harness, a single test failure prevents tests from running on all the remaining tests. Particularly annoying for anyone running automated regression tests. We should add this to either the style guide or mesos-testing-patterns. -- @paul_b On Mon, Jul 27, 2015 at 2:28 PM, Michael Park <[email protected]> wrote: > I've had at least 3 individuals who ran into the issue of *ASSERT_** macro > placement and since the generated error message is less than useless, I > would like to share with you what the issue is. > > The source of the issue is that *ASSERT_** macros from *gtest* can only be > placed in functions that return *void* as described in Assertion Placement > < > https://code.google.com/p/googletest/wiki/AdvancedGuide#Assertion_Placement > > > . > > By placing it in a non-void function, you get useless error messages like > this: > > From *GCC*: "*error: void value not ignored as it ought to be*" > From *Clang*: "*error: could not convert > ‘testing::internal::AssertHelper((testing::TestPartResult::Type)2u, ((const > char*)"../../src/tests/containerizer/port_mapping_tests.cpp"), 320, > > gtest_ar.testing::AssertionResult::failure_message()).testing::internal::AssertHelper::operator=(testing::Message())’ > from ‘void’ to ‘int’*" > > I think the following are typical situations that result in this mess: > > - Using them in *constructors/destructors*. For example, it would be > really confusing if you're simply translating a *SetUp*/*TearDown* of a > test fixture to be *constructor/destructor* instead. > - Using them in *main*, since it returns an *int*. > - Refactoring a chunk of logic from a test case into a helper function > that doesn't return *void*. For example, if we were factor out the > following code inside of a test case: > > > > > *AWAIT_READY(offers); ASSERT_EQ(1u, offers.get().size()); offer = > offers.get()[0]; * > into a function like this: > > > > > > * Offer getOffer(Future<vector<Offer>> &offers) { > AWAIT_READY(offers); ASSERT_EQ(1u, offers.get().size()); > return > offers.get()[0]; }* > > this innocent-looking transformation would trigger the obscure error > message and you'll be upset once you figure out why. > > If you encounter this case, prefer to resort to *CHECK_** from *glog*, > rather > than *EXPECT_**, since *CHECK_** has closer semantics. > > I hope this will help folks save time and also reduce the amount of head > banging! > > Thanks, > > MPark. > -- -- Paul Brett
