Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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



This needs rebasing after the changes in 59177

- Kevin Klues


On May 25, 2017, 9:08 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 25, 2017, 9:08 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/12/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [58719, 58720]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 25, 2017, 9:08 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 25, 2017, 9:08 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/12/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

(Updated May 25, 2017, 9:08 a.m.)


Review request for mesos and Kevin Klues.


Changes
---

Lambdas removed.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


Diff: https://reviews.apache.org/r/58720/diff/11/

Changes: https://reviews.apache.org/r/58720/diff/10-11/


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

> On May 25, 2017, 3:16 a.m., Kevin Klues wrote:
> > src/cli_new/lib/cli/tests/base.py
> > Lines 216 (patched)
> > 
> >
> > We need to be consistent here. If all functions in this file that we 
> > pass to `get_json` can be written as lambda's then we can write them all as 
> > lambdas (though I'd argue that it's less readable this way). 
> > 
> > If even one can't be written as a lambda (e.g. it requires an if 
> > statement inside of it), we should avoid using lambdas everywhere.

As a project, we tend to prefer consistency and readability over conciseness 
and efficiency (especially in places where it's not a huge bottleneck).


- Kevin


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


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/10/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 216 (patched)


We need to be consistent here. If all functions in this file that we pass 
to `get_json` can be written as lambda's then we can write them all as lambdas 
(though I'd argue that it's less readable this way). 

If even one can't be written as a lambda (e.g. it requires an if statement 
inside of it), we should avoid using lambdas everywhere.


- Kevin Klues


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/10/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 281-288 (patched)


This does not make sense as a standalone function in this class. The `data` 
parameter is meaningless outside of a call to `__wait_for_containers` and needs 
to be logically paried with it.

Either we build some abstraction around `__wait_for_containers` that has 
both `__wait_for_containers` and `__container_exists` inside of it, or we do 
what was being done previously, and define a one-off function just before 
calling `__wait_for_containers`.


- Kevin Klues


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/10/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 51 (patched)


you can remove this extra str()



src/cli_new/lib/cli/tests/base.py
Lines 312 (patched)


you can remove this extra str()


- Andrei Budnik


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/9/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 356 (patched)


container_does_not_exist = lambda data: not self.__container_exists(data)


- Andrei Budnik


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/7/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 269 (patched)


Please, use Task.task_id instead of self.task_id:
flags["name"] = "task-{id}".format(id=Task.task_id)
Task.task_id += 1

Otherwise every instance of Task(Executable) will have same task_id.


- Andrei Budnik


On May 24, 2017, 1:28 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 24, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/7/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

(Updated May 24, 2017, 1:28 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Solved the issues raised by Andrei.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


Diff: https://reviews.apache.org/r/58720/diff/7/

Changes: https://reviews.apache.org/r/58720/diff/6-7/


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [58719, 58720]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 263 (patched)


"{}:{}".format(DEFAULT_MASTER_IP, DEFAULT_MASTER_PORT)


- Andrei Budnik


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 265 (patched)


Is it possible to run multiple tasks? What will be in case of task-id 
collision?

It's possible to omit str() in '.format(id=str(random.random()))'. Also, 
random.random() returns floating point number in the range [0.0, 1.0).


- Andrei Budnik


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 333 (patched)


__wait_for_containers() might throw an exception like 

CLIException("Unable to parse agent info: {error}" 
.format(error=str(exception)))

But here the message inside the exception states that "Container not found" 
- so, a user of tests might be confused with this mixed message: was the 
container disappeared or was there some kind of misconfiguration error?

I would suggest message like "Wait for container failed: {error}{stdout}".


- Andrei Budnik


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 198 (patched)


virtualenv package might help. It can create sandbox for tests and clear it 
automatically.



src/cli_new/lib/cli/tests/base.py
Lines 214 (patched)


you can use lambda function 'single_slave', since it isn't used anywhere 
else.

single_slave = lambda data: len(data["slaves"]) == 1



src/cli_new/lib/cli/tests/base.py
Lines 321 (patched)


return any(container["executor_id"] == self.flags["name"] for container in 
data)

BTW, should we use lambda function here like I've mentioned above?



src/cli_new/lib/cli/tests/base.py
Lines 352 (patched)


Oh, I think we can use same function as container_exists() to avoid code 
copy-paste. And use lambda to make logical 'not':

container_does_not_exist = lambda data: not container_exists(data)



src/cli_new/lib/cli/tests/tests.py
Lines 60 (patched)


Consider using lambda function 'command' here.


- Andrei Budnik


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Andrei Budnik]

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




src/cli_new/lib/cli/tests/base.py
Lines 52 (patched)


I would prefer os.linesep to '\n' here and everywhere to avoid extra 
constant.


- Andrei Budnik


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58719, 58720]

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

- Mesos Reviewbot


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/6/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

> On May 7, 2017, 8:13 p.m., Kevin Klues wrote:
> > src/cli_new/lib/cli/constants.py
> > Lines 18-25 (patched)
> > 
> >
> > Aso is, I think we should bundle this under cli/tests/constants.py.
> > 
> > However, if you want to keep this file as cli/constants.py, that's 
> > fine, but the default ports should be 5050 and 5051.
> > 
> > I chose 9090 and 9091 for the tests so they wouldn't conflict with and 
> > mesos instances running on the (normal) default ports.
> > 
> > If you want to keep these constants all in one place, we should add 
> > separate default ports for the tests. Something like:
> > ```
> > class namespace:
> > def __init__(self, **entries): self.__dict__.update(entries)
> > 
> > DEFAULT_MASTER_IP = "127.0.0.1"
> > DEFAULT_MASTER_PORT = "9090"
> > 
> > DEFAULT_AGENT_IP = "127.0.0.1"
> > DEFAULT_AGENT_PORT = "9091"
> > 
> > test = namespace(
> >   DEFAULT_MASTER_IP = DEFAULT_MASTER_IP,
> >   DEFAULT_MASTER_PORT = "9090",
> > 
> >   DEFAULT_AGENT_IP = DEFAULT_AGENT_IP
> >   DEFAULT_AGENT_PORT = "9091"
> > )
> > ```
> > 
> > Though I'd probably put the namespace function into `cli/util.py`

Having constants with different values depending on the namespace seems 
cumbersome. I will put the constants in this review request back under 
cli/tests/constants.py and create cli/constants.py in a future review request.


> On May 7, 2017, 8:13 p.m., Kevin Klues wrote:
> > src/cli_new/lib/cli/tests/base.py
> > Lines 139 (patched)
> > 
> >
> > The parameters to this function should not mix having 2 parameters on 
> > one line and just one on the next.
> > 
> > Either choose:
> > ```
> > self.executable = os.path.join(mesos_build_path(),
> >"bin",
> >"mesos-{name}.sh".format(name=self.name))
> >  
> > ```
> > 
> > or
> > ```
> > self.executable = os.path.join(
> > mesos_build_path(),
> > "bin",
> > "mesos-{name}.sh".format(name=self.name))
> > ```
> > 
> > I typically prefer the second.

I have used the first solution as it is everywhere like this right now.


> On May 7, 2017, 8:13 p.m., Kevin Klues wrote:
> > src/cli_new/lib/cli/tests/base.py
> > Lines 384-401 (patched)
> > 
> >
> > This is fine for now, but I'm wondering if we can't find a more generic 
> > way of discovering the build directorty.  This current method assumes we 
> > always create the build directory in a specific location.
> > 
> > It also assumes we have a build directory at all. Which maybe we should 
> > safeguard againts.

True, we could add an option like for our configure script with 
`--with-mesos-build-dir`. Our script mesos-cli-tests is currently small and 
supporting long command line options is gonna require a lot of code (it does 
not work out of the box on macOS). The easiest solution would be to have a 
short command line option to specify your build path (e.g. `mesos-cli-tests 
-b=path/to/mesos/build/dir`).


- Armand


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


On May 8, 2017, 5:27 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 8, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/5/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel 

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

(Updated May 8, 2017, 5:27 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Changed code following the issues raised.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


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

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


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/__init__.py
Line 18 (original), 18 (patched)


This should be called Mesos CLI module since the renaming. This shouldn't 
be bundled into this patch though. It should be it's own isolated one.



src/cli_new/lib/cli/constants.py
Lines 18-25 (patched)


Aso is, I think we should bundle this under cli/tests/constants.py.

However, if you want to keep this file as cli/constants.py, that's fine, 
but the default ports should be 5050 and 5051.

I chose 9090 and 9091 for the tests so they wouldn't conflict with and 
mesos instances running on the (normal) default ports.

If you want to keep these constants all in one place, we should add 
separate default ports for the tests. Something like:
```
class namespace:
def __init__(self, **entries): self.__dict__.update(entries)

DEFAULT_MASTER_IP = "127.0.0.1"
DEFAULT_MASTER_PORT = "9090"

DEFAULT_AGENT_IP = "127.0.0.1"
DEFAULT_AGENT_PORT = "9091"

test = namespace(
  DEFAULT_MASTER_IP = DEFAULT_MASTER_IP,
  DEFAULT_MASTER_PORT = "9090",

  DEFAULT_AGENT_IP = DEFAULT_AGENT_IP
  DEFAULT_AGENT_PORT = "9091"
)
```

Though I'd probably put the namespace function into `cli/util.py`



src/cli_new/lib/cli/tests/__init__.py
Lines 18 (patched)


This isn't an executable file. This is a module.



src/cli_new/lib/cli/tests/base.py
Lines 31 (patched)


Newline above this.



src/cli_new/lib/cli/tests/base.py
Lines 33-38 (patched)


Assuming you follow my suggestion from above. This should look like:
```
from cli.constants.test import DEFAULT_MASTER_IP
from cli.constants.test import DEFAULT_MASTER_PORT
from cli.constants.test import DEFAULT_AGENT_IP
from cli.constants.test import DEFAULT_AGENT_PORT
```

I prefer to be explicit on each line about what we are importing, rather 
than use the `()` syntax.



src/cli_new/lib/cli/tests/base.py
Lines 39 (patched)


Newline above this.



src/cli_new/lib/cli/tests/base.py
Lines 48 (patched)


I may have missed this in teh first round of reviews. This should use the 
`.format()` syntax.



src/cli_new/lib/cli/tests/base.py
Lines 139 (patched)


The parameters to this function should not mix having 2 parameters on one 
line and just one on the next.

Either choose:
```
self.executable = os.path.join(mesos_build_path(),
   "bin",
   "mesos-{name}.sh".format(name=self.name))
 
```

or
```
self.executable = os.path.join(
mesos_build_path(),
"bin",
"mesos-{name}.sh".format(name=self.name))
```

I typically prefer the second.



src/cli_new/lib/cli/tests/base.py
Lines 141 (patched)


Can you move this up to just below:
```
self.name = "master"
```

It will look cleaner this way



src/cli_new/lib/cli/tests/base.py
Lines 176 (patched)


This should use the defined constants.



src/cli_new/lib/cli/tests/base.py
Lines 184-186 (patched)


ditto on comments from above.



src/cli_new/lib/cli/tests/base.py
Lines 198 (patched)


We should probably define this timeout either in the contants file or at 
the top of this file (my preference is at the top of this file).



src/cli_new/lib/cli/tests/base.py
Lines 214 (patched)


Can we use `.format()` syntax here?



src/cli_new/lib/cli/tests/base.py
Lines 219-220 (patched)


This looks lik it might be able to fit on one line.



src/cli_new/lib/cli/tests/base.py
Lines 247 (patched)


s/launch task/launch a task/



src/cli_new/lib/cli/tests/base.py
Lines 265-266 (patched)


as before, we shouldn't have two paramters on one line and just one on the 
next.



src/cli_new/lib/cli/tests/base.py
Lines 268 (patched)


timeout should use a constant

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58719, 58720]

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

- Mesos Reviewbot


On May 5, 2017, 5:02 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 5, 2017, 5:02 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
>   src/cli_new/lib/cli/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/4/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

(Updated May 5, 2017, 5:02 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Constants moved to be accessible from the CLI and not just the tests.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/__init__.py f4fc3f18af5641a4a87143adaba81e62334ccffb 
  src/cli_new/lib/cli/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


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

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


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58719, 58720]

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

- Mesos Reviewbot


On May 4, 2017, 1:59 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated May 4, 2017, 1:59 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/3/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

(Updated May 4, 2017, 1:59 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Fixed raised issues.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs (updated)
-

  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/constants.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


Diff: https://reviews.apache.org/r/58720/diff/2/

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


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 116-117 (patched)


We should not end strings with a space (i.e. "something "). The space 
should always begin on the next line. Also, we should try and balance the 
string across the two lines, i.e.:

```
if Master.count > 0:
raise CLIException("Creating more than one master"
   " is currently not possible")
```



src/cli_new/lib/cli/tests/base.py
Lines 165-168 (patched)


Let's pull these constants out into module variables.

```
DEFAULT_MASTER_IP = 127.0.0.1
DEFAULT_MASTER_PORT = 9090

DEFAULT_AGENT_IP = 127.0.0.1
DEFAULT_AGENT_PORT = 9091
```



src/cli_new/lib/cli/tests/base.py
Lines 177 (patched)


Here too.



src/cli_new/lib/cli/tests/base.py
Lines 257 (patched)


here too



src/cli_new/lib/cli/tests/base.py
Lines 386 (patched)


Let's use the `os.path.join` here.


- Kevin Klues


On April 25, 2017, 10:18 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated April 25, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/1/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 58720: CLI: Extended the unit test infrastructure.

[Kevin Klues]

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




src/cli_new/lib/cli/tests/base.py
Lines 133 (patched)


We should use the `.format` syntax here. Also, we should use `os.path.join`.


- Kevin Klues


On April 25, 2017, 10:18 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58720/
> ---
> 
> (Updated April 25, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7283
> https://issues.apache.org/jira/browse/MESOS-7283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This infrastructure includes the ability to bring up a test cluster to
> run the CLI against. Future unit tests will use this infrastructure to
> test their commands against a running mesos cluster. The tests require
> some binaries created when building Mesos.
> 
> 
> Diffs
> -
> 
>   src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/base.py PRE-CREATION 
>   src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
>   src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
>   src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 
> 
> 
> Diff: https://reviews.apache.org/r/58720/diff/1/
> 
> 
> Testing
> ---
> 
> PEP8 and Pylint used to make sure that the code style is correct. Manuel test:
> 
> $ cd src/cli_new
> $ ./bootstrap
> $ source activate
> 
> (mesos-cli) $ mesos-cli-tests
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 58720: CLI: Extended the unit test infrastructure.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

This infrastructure includes the ability to bring up a test cluster to
run the CLI against. Future unit tests will use this infrastructure to
test their commands against a running mesos cluster. The tests require
some binaries created when building Mesos.


Diffs
-

  src/cli_new/lib/cli/tests/__init__.py PRE-CREATION 
  src/cli_new/lib/cli/tests/base.py PRE-CREATION 
  src/cli_new/lib/cli/tests/tests.py PRE-CREATION 
  src/cli_new/pip-requirements.txt 28613e56a5c6d5c7606a7e58d6125b0c34748e83 
  src/cli_new/tests/main.py dff5d48b0ddae87960a78f9d05e4ae597912f1f6 


Diff: https://reviews.apache.org/r/58720/diff/1/


Testing
---

PEP8 and Pylint used to make sure that the code style is correct. Manuel test:

$ cd src/cli_new
$ ./bootstrap
$ source activate

(mesos-cli) $ mesos-cli-tests


Thanks,

Armand Grillet