----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70671/#review215497 -----------------------------------------------------------
Thanks for adding this: (1) If we're going to introduce this, let's include a patch after it that updates all the `SUBSCRIBE` related tests in `src/tests/api_tests.cpp` to use this. (2) Can you also include some folks with context for the `SUBSCRIBE` code / tests in this review (and the one that updates `src/tests/api_tests.cpp`?) (3) It seems the code is prone to crashing if the object gets destructed, consider using a Process wrapper and dispatching. (4) How about placing this in a mocks/ folder? src/tests/mock_master_api_subscriber.hpp Lines 53 (patched) <https://reviews.apache.org/r/70671/#comment302204> ``` EXPECT_CALL(*this, other(testing::_)) .WillRepeatedly(testing::Return()); ``` src/tests/mock_master_api_subscriber.hpp Lines 60-63 (patched) <https://reviews.apache.org/r/70671/#comment302205> Per my comment above, since we want to use this class to improve the existing testing as well, let's just include all the possible events to start with and no `other` case. I suppose I haven't seen this before, the `_T` here lets you define these mock methods without them overriding anything? In this sense, this class isn't really a "mock" that mocks an existing interface or implementation, but I suppose calling it a "Mock" is ok. (the alternative would be "TestMasterAPISubscriber", but then rather than a mocks/ folder we probably would want to group master/ tests together). src/tests/mock_master_api_subscriber.cpp Lines 40 (patched) <https://reviews.apache.org/r/70671/#comment302207> newline src/tests/mock_master_api_subscriber.cpp Lines 42 (patched) <https://reviews.apache.org/r/70671/#comment302208> newline src/tests/mock_master_api_subscriber.cpp Lines 48-50 (patched) <https://reviews.apache.org/r/70671/#comment302209> This can crash if `this` has been destructed, no? src/tests/mock_master_api_subscriber.cpp Lines 71-80 (patched) <https://reviews.apache.org/r/70671/#comment302210> This can crash if the subscriber is destructed, no? src/tests/mock_master_api_subscriber.cpp Lines 103-105 (patched) <https://reviews.apache.org/r/70671/#comment302206> Per comment above, let's include all switch cases / events to start with and not use `default`. If `default` is absent the build will break when a new switch case is added and this code isn't updated, which is what we want to happen. - Benjamin Mahler On May 21, 2019, 1:54 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70671/ > ----------------------------------------------------------- > > (Updated May 21, 2019, 1:54 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > ------- > > This patch introduces a class with mock methods for subscribing to the > events of master's V1 streaming API and setting expectations for them. > > > Diffs > ----- > > src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f > src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 > src/tests/mock_master_api_subscriber.hpp PRE-CREATION > src/tests/mock_master_api_subscriber.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70671/diff/2/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
