----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70728/#review216979 -----------------------------------------------------------
Fix it, then Ship it! src/tests/mesos.hpp Lines 3038 (patched) <https://reviews.apache.org/r/70728/#comment304212> It seems that I'm wrong about `Self`, which seems to become a type template in a class template. However, I've tested that we can simply use `MockResourceProviderProcess` (whitout any template argument) within this class without introducing a type alias. For example, use `&MockResourceProviderProcess::connectDefault`. That said, please feel free to drop this if you think introducing the `Self` type alias make the code more consistent. src/tests/mesos.hpp Line 3142 (original), 3150 (patched) <https://reviews.apache.org/r/70728/#comment304213> s/since // src/tests/mesos.hpp Lines 3151 (patched) <https://reviews.apache.org/r/70728/#comment304214> This introduces a coupling beween this test and the implementation of the JWT secret generator being synchoronous. How about the following instead: ``` process::Future<Option<std::string>> token = None(); #ifdef USE_SSL_SOCKET ... token = secretGenerator.generate(principal) .then([](const Secret& secret) -> Option<std::string> { return secret.value().data(); }); #endif // TODO(bbannier): Remove the `shared_ptr` once we get C++14. auto detector_ = std::make_shared<process::Owned<EndpointDetector>>(std::move(detector)); token.then(defer(self(), [=](const Option<std::string>& token) { driver.reset(new Driver( std::move(*detector_), ..., token)); driver->start(); return Nothing(); })); ``` src/tests/mesos.hpp Lines 3302 (patched) <https://reviews.apache.org/r/70728/#comment304216> Do we use `FIXME` in the codebase? src/tests/mesos.hpp Lines 3312 (patched) <https://reviews.apache.org/r/70728/#comment304215> s/provider_id/providerId/ for naming consistency. src/tests/mesos.hpp Lines 3359 (patched) <https://reviews.apache.org/r/70728/#comment304217> No need to make it public anymore. src/tests/mesos.hpp Line 3350 (original), 3408 (patched) <https://reviews.apache.org/r/70728/#comment304218> How about s/Mock/Test/ here and below? This is more of a personal naming opinion so please feel free to drop it. src/tests/resource_provider_manager_tests.cpp Lines 1203 (patched) <https://reviews.apache.org/r/70728/#comment304219> s/terminate/stop/ and remove the line break. src/tests/resource_provider_manager_tests.cpp Lines 1436 (patched) <https://reviews.apache.org/r/70728/#comment304220> Ditto. - Chun-Hung Hsiao On July 27, 2019, 7:04 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70728/ > ----------------------------------------------------------- > > (Updated July 27, 2019, 7:04 p.m.) > > > Review request for mesos, Chun-Hung Hsiao and Jan Schlicht. > > > Bugs: MESOS-9560 > https://issues.apache.org/jira/browse/MESOS-9560 > > > Repository: mesos > > > Description > ------- > > We were passing callbacks into `MockResourceProvider` to the HTTP > driver. Since the lifecycle of the callbacks and the mock provider were > decoupled and these callbacks were binding the mock provider instance > the code was not safe as written as the driver could invoke the callback > after the provider had been destructed. > > This patch makes sure that the callbacks are defered to a process. We > also dispatch a number of other functions which are strongly coupled to > the lifecycle of the provider. We still do not hide the provider away > completely so the provider can be mocked in tests. > > > Diffs > ----- > > src/tests/api_tests.cpp 641eb15153ddb85df322aa6a133ca8e4c6d6a510 > src/tests/master_slave_reconciliation_tests.cpp > 7b6ac50adacc8416b91c0dde55ff7ba46a20515c > src/tests/master_tests.cpp b9ef13c31a9c3ae16e55d3ae8f9b1538a49cf49a > src/tests/mesos.hpp 4612e2e3d2bd32590248df58b546de8756636964 > src/tests/operation_reconciliation_tests.cpp > eae318da2273faae904f0155e49bb23cdb24f007 > src/tests/resource_provider_manager_tests.cpp > 7d48f18e89f046df6c92e52edeef592acfb13b10 > src/tests/slave_tests.cpp abee107720d6b78bb017d2762431ae36c0679026 > > > Diff: https://reviews.apache.org/r/70728/diff/7/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
