IMHO we would be better off with exception-based asserts, checks, and expects.
Bernd > On Jul 28, 2015, at 7:53 AM, Paul Brett <[email protected]> wrote: > > 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
