----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63625/#review190330 -----------------------------------------------------------
src/tests/resource_provider_manager_tests.cpp Lines 730 (patched) <https://reviews.apache.org/r/63625/#comment267626> Let's instead `ASSERT` on the value from `subscribed->provider_id()`. src/tests/resource_provider_manager_tests.cpp Lines 738-739 (patched) <https://reviews.apache.org/r/63625/#comment267627> Maybe use a helper here? disk.mutable_disk()->mutable_source()->CopyFrom(createDiskSourceRaw()); src/tests/resource_provider_manager_tests.cpp Lines 749 (patched) <https://reviews.apache.org/r/63625/#comment267629> This does not guarantee that the allocator will offer the resources from the resource provider in the next offer cycle since it might still be behind the master's view. We could instead e.g., reject decline all offers not containing the resource provider resources below; maybe you have another idea. src/tests/resource_provider_manager_tests.cpp Lines 754 (patched) <https://reviews.apache.org/r/63625/#comment267630> Might not need this for a single use below. src/tests/resource_provider_manager_tests.cpp Lines 817 (patched) <https://reviews.apache.org/r/63625/#comment267631> We should add helpers for the new operations to `src/tests/mesos.hpp`, and use them here, e.g., mesos::v1::Offer::Operation = v1::CREATE_BLOCK(disk); src/tests/resource_provider_manager_tests.cpp Lines 828 (patched) <https://reviews.apache.org/r/63625/#comment267632> Let's `ASSERT(operation->info().has_create_block())`. src/tests/resource_provider_manager_tests.cpp Lines 835 (patched) <https://reviews.apache.org/r/63625/#comment267637> Can we run the whole test with paused clock? It seems that the allocator could still be assembling an offer before we pause the clock so we do not get the expected offer below. src/tests/resource_provider_manager_tests.cpp Lines 858 (patched) <https://reviews.apache.org/r/63625/#comment267634> `const` ref. src/tests/resource_provider_manager_tests.cpp Lines 867 (patched) <https://reviews.apache.org/r/63625/#comment267636> `ASSERT_SOME(block)`. src/tests/resource_provider_manager_tests.cpp Lines 869 (patched) <https://reviews.apache.org/r/63625/#comment267635> Break the line after `,`. - Benjamin Bannier On Nov. 7, 2017, 4:36 p.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63625/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2017, 4:36 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Added a test for resource conversion using a resource provider. > > > Diffs > ----- > > src/tests/resource_provider_manager_tests.cpp > 4008b1c751d6227b99adef756e95174d7d8a62f2 > > > Diff: https://reviews.apache.org/r/63625/diff/1/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
