> On June 9, 2015, 6:11 p.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! > > Bernd Mathiske wrote: > 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. > > Bernd Mathiske wrote: > Maybe you meant the long term plan wrt. the code structure here. In this > case, it is to create more generally applicable test pattern lego blocks that > aggregate such things as in the Setup() here. These will have to be carefully > selected/crafted then.
i would prefer to inline them as we do everywhere else in the code base. The current abstractions are a bit hard to follow. > On June 9, 2015, 6:11 p.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)? > > Bernd Mathiske wrote: > That does not compile here, because it contains a "return" of type void > and that does not match the return type of "launch()". aah. ok. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35247/#review87235 ----------------------------------------------------------- On June 9, 2015, 9: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, 9: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 > >
