mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 35247: Fixed race between EXPECT_CALL(resourceOffers, _) and driver.start() in fetcher_cache_tests.cpp.
Date Tue, 09 Jun 2015 20:07:37 GMT


> On June 9, 2015, 11:11 a.m., Vinod Kone wrote:
> > src/tests/fetcher_cache_tests.cpp, lines 360-361
> > <https://reviews.apache.org/r/35247/diff/1/?file=981340#file981340line360>
> >
> >     Why not just do AWAIT_READY(offers)?

That does not compile here, because it contains a "return" of type void and that does not
match the return type of "launch()".


> On June 9, 2015, 11:11 a.m., Vinod Kone wrote:
> > src/tests/fetcher_cache_tests.cpp, lines 201-207
> > <https://reviews.apache.org/r/35247/diff/1/?file=981340#file981340line201>
> >
> >     While this looks good as a temporary fix, what is the long term strategy here?
> >     
> >     I really don't like setting expectations in SetUp() or TearDown() because it's
really hard to reason about in the individual tests. For example, why did you set expecations
on registered and offers but not others? I prefer to move these expectations to tests. 
> >     
> >     Also, this SetUp() is doing too much (starting slave, starting master, constructing
scheduler but not starting it, setting some expectations) and there is no documentation for
it!

Long term I am working on developing up stress tests for the fetcher. These are still relatively
basic functionality tests so far.

Yes, SetUp() and TearDown() do a lot here. Would you prefer a) inlining them or b) commenting
what they do more or c) both? In my experience a) would be most consistent with existing patterns,
but it makes it harder to spot what the different tests are doing besides all the code that
is always the same. And the general code blowup would be rather substantial in this test series.


> On June 9, 2015, 11:11 a.m., Vinod Kone wrote:
> > src/tests/fetcher_cache_tests.cpp, lines 77-78
> > <https://reviews.apache.org/r/35247/diff/1/?file=981340#file981340line77>
> >
> >     reorder

thx. will fix.


- Bernd


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35247/#review87235
-----------------------------------------------------------


On June 9, 2015, 2:32 a.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35247/
> -----------------------------------------------------------
> 
> (Updated June 9, 2015, 2:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-2815, MESOS-2829 and MESOS-2831
>     https://issues.apache.org/jira/browse/MESOS-2815
>     https://issues.apache.org/jira/browse/MESOS-2829
>     https://issues.apache.org/jira/browse/MESOS-2831
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixed race between EXPECT_CALL(resourceOffers, _) and driver.start() in fetcher_cache_tests.cpp.
> 
> Installed a default response that provides teporary cover for this mocked method until
we install more interesting callbacks later. This prevents gmock from complaining about an
"uninteresting gmock call", which led to a variety of tests failing due to offers not getting
through subsequently.
> 
> All fetcher cache tests have been affected by this race and should be fixed in this regard
now.
> 
> (Also fixed some typos.)
> 
> 
> Diffs
> -----
> 
>   src/tests/fetcher_cache_tests.cpp cbd44b98d19953d174fac977f509d4900a37481f 
> 
> Diff: https://reviews.apache.org/r/35247/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message