> 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. > > Greg Mann wrote: > 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?
Ah, actually Chun-Hung showed me that this is documented behavior in googletest: https://github.com/google/googletest/blob/master/googlemock/docs/ForDummies.md#using-multiple-expectations So, while the code here is a bit awkward, it is correct. I think I'll just add a comment here similar to what Chun-Hung has done is his patches, to explain the reason for the reverse ordering. - 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 > >
