> On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/Makefile.am, line 1366 > > <https://reviews.apache.org/r/29748/diff/7/?file=897372#file897372line1366> > > > > Is one tab missing?
Haha I have no idea. I see examples of lines with 1, 2 or 3 tabs, spaces, etc. The example I took this line from, `tests/resource_offers_tests.cpp` has one tab. > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, line 618 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line618> > > > > Let's adjust the resources to be not greater than available resources, > > in order we test what we want : ). Great catch! Fixed. > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 430-431 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line430> > > > > Do you need to set this expectation explicitly? `TestAllocator`'s c-tor > > sets it. Didn't know that. Removed. > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 80-85 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line80> > > > > Do we really want to do these tests for all allocator types? I don't have an argument for why we should but I don't see an argument for why we shouldn't either. Would you prefer that we don't test for all allocators? > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 337-338 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line337> > > > > Again, do we need to set this explicitly? Removing this results in `Uninteresting mock function call` warnings from `gmock`. > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 311-312 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line311> > > > > Ensure `frameworkId` is set by awaiting. Ditto above and below. I've added the `AWAIT_READY`s since we use that pattern for `offer`s everywhere. But a `Future`'s `get` function waits if it's not ready yet. I suppose we prefer to be explicit in our tests? > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 253-255 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line253> > > > > What do you check with this expectation? This was taken from other tests that do this. I thought it was there to suppress the `Uninteresting mock function call` warning, but I removed it and I don't see any warnings. Do you know what conditions trigger the warning? > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, line 271 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line271> > > > > Can we merge this test with the previous one? By "merge" do you mean I can remove the previous since this is a superset of it? > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 218-222 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line218> > > > > Shouldn't we `driver.start();` here between setting an expectation and > > awaiting offers? Ditto above and below. I think you're right. Am I getting lucky in race conditions here? or is this a valid way to write tests, just not preferred? > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, lines 235-236 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line235> > > > > This looks shady: why do we send TASK_FINISHED more than one time? `s/WillRepeatedly/WillOnce/` > On March 18, 2015, 12:29 a.m., Alexander Rukletsov wrote: > > src/tests/reservation_tests.cpp, line 162 > > <https://reviews.apache.org/r/29748/diff/7/?file=897374#file897374line162> > > > > Does it make sense to check we can unreserve prior launching anything? Changed the `Reserve` test to `ReserveThenUnreserve`, and removed `ReserveAndLaunch` since `ReserveAndLaunchThenUnreserve` is a superset of it. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29748/#review76762 ----------------------------------------------------------- On March 18, 2015, 10:48 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29748/ > ----------------------------------------------------------- > > (Updated March 18, 2015, 10:48 p.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu. > > > Bugs: MESOS-2489 > https://issues.apache.org/jira/browse/MESOS-2489 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb > src/tests/reservation_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/29748/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Michael Park > >
