You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Michael Park <mc...@gmail.com> on 2015/07/27 23:28:52 UTC
Beware of ASSERT_* Placement
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.
Re: Beware of ASSERT_* Placement
Posted by Benjamin Mahler <be...@gmail.com>.
Because?
On Tue, Jul 28, 2015 at 12:54 AM, Bernd Mathiske <be...@mesosphere.io>
wrote:
> IMHO we would be better off with exception-based asserts, checks, and
> expects.
>
> Bernd
>
> > On Jul 28, 2015, at 7:53 AM, Paul Brett <pb...@twitter.com.INVALID>
> 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 <mc...@gmail.com> 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 <be...@gmail.com>
> >> 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 <pbrett@twitter.com.invalid
> >
> >>> 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 <mc...@gmail.com>
> >> 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
>
>
Re: Beware of ASSERT_* Placement
Posted by Bernd Mathiske <be...@mesosphere.io>.
IMHO we would be better off with exception-based asserts, checks, and expects.
Bernd
> On Jul 28, 2015, at 7:53 AM, Paul Brett <pb...@twitter.com.INVALID> 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 <mc...@gmail.com> 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 <be...@gmail.com>
>> 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 <pb...@twitter.com.invalid>
>>> 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 <mc...@gmail.com>
>> 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
Re: Beware of ASSERT_* Placement
Posted by Paul Brett <pb...@twitter.com.INVALID>.
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 <mc...@gmail.com> 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 <be...@gmail.com>
> 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 <pb...@twitter.com.invalid>
> > 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 <mc...@gmail.com>
> 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
Re: Beware of ASSERT_* Placement
Posted by Michael Park <mc...@gmail.com>.
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 <be...@gmail.com> 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 <pb...@twitter.com.invalid>
> 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 <mc...@gmail.com> 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
> >
>
Re: Beware of ASSERT_* Placement
Posted by Benjamin Mahler <be...@gmail.com>.
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 <pb...@twitter.com.invalid>
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 <mc...@gmail.com> 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
>
Re: Beware of ASSERT_* Placement
Posted by Paul Brett <pb...@twitter.com.INVALID>.
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 <mc...@gmail.com> 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