> On Jan. 17, 2018, 9:44 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 1797 (patched) > > <https://reviews.apache.org/r/65039/diff/2/?file=1936333#file1936333line1797> > > > > This seems unneeded. > > Greg Mann wrote: > If the clock isn't resumed at the end of the test, teardown will not > complete successfully. The destruction of containers in `Slave::~Slave()` in > 'cluster.cpp' requires the clock to be running in order to finish.
That sounds like a bug in `cluster::Slave::~Slave`, and I do not see this explicit `resume` pattern elsewhere a lot either. Adding an explicit `resume` also does not help in cases where an assertion before the `resume` leads to the rest of the test being skipped. The only way to make sure it is called is to invoke it in some destructor RAII style. I'd suggest to drop the `resume` here for consistency, and instead fix the destructor of `~Slave` to make it self-sufficient (either file a ticket or add a cleanup). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65039/#review195539 ----------------------------------------------------------- On Jan. 18, 2018, 2:57 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65039/ > ----------------------------------------------------------- > > (Updated Jan. 18, 2018, 2:57 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/3/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Greg Mann > >
