Michael, note that we've avoided having ASSERT_ or EXPECT_ inside test helper methods because they print out the line number of the helper method, rather than the line number where the helper method was called from the test. This is why we've been pretty careful when adding helpers and have tried to push assertions into the test itself (e.g. helper returns a Try instead of internally asserting).
Paul, are you saying that ASSERT within one case in a fixture will stop running all other cases for the fixture? Do you have a pointer to this? Sounds surprising. On Mon, Jul 27, 2015 at 3:04 PM, Paul Brett <[email protected]> wrote: > 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 >
