----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65039/#review195539 -----------------------------------------------------------
src/tests/storage_local_resource_provider_tests.cpp Lines 1613-1624 (patched) <https://reviews.apache.org/r/65039/#comment274746> We future-proofed and clarified setting the resource-provider capability in https://reviews.apache.org/r/64891/. Could you update the code here to follow the same pattern? src/tests/storage_local_resource_provider_tests.cpp Lines 1652 (patched) <https://reviews.apache.org/r/65039/#comment274747> Did you make sure that the ordering `updateSlave2` will always be ready before `updateSlave1`? I see that `FUTURE_PROTOBUF` leverages `EXPECT_CALL` (via `FutureMessage`), so I wonder if we need something like `::testing::InSequence` to make this this test safe. src/tests/storage_local_resource_provider_tests.cpp Lines 1656 (patched) <https://reviews.apache.org/r/65039/#comment274748> Would it make sense to have the whole test run with paused clock? src/tests/storage_local_resource_provider_tests.cpp Lines 1677 (patched) <https://reviews.apache.org/r/65039/#comment274749> Let's move this right before the code setting up the mock expectation. src/tests/storage_local_resource_provider_tests.cpp Lines 1695-1696 (patched) <https://reviews.apache.org/r/65039/#comment274750> Is this needed? Adding a framework should already trigger a allocation. src/tests/storage_local_resource_provider_tests.cpp Lines 1715 (patched) <https://reviews.apache.org/r/65039/#comment274751> Could we bind the return value here and make sure we actually dropped a message by awaiting it to become ready? src/tests/storage_local_resource_provider_tests.cpp Lines 1797 (patched) <https://reviews.apache.org/r/65039/#comment274753> This seems unneeded. - Benjamin Bannier On Jan. 9, 2018, 10:25 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65039/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2018, 10:25 a.m.) > > > Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, > and Jie Yu. > > > Bugs: MESOS-8373 > https://issues.apache.org/jira/browse/MESOS-8373 > > > Repository: mesos > > > Description > ------- > > This patch adds > StorageLocalResourceProviderTest.ROOT_ReconcileDroppedOperation > in order to verify that reconciliation is performed correctly > when an operation is dropped on its way from the master to the > agent. > > > Diffs > ----- > > src/tests/storage_local_resource_provider_tests.cpp > bbfe95e9818f25fdd5405db3ad2fe355e023f743 > > > Diff: https://reviews.apache.org/r/65039/diff/2/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Greg Mann > >
