Re: Review Request 45670: Added tests for HTTP command executor.

[Qian Zhang]

> On April 13, 2016, 5:11 a.m., Vinod Kone wrote:
> > src/tests/command_executor_tests.cpp, line 67
> > 
> >
> > This is not really a type, this is a bool.
> > 
> > s/ExecutorType/IsHTTPCommandExecutor/

Thanks for the comment! And I think we do not need `Is`, just 
`HTTPCommandExecutor` should be OK, because I see the registrar 
tests(https://github.com/apache/mesos/blob/0.28.0/src/tests/registrar_tests.cpp#L196)
 also uses a bool parameter and names it as `Strict` rather than `IsStrict`.


- Qian


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


On April 9, 2016, 4:48 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 9, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Vinod Kone]

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



Are you also planning to update slave recovery tests? Those are the most 
crucial.


src/tests/command_executor_tests.cpp (line 67)


This is not really a type, this is a bool.

s/ExecutorType/IsHTTPCommandExecutor/


- Vinod Kone


On April 9, 2016, 8:48 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 9, 2016, 8:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [45670, 44427, 44424, 44423]

Failed command: ./support/apply-review.sh -n -r 44423

Error:
2016-04-09 21:02:50 URL:https://reviews.apache.org/r/44423/diff/raw/ 
[30696/30696] -> "44423.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/12429/console

- Mesos ReviewBot


On April 9, 2016, 8:48 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 9, 2016, 8:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Anand Mazumdar]

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


Ship it!




LGTM

- Anand Mazumdar


On April 9, 2016, 8:48 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 9, 2016, 8:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Qian Zhang]

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

(Updated April 9, 2016, 4:48 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added tests for HTTP command executor.


Diffs (updated)
-

  src/tests/command_executor_tests.cpp 843233adaa68ab1f5cedb7b075439bb8b77469a8 

Diff: https://reviews.apache.org/r/45670/diff/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45670: Added tests for HTTP command executor.

[Anand Mazumdar]

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



Looks good. Just some minor comments about handling the parameterization in 
test body.


src/tests/command_executor_tests.cpp (line 62)


How about:

```cpp
// The Command Executor tests are parameterized by the underlying library 
used by the executor (e.g., driver or the HTTP based executor library).
```



src/tests/command_executor_tests.cpp (line 64)


s/HttpBasedExecutor/ExecutorType



src/tests/command_executor_tests.cpp (line 78)


How about:

```cpp
slave::Flags flags = CreateSlaveFlags();
flags.http_command_executor = GetParam();

Try slave = StartSlave(detector.get(), flags);
ASSERT_SOME(slave);
```



src/tests/command_executor_tests.cpp (lines 80 - 85)


Kill this



src/tests/command_executor_tests.cpp (line 148)


Ditto as above.



src/tests/command_executor_tests.cpp (lines 150 - 155)


Kill this


- Anand Mazumdar


On April 7, 2016, 1:49 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 7, 2016, 1:49 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 970cdc39f4f2b0377d36acf2465d377d2a6e1d05 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [45670, 44427, 44424, 44423]

Failed command: ./support/apply-review.sh -n -r 44423

Error:
2016-04-07 14:18:54 URL:https://reviews.apache.org/r/44423/diff/raw/ 
[30696/30696] -> "44423.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/12381/console

- Mesos ReviewBot


On April 7, 2016, 1:49 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 7, 2016, 1:49 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 970cdc39f4f2b0377d36acf2465d377d2a6e1d05 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Qian Zhang]

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

(Updated April 7, 2016, 9:49 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added tests for HTTP command executor.


Diffs (updated)
-

  src/tests/command_executor_tests.cpp 970cdc39f4f2b0377d36acf2465d377d2a6e1d05 

Diff: https://reviews.apache.org/r/45670/diff/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45670: Added tests for HTTP command executor.

[Qian Zhang]

> On April 5, 2016, 3:07 a.m., Anand Mazumdar wrote:
> > hmm.. Would it be possible to not create a new file just for these two 
> > tests? IIUC, the existing tests in `src/tests/command_executor_tests.cpp` 
> > can be easily parameterized by value. The parameterization argument can be 
> > a boolean flag to the tests to enable/disable the HTTP based executor. You 
> > can look at some possible examples in `SlaveRecoveryTest`/`SchedulerTest` 
> > for possible examples.
> > 
> > In the near future, we would like to make 
> > `flags.http_command_executor=true` as the default i.e. always run our test 
> > suite with this option set to `true`. I did a quick run using your patches 
> > and there would be around ~20 tests mostly slave recovery tests that would 
> > need their expectations modified. We can do that as part of a follow up 
> > JIRA later and not worry about them for now.

Agree! Let me update the existing tests in 
`src/tests/command_executor_tests.cpp` by parameterizing it.


- Qian


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


On April 4, 2016, 5:12 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 4, 2016, 5:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/http_command_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Anand Mazumdar]

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



hmm.. Would it be possible to not create a new file just for these two tests? 
IIUC, the existing tests in `src/tests/command_executor_tests.cpp` can be 
easily parameterized by value. The parameterization argument can be a boolean 
flag to the tests to enable/disable the HTTP based executor. You can look at 
some possible examples in `SlaveRecoveryTest`/`SchedulerTest` for possible 
examples.

In the near future, we would like to make `flags.http_command_executor=true` as 
the default i.e. always run our test suite with this option set to `true`. I 
did a quick run using your patches and there would be around ~20 tests mostly 
slave recovery tests that would need their expectations modified. We can do 
that as part of a follow up JIRA later and not worry about them for now.

- Anand Mazumdar


On April 4, 2016, 9:12 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 4, 2016, 9:12 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/http_command_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45670: Added tests for HTTP command executor.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44423, 44424, 44427, 45670]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 4, 2016, 9:12 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 4, 2016, 9:12 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/http_command_executor_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 45670: Added tests for HTTP command executor.

[Qian Zhang]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added tests for HTTP command executor.


Diffs
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/http_command_executor_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/45670/diff/


Testing
---


Thanks,

Qian Zhang