Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > Looks good, thanks!
> > 
> > I will make some adjustments prior to committing, the biggest is to remove 
> > the need for the nullptr assingment and loop check, by using finalize() 
> > instead of shutdown(). Also will add a receiveLoop future that we discard 
> > when finalizing:
> > 
> > ```
> > diff --git a/src/tests/master/mock_master_api_subscriber.cpp 
> > b/src/tests/master/mock_master_api_subscriber.cpp
> > index 8c49c1af5..a1cd53844 100644
> > --- a/src/tests/master/mock_master_api_subscriber.cpp
> > +++ b/src/tests/master/mock_master_api_subscriber.cpp
> > @@ -44,7 +44,7 @@ class MockMasterAPISubscriberProcess
> >  {
> >  public:
> >MockMasterAPISubscriberProcess(MockMasterAPISubscriber* subscriber_)
> > -: Process(), subscriber(subscriber_){};
> > +: subscriber(subscriber_) {};
> >  
> >Future subscribe(
> >  const process::PID& pid, ContentType contentType)
> > @@ -64,15 +64,13 @@ public:
> >.then(defer(self(), ::_subscribe, lambda::_1, contentType));
> >}
> >  
> > -  Nothing shutdown()
> > +protected:
> > +  void finalize() override
> >{
> > -subscriber = nullptr;
> > -return Nothing();
> > +receiveLoop.discard();
> >}
> >  
> >  private:
> > -  MockMasterAPISubscriber* subscriber;
> > -
> >Future _subscribe(
> >  const process::Future& response,
> >  ContentType contentType)
> > @@ -106,7 +104,7 @@ private:
> >  [](std::unique_ptr>& d) { return d->read(); },
> >  std::move(reader));
> >  
> > -process::loop(
> > +receiveLoop = process::loop(
> >  self(),
> >  std::move(decode),
> >  [this](const Result& event) -> 
> > process::ControlFlow {
> > @@ -123,17 +121,21 @@ private:
> >LOG(INFO) << "Received " << event->type()
> >  << " event from master streaming API";
> >  
> > -  if (subscriber != nullptr) {
> > -subscriber->handleEvent(event.get());
> > -  }
> > -
> > +  subscriber->handleEvent(event.get());
> >return process::Continue();
> >  });
> >  
> >  LOG(INFO) << "Subscribed to master streaming API events";
> >  
> > +receiveLoop.onAny([]() {
> > +  LOG(INFO) << "Stopped master streaming API receive loop";
> > +});
> > +
> >  return Nothing();
> >}
> > +
> > +  MockMasterAPISubscriber* subscriber;
> > +  Future receiveLoop;
> >  };
> >  
> >  
> > @@ -175,12 +177,14 @@ MockMasterAPISubscriber::MockMasterAPISubscriber()
> >  
> >  MockMasterAPISubscriber::~MockMasterAPISubscriber()
> >  {
> > -  process::Future shutdown =
> > -dispatch(process, ::shutdown);
> > -
> > -  shutdown.get();
> > -
> > -  terminate(process);
> > +  process::terminate(process);
> > +
> > +  // The process holds a pointer to this object, and so
> > +  // we must ensure it won't access the pointer before
> > +  // we exit the destructor.
> > +  //
> > +  // TODO(asekretenko): Figure out a way to avoid blocking.
> > +  process::wait(process);
> >  }
> >  
> >  
> > 
> > ```

Thanks for cleaning this up! 
There really were some leftovers of the workaround for the libprocess deadlock, 
I should have removed them myself.


However, I do not fully understand one of your changes: discarding the 
`receiveLoop` future. 
While running the tests, I'm observing that:
- the onAny callback of `receiveLoop` is not called (at least typically)
- removing `receiveLoop.discard()` does not affect the tests (at least, the 
typical runs)
- if, before terminating this process, I perform a discard+await on this 
future, the await hangs. (If I understand the code of the `Reader` correctly, 
the reason behind the hang is that the promise which satisfies the future 
returned by `reader->read()` is never discarded.)

So I'm left puzzled - why is it needed to discard this future?... Is it some 
corner case which does not show up under typical conditions, or something else?


> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > src/tests/master/mock_master_api_subscriber.cpp
> > Lines 171-172 (patched)
> > 
> >
> > Why do we need the pointer? We can use a PID?
> > 
> > ```
> >   process = spawn(new MockMasterAPISubscriberProcess(this), true);
> > ```
> > 
> > That way, we also don't have the strangeness of holding a pointer to a 
> > Process that is managed (which I don't think is done anywhere in our code).
> > 
> > Is it because `PID` doesn't work with 
> > it forward declared?

You are right, there is no need to store the pointer. The follow-up patch: 
https://reviews.apache.org/r/70843/


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215771
---


On June 7, 

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215771
---


Fix it, then Ship it!




Looks good, thanks!

I will make some adjustments prior to committing, the biggest is to remove the 
need for the nullptr assingment and loop check, by using finalize() instead of 
shutdown(). Also will add a receiveLoop future that we discard when finalizing:

```
diff --git a/src/tests/master/mock_master_api_subscriber.cpp 
b/src/tests/master/mock_master_api_subscriber.cpp
index 8c49c1af5..a1cd53844 100644
--- a/src/tests/master/mock_master_api_subscriber.cpp
+++ b/src/tests/master/mock_master_api_subscriber.cpp
@@ -44,7 +44,7 @@ class MockMasterAPISubscriberProcess
 {
 public:
   MockMasterAPISubscriberProcess(MockMasterAPISubscriber* subscriber_)
-: Process(), subscriber(subscriber_){};
+: subscriber(subscriber_) {};
 
   Future subscribe(
 const process::PID& pid, ContentType contentType)
@@ -64,15 +64,13 @@ public:
   .then(defer(self(), ::_subscribe, lambda::_1, contentType));
   }
 
-  Nothing shutdown()
+protected:
+  void finalize() override
   {
-subscriber = nullptr;
-return Nothing();
+receiveLoop.discard();
   }
 
 private:
-  MockMasterAPISubscriber* subscriber;
-
   Future _subscribe(
 const process::Future& response,
 ContentType contentType)
@@ -106,7 +104,7 @@ private:
 [](std::unique_ptr>& d) { return d->read(); },
 std::move(reader));
 
-process::loop(
+receiveLoop = process::loop(
 self(),
 std::move(decode),
 [this](const Result& event) -> process::ControlFlow {
@@ -123,17 +121,21 @@ private:
   LOG(INFO) << "Received " << event->type()
 << " event from master streaming API";
 
-  if (subscriber != nullptr) {
-subscriber->handleEvent(event.get());
-  }
-
+  subscriber->handleEvent(event.get());
   return process::Continue();
 });
 
 LOG(INFO) << "Subscribed to master streaming API events";
 
+receiveLoop.onAny([]() {
+  LOG(INFO) << "Stopped master streaming API receive loop";
+});
+
 return Nothing();
   }
+
+  MockMasterAPISubscriber* subscriber;
+  Future receiveLoop;
 };
 
 
@@ -175,12 +177,14 @@ MockMasterAPISubscriber::MockMasterAPISubscriber()
 
 MockMasterAPISubscriber::~MockMasterAPISubscriber()
 {
-  process::Future shutdown =
-dispatch(process, ::shutdown);
-
-  shutdown.get();
-
-  terminate(process);
+  process::terminate(process);
+
+  // The process holds a pointer to this object, and so
+  // we must ensure it won't access the pointer before
+  // we exit the destructor.
+  //
+  // TODO(asekretenko): Figure out a way to avoid blocking.
+  process::wait(process);
 }
 
 

```


src/tests/master/mock_master_api_subscriber.cpp
Lines 47 (patched)


Why was the explicit parent constructor call needed?

s/){}/) {}/



src/tests/master/mock_master_api_subscriber.cpp
Lines 67-71 (patched)


This is called "shutdown" but it doesn't shut anything down. The loop still 
runs when it encounters 'nullptr' and we don't discard the loop future, it 
would be good to stop the loop here and also have the loop break if it sees 
nullptr.

E.g.

```
// TODO(asekretenko): Ideally this function returns the receive
// loop future so that shutdown completes when the receive
// loop is stopped. That way, we also wouldn't need to assign
// `subscriber` to nullptr and check it in the loop.
receiveLoop.discard();
```

Also, this can just be done within `finalize()` and the caller can 
`process::wait()` for it?

Actually, if we take the `finalize()` approach, we don't need to assign to 
null pointer and check it.



src/tests/master/mock_master_api_subscriber.cpp
Lines 126-130 (patched)


Per the above comment, we should break when we encounter the nullptr?

```
  if (subscriber == nullptr) {
return process::Break();
  }

  subscriber->handleEvent(event.get());
  return process::Continue();
```

(Actually, we don't need this if we use finalize() instead of adding 
shutdown()).



src/tests/master/mock_master_api_subscriber.cpp
Lines 171-172 (patched)


Why do we need the pointer? We can use a PID?

```
  process = spawn(new MockMasterAPISubscriberProcess(this), true);
```

That way, we also don't have the strangeness of holding a pointer to a 
Process that is managed (which I don't think is done anywhere in our code).

Is it because `PID` 

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated June 7, 2019, 4:21 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


Changes
---

Removed workaround for libprocess deadlock (which was fixed in MESOS-9808).


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


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 (updated)
-

  src/Makefile.am 93b2606841745c74dc33713e6b1b2fd62a1c7e74 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/6/

Changes: https://reviews.apache.org/r/70671/diff/5-6/


Testing
---

`./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" 
--gtest_break_on_failure --gtest_repeat=1000` with new tests from 
https://reviews.apache.org/r/70534/

`./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" 
--gtest_break_on_failure --gtest_repeat=1000` with refactored tests from 
https://reviews.apache.org/r/70756/


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated June 3, 2019, 8:28 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


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/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/5/


Testing (updated)
---

`./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" 
--gtest_break_on_failure --gtest_repeat=1000` with new tests from 
https://reviews.apache.org/r/70534/

`./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" 
--gtest_break_on_failure --gtest_repeat=1000` with refactored tests from 
https://reviews.apache.org/r/70756/


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated June 3, 2019, 8:22 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


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/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/5/


Testing (updated)
---

./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" 
--gtest_break_on_failure --gtest_repeat=1000 with new tests from 
https://reviews.apache.org/r/70534/

./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" 
--gtest_break_on_failure --gtest_repeat=1000 with refactored tests from 
https://reviews.apache.org/r/70756/


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

> On May 29, 2019, 6:02 p.m., Benjamin Mahler wrote:
> > src/tests/master/mock_master_api_subscriber.cpp
> > Lines 131-134 (patched)
> > 
> >
> > If possible, we prefer to use the "managed=true" approach to avoid 
> > self-wait deadlocks:
> > 
> > ```
> > PID process =
> >   spawn(new MockMasterAPISubscriberProcess(this), true);
> >   
> > ...
> > 
> > terminate(process);
> > // no need to wait
> > ```
> > 
> > However, it looks like that's not possible here since the process holds 
> > a back pointer to `this`? Perhaps it's worth noting that:
> > 
> > ```
> >   // Note that we must wait for the process to terminate before we leave
> >   // the destructor (and therefore cannot spawn the process with 
> > `managed=true`)
> >   // since the process may invoke the mock methods on this class.
> >   terminate(process.get());
> >   wait(process.get());
> > ```
> 
> Andrei Sekretenko wrote:
> Hmm... That's a good question. 
> I've recently (after refactoring a couple of existing master API tests) 
> started to run into some deadlock on termination once in a while. Probably 
> that's what's going on - I'll have a look.

I've converted the code to use a managed process - however, this didn't help as 
this deadlock was not related to self-wait. See 
https://issues.apache.org/jira/browse/MESOS-9808.

The deadlock was being triggered by terminating the Reader's internal process 
while hoilding a ProcessReference to the MockMasterAPISubscriberProcess (in one 
thread) and performing cleanup of MockMasterAPISubscriberProcess (in another 
thread).

I've implemented a workaround: now the MockMasterAPISubscriberProcess is 
terminated only after the loop (which owns the Reader) exits.


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215579
---


On June 3, 2019, 7:55 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated June 3, 2019, 7:55 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> 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/master/mock_master_api_subscriber.hpp PRE-CREATION 
>   src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70671/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated June 3, 2019, 7:55 p.m.)


Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.


Changes
---

Implemented workaround to avoid libprocess deadlock; fixed other remaining 
issues.


Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258


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 (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/5/

Changes: https://reviews.apache.org/r/70671/diff/4-5/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benno Evers]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215617
---


Fix it, then Ship it!




This looks pretty sweet, thanks for going the extra mile and improving our unit 
test suite!


src/tests/master/mock_master_api_subscriber.cpp
Lines 80 (patched)


Nit: No `.` at the end of log messages.

(same for the messages below)


- Benno Evers


On May 29, 2019, 7:17 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated May 29, 2019, 7:17 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> 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/master/mock_master_api_subscriber.hpp PRE-CREATION 
>   src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70671/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

> On May 29, 2019, 6:02 p.m., Benjamin Mahler wrote:
> > src/tests/master/mock_master_api_subscriber.cpp
> > Lines 131-134 (patched)
> > 
> >
> > If possible, we prefer to use the "managed=true" approach to avoid 
> > self-wait deadlocks:
> > 
> > ```
> > PID process =
> >   spawn(new MockMasterAPISubscriberProcess(this), true);
> >   
> > ...
> > 
> > terminate(process);
> > // no need to wait
> > ```
> > 
> > However, it looks like that's not possible here since the process holds 
> > a back pointer to `this`? Perhaps it's worth noting that:
> > 
> > ```
> >   // Note that we must wait for the process to terminate before we leave
> >   // the destructor (and therefore cannot spawn the process with 
> > `managed=true`)
> >   // since the process may invoke the mock methods on this class.
> >   terminate(process.get());
> >   wait(process.get());
> > ```

Hmm... That's a good question. 
I've recently (after refactoring a couple of existing master API tests) started 
to run into some deadlock on termination once in a while. Probably that's 
what's going on - I'll have a look.


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215579
---


On May 29, 2019, 7:17 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated May 29, 2019, 7:17 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> 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/master/mock_master_api_subscriber.hpp PRE-CREATION 
>   src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70671/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated May 29, 2019, 6:26 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Fixed hardcoded ContentType


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 (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/4/

Changes: https://reviews.apache.org/r/70671/diff/3-4/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215579
---



Overall this is looking pretty good, thanks Andrei!


src/Makefile.am
Lines 2615-2616 (patched)


alphabetical?

Can you line up the backslashes with the other lines?



src/tests/CMakeLists.txt
Lines 55 (patched)


alphabetical?



src/tests/master/mock_master_api_subscriber.hpp
Lines 72-73 (patched)


let's not mention the implementation detail of the libprocess loop here



src/tests/master/mock_master_api_subscriber.hpp
Lines 78-80 (patched)


Can we introduce a little bit of state to prevent this more than 1 call to 
subscribe?



src/tests/master/mock_master_api_subscriber.cpp
Lines 51-57 (patched)


if you use .then you can return directly and won't need the CHECK(!failed):

```
return process::http::streaming::post(
pid,
"api/v1",
headers,
serialize(contentType, call),
stringify(contentType))
  .then(defer(self(), ::_subscribe, lambda::_1));
```

See comment below, this lets us then surface errors to the test and the 
test can wait and ASSERT for a correct connection rather than having the 
implicit CHECKs crash the whole test binary.



src/tests/master/mock_master_api_subscriber.cpp
Lines 65-68 (patched)


Rather than crashing the program here, let's return a failure to the caller 
of subscribe if there's an error. And the test will ASSERT that the future 
coming out of subscribe succeeded.



src/tests/master/mock_master_api_subscriber.cpp
Lines 82 (patched)


This seems a bit hard for the reader, consider:

```
Owned> reader = ...;

process::loop(
self(),
[=]() {
  return reader->read();
},
```



src/tests/master/mock_master_api_subscriber.cpp
Lines 85 (patched)


"Received EOF from master streaming API"

no periods in log messages



src/tests/master/mock_master_api_subscriber.cpp
Lines 89-92 (patched)


Can you put this first before EOF? That would be more consistent with how 
we tend to handle errors first



src/tests/master/mock_master_api_subscriber.cpp
Lines 90 (patched)


Failed to read master streaming API event: ...



src/tests/master/mock_master_api_subscriber.cpp
Lines 94 (patched)


Isn't this going to print the number rather than the string? (i.e. 1 
instead of ENUM_NAME)

Or do we have a << overload?

"Received SUBSCRIBE from master streaming API" (i.e. a bit more consistent 
with the EOF logging, see suggestion above)



src/tests/master/mock_master_api_subscriber.cpp
Lines 131-134 (patched)


If possible, we prefer to use the "managed=true" approach to avoid 
self-wait deadlocks:

```
PID process =
  spawn(new MockMasterAPISubscriberProcess(this), true);
  
...

terminate(process);
// no need to wait
```

However, it looks like that's not possible here since the process holds a 
back pointer to `this`? Perhaps it's worth noting that:

```
  // Note that we must wait for the process to terminate before we leave
  // the destructor (and therefore cannot spawn the process with 
`managed=true`)
  // since the process may invoke the mock methods on this class.
  terminate(process.get());
  wait(process.get());
```



src/tests/master/mock_master_api_subscriber.cpp
Lines 141-143 (patched)


Can you remove the logging here?



src/tests/master/mock_master_api_subscriber.cpp
Lines 162-165 (patched)


Can you put this case at the bottom and remove the UNREACHABLE()? (We 
usually treat LOG(FATAL) as terminating without adding UNREACHABLE to guard 
against it somehow not terminating).


- Benjamin Mahler


On May 28, 2019, 5:35 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated May 28, 2019, 5:35 p.m.)
> 
> 
> Review request for 

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

> On May 24, 2019, 2:54 p.m., Benjamin Mahler wrote:
> > src/tests/mock_master_api_subscriber.hpp
> > Lines 60-63 (patched)
> > 
> >
> > 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).

Added all the existing events and removed the defaults in switches, as you 
suggesteded.

Actually, the `_T` is not needed (and this is not the intended use of `_T`, so 
I replaced them with non-`_T` macros. It turns out that the part of 
`MOCK_METHOD*` macro which adds the method declaration, uses neither `virtual` 
nor `override`.

Calling this a "Mock" is objectionable since it mocks nothing, but I cannot 
come up with a better name for something that has gmock's mock methods and is 
intended for use with gmock's API.


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215497
---


On May 28, 2019, 5:35 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> ---
> 
> (Updated May 28, 2019, 5:35 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/master/mock_master_api_subscriber.hpp PRE-CREATION 
>   src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70671/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
---

(Updated May 28, 2019, 5:35 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

- added mock methods for all event types
- fixed the lifetime issues by using the common process+wrapper pattern 
(destroying a mock terminates the process first)


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 (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70671/diff/3/

Changes: https://reviews.apache.org/r/70671/diff/2-3/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benjamin Mahler]

> On May 24, 2019, 2:54 p.m., Benjamin Mahler wrote:
> > 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?

Actually, since you have a master/ folder in the subsequent patch, consider 
placing these files in that folder.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215497
---


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
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Benjamin Mahler]

---
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)


```
EXPECT_CALL(*this, other(testing::_))
  .WillRepeatedly(testing::Return());
```



src/tests/mock_master_api_subscriber.hpp
Lines 60-63 (patched)


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)


newline



src/tests/mock_master_api_subscriber.cpp
Lines 42 (patched)


newline



src/tests/mock_master_api_subscriber.cpp
Lines 48-50 (patched)


This can crash if `this` has been destructed, no?



src/tests/mock_master_api_subscriber.cpp
Lines 71-80 (patched)


This can crash if the subscriber is destructed, no?



src/tests/mock_master_api_subscriber.cpp
Lines 103-105 (patched)


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
> 
>

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

---
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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/70671/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko