> On May 7, 2017, 8:13 p.m., Kevin Klues wrote: > > src/cli_new/lib/cli/constants.py > > Lines 18-25 (patched) > > <https://reviews.apache.org/r/58720/diff/4/?file=1709419#file1709419line18> > > > > 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) > > <https://reviews.apache.org/r/58720/diff/4/?file=1709421#file1709421line139> > > > > 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) > > <https://reviews.apache.org/r/58720/diff/4/?file=1709421#file1709421line384> > > > > 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 test: > > $ cd src/cli_new > $ ./bootstrap > $ source activate > > (mesos-cli) $ mesos-cli-tests > > > Thanks, > > Armand Grillet > >
