Michael I think Ben's suggestion of using Try<> is just what we want for common functions.
In regards to ASSERTs, they can cause tests to be skipped within instantiations of the fixtures or test case as expected. For example, If you look at tests such as SlaveRecoveryTest::ReconnectExecutor, it has 9 ASSERTs in a single test case. The first 5 are in setup code and seem pretty reasonable but the last 4 are: 489 // Executor should inform about the unacknowledged update. 490 ASSERT_EQ(1, reregister.updates_size()); 491 const StatusUpdate& update = reregister.updates(0); 492 ASSERT_EQ(task.task_id(), update.status().task_id()); 493 ASSERT_EQ(TASK_RUNNING, update.status().state()); 494 495 // Scheduler should receive the recovered update. 496 AWAIT_READY(status); 497 ASSERT_EQ(TASK_RUNNING, status.get().state()); So looking at this code, I suspect that lines 492 and 493 might be better as EXPECT? What about 497? What follows afterwards is only cleanup code, so either it is not necessary and we can omit it or 497 should be an expect. Looking through the tests directory, this appears to be a common pattern. Of course, it is all harmless while the code is passing the tests but when a change breaks things, the scope of the breakage can be obscured because of the skipped test conditions. Given the restrictions you point out on the use of ASSERT combined with the ability to hide failing tests, I believe we should have a strong preference for EXPECT over ASSERT unless it is clear that every subsequent test in the test cast is dependent on the result of this test. Just my 5c worth @paul_b On Mon, Jul 27, 2015 at 7:34 PM, Michael Park <[email protected]> wrote: > 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 > > > > > > -- @paul_b
