> 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? > > Michael Park wrote: > 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.
I think we insert tabs (8 chars length) so that \ align vertically. > 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? > > Michael Park wrote: > 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? I think that a custom allocator doesn't need to reserve and unreserve resources. We introduce this feature into our allocator, but I'm not sure we enforce any allocator to account for reservations. > 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. > > Michael Park wrote: > 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? I guess you're lucky. You can insert `sleep(20)` and check it out : ). > 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? > > Michael Park wrote: > 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? We suppress warnings in a different way, see e.g. `TestAllocator`. > 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? > > Michael Park wrote: > By "merge" do you mean I can remove the previous since this is a superset > of it? > > Michael Park wrote: > If yes, I did that. Otherwise, please let me know what you would like > done here :) Yes, you're reading my thougts well! : ) > 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. > > Michael Park wrote: > 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? I personally find it much easier to read and understand your intention: when do you expect a certain action to be completed. > 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? > > Michael Park wrote: > Removing this results in `Uninteresting mock function call` warnings from > `gmock`. Makes sense. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29748/#review76762 ----------------------------------------------------------- On March 23, 2015, 5:21 p.m., Michael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29748/ > ----------------------------------------------------------- > > (Updated March 23, 2015, 5:21 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 > >
