> On Jan. 17, 2018, 8:44 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 1652 (patched) > > <https://reviews.apache.org/r/65039/diff/2/?file=1936333#file1936333line1652> > > > > 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.
Yea I think here I am unfortunately relying on an implementation detail of googletest. It does seem to be quite reliable, but it's not great. In order to effectively use a `Sequence` to force the order, we would need to pass a `Sequence` object into `FUTURE_PROTOBUF`, so that it can be chained onto `EXPECT_CALL` via `.InSequence()`. A single `InSequence` at the scope of the test wouldn't do the job, since each `EXPECT_CALL` invocation occurs within the scope of the `FutureMessage()` call. I would propose the following solution: we add a `SEQUENCED_FUTURE_PROTOBUF` macro which accepts a `Sequence` argument, and we update `FutureProtobuf` and `FutureMessage` to accept an `Option<Sequence>` with a default value of `None()`. This would enable the use of ordered expectations without requiring us to update a ton of callsites. WDYT? > On Jan. 17, 2018, 8:44 a.m., Benjamin Bannier wrote: > > src/tests/storage_local_resource_provider_tests.cpp > > Lines 1656 (patched) > > <https://reviews.apache.org/r/65039/diff/2/?file=1936333#file1936333line1656> > > > > Would it make sense to have the whole test run with paused clock? I need to leave a comment here on why I do this. Unfortunately, the standalone container running the CSI plugin complicates things a bit when running with the clock paused. The SLRP runs a `loop()` which looks for the CSI container's domain socket; once the loop finds it, SLRP init continues. Ideally, we would obtain a `Future` which is fulfilled when that container comes online; at that point we could advance the clock enough for the SLRP's loop to execute, and SLRP initialization would continue. However, I don't think there is a way to obtain such a `Future`. I think the easiest way around this is to resume the clock while the CSI container is launched, but I need a nice comment here explaining the need for this. - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65039/#review195539 ----------------------------------------------------------- On Jan. 17, 2018, 10:05 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65039/ > ----------------------------------------------------------- > > (Updated Jan. 17, 2018, 10:05 p.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 > >
