----------------------------------------------------------- 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) <https://reviews.apache.org/r/58720/#comment247229> 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) <https://reviews.apache.org/r/58720/#comment247230> 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) <https://reviews.apache.org/r/58720/#comment247231> This isn't an executable file. This is a module. src/cli_new/lib/cli/tests/base.py Lines 31 (patched) <https://reviews.apache.org/r/58720/#comment247235> Newline above this. src/cli_new/lib/cli/tests/base.py Lines 33-38 (patched) <https://reviews.apache.org/r/58720/#comment247233> 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) <https://reviews.apache.org/r/58720/#comment247234> Newline above this. src/cli_new/lib/cli/tests/base.py Lines 48 (patched) <https://reviews.apache.org/r/58720/#comment247236> 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) <https://reviews.apache.org/r/58720/#comment247237> 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) <https://reviews.apache.org/r/58720/#comment247238> 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) <https://reviews.apache.org/r/58720/#comment247239> This should use the defined constants. src/cli_new/lib/cli/tests/base.py Lines 184-186 (patched) <https://reviews.apache.org/r/58720/#comment247240> ditto on comments from above. src/cli_new/lib/cli/tests/base.py Lines 198 (patched) <https://reviews.apache.org/r/58720/#comment247242> 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) <https://reviews.apache.org/r/58720/#comment247241> Can we use `.format()` syntax here? src/cli_new/lib/cli/tests/base.py Lines 219-220 (patched) <https://reviews.apache.org/r/58720/#comment247243> This looks lik it might be able to fit on one line. src/cli_new/lib/cli/tests/base.py Lines 247 (patched) <https://reviews.apache.org/r/58720/#comment247244> s/launch task/launch a task/ src/cli_new/lib/cli/tests/base.py Lines 265-266 (patched) <https://reviews.apache.org/r/58720/#comment247245> 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) <https://reviews.apache.org/r/58720/#comment247246> timeout should use a constant src/cli_new/lib/cli/tests/base.py Lines 304 (patched) <https://reviews.apache.org/r/58720/#comment247247> timeout should use constant. src/cli_new/lib/cli/tests/base.py Lines 323 (patched) <https://reviews.apache.org/r/58720/#comment247248> we should use `.format()` syntax here. src/cli_new/lib/cli/tests/base.py Lines 384-401 (patched) <https://reviews.apache.org/r/58720/#comment247249> 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. - Kevin Klues 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 > >
