Paul, 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.
We should only be using *ASSERT_** in cases where it doesn't make sense to proceed with the rest of the test if the condition fails, so exiting the test case seems like it's exactly what we would want. If you're saying that we currently use it incorrectly, then yeah, we should perhaps write a guide to help with how to use it correctly. But it sounds like you're saying we shouldn't use it at all? 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. Perhaps my suggestion of resorting to *CHECK_** was not a good one, but I still don't think *EXPECT_** is what we want. If we have a condition in which it doesn't make sense to proceed with the rest of the test, we should stop. Perhaps the helper function should return a *Try* as Ben suggested, proceeded by an *ASSERT_** of the result within the test case or something like that. I mainly wanted to inform folks of this limitation and the corresponding confusing error message that follows. On 27 July 2015 at 18:42, Benjamin Mahler <[email protected]> wrote: > 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 > > >
