> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/Makefile.am, line 1753
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134890#file1134890line1753>
> >
> >     This would be a perfect opportunity to add a config header, and not 
> > pass any more strings as command line flags. Are we sure really we liked 
> > the approach taken previously with`SOURCE_DIR` and `BUILD_DIR`? OK if you 
> > disagree, not an issue.

I'd be happy to do that as a separate Jira, but I think that this change is 
intrusive enough already.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.cpp, line 71
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134906#file1134906line71>
> >
> >     Should this be a JIRA issue?

I don't know whether there is even supposed to be a module directory. I removed 
the XXX because this seems to be intentional.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.hpp, line 38
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134905#file1134905line38>
> >
> >     These functions cannot promise to return *valid* paths; they should 
> > probably all return `Try<string>` instead. Please also check their impls.

Well they may return a valid path to something that doesn't exist, which is 
fine and will cause the test to fail. It might be reasonable to return a 
```Try``` and change the callers to do ```findX().get()```, which would 
```ABORT``` at the call-site rather than some way into the test.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.hpp, line 37
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134905#file1134905line37>
> >
> >     I think `getX` would be more in line with other places in the code base.

Agreed.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/memory_test_helper.cpp, line 197
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134892#file1134892line197>
> >
> >     Should probably be passed as an arg (`bool` or even better an `enum` 
> > value). You can always pick a default.

I am not sure that I follow what you mean here. Are you suggesting that the 
caller should specify where to search, based on the ```MESOS_INSTALL_TESTS``` 
define? If so, I don't think the callers should need to know that. The default 
needs to be consistent across a test run, which implies that specific tests 
should not be toggling this in an ad-hoc fashion.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.cpp, line 56
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134906#file1134906line56>
> >
> >     All the `findX` functions here (but maybe `findModuleDir`) seem very 
> > easy to use incorrectly. We might e.g., depending on what is in our build 
> > directory or in the install prefix load a mix of different versions.
> >     
> >     I would much prefer if we'd either load from one place or the other.
> >     
> >     Going down that path would probably also lead us to remove a lot of the 
> > duplication here, by e.g., calling once into `findModuleDir` and building 
> > up the more specialized functions from there.

Yes, forcing a blanket policy is exactly why I needed the ```findX``` family. 
There are only 2 scenarios that can cause inconsistent paths to tbe used: (1) 
if you run the tests from an installation and do not specify --build_dir=/none 
you could run tests from installed version A with helpers from built version B, 
and (2) if you nuke some parts of the build directory before running the tests.

I avoided forcing either one path or the other because it seemed reasonable to 
run "make check" before installing. It would be simpler to always force the 
tests to be installed before running in the ```--enable-tests-install``` case 
if we could agree that "make check" would fail in that case.

In general, I don't see these functions as easily composable. Some of them 
happen to produce the same path string, but there's no real relationship 
between the launcher directory and the test helper directory, for example.


> On Nov. 25, 2015, 2:36 p.m., Benjamin Bannier wrote:
> > src/tests/utils.cpp, line 64
> > <https://reviews.apache.org/r/40553/diff/1/?file=1134906#file1134906line64>
> >
> >     Here and in the following: I have the feeling explicitly passing in a 
> > flag (e.g., `bool searchInstallationDirectory`, or even clearer with an 
> > `enum` value) would make this easier to digest. All of these could have a 
> > default.

How would a test know whether to specify ```searchInstallationDirectory```? A 
global command-line flag?


- James


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


On Nov. 25, 2015, 2:09 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> -----------------------------------------------------------
> 
> (Updated Nov. 25, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Bugs: MESOS-3608
>     https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch enables the installation mesos-tests and its dependencies
> and helper tool. The goal is to allow operators to build a separate
> test package that can be run at deployment time to verify that Mesos
> works in the deployment environment.
> 
> Since the build directory is searched first, to run it on a host
> that has a build tree, you need to specify a non-existent tree:
> 
> ~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none
> 
> - Add --enable-tests-install
> - Fix mesos-tests gmock dependencies
> - Optionally install tests, helpers and test modules
> - Add utility helpers to find various test resources
> 
> Current test status:
> 
>   [==========] 784 tests from 106 test cases ran. (135304 ms total)
>   [  PASSED  ] 780 tests.
>   [  FAILED  ] 4 tests, listed below:
>   [  FAILED  ] ExamplesTest.TestFramework
>   [  FAILED  ] ExamplesTest.NoExecutorFramework
>   [  FAILED  ] ExamplesTest.EventCallFramework
>   [  FAILED  ] ExamplesTest.PersistentVolumeFramework
> 
> 
> Diffs
> -----
> 
>   configure.ac 8b28ac78eeb3e3b5905b411b4bc0db3ccf0f543f 
>   src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
>   src/tests/containerizer/launch_tests.cpp 
> bf6b7561bf9b5c06fd5d1101e2b2511776a12010 
>   src/tests/containerizer/memory_test_helper.cpp 
> 6abd29ba6f0d208a5f10f645977fec309334af0a 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> d1ff139b0674ea266a4d554de112094045add509 
>   src/tests/containerizer/ns_tests.cpp 
> ac3c9ff1767bd34d3257bb39effce789fae47252 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 91953389f793755770ff957b675936521b0e338a 
>   src/tests/environment.cpp c07d9f037f81bce315df7c818131efcb0b186595 
>   src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8 
>   src/tests/health_check_tests.cpp ff6275b19206b49eacb6761f3aeb58dd87651ade 
>   src/tests/mesos.cpp 766a51cddc8801d5e79188f80e9ce0828598c8b9 
>   src/tests/module.cpp edab0b37dcf0bd8e15d439726354039c1bbcd51f 
>   src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 
>   src/tests/oversubscription_tests.cpp 
> 0d0bf7e0b9a4028ed116e00b56d59b670510c5ce 
>   src/tests/script.cpp c9b04e67ce8176798412c0c17e436bbc01a94d70 
>   src/tests/slave_tests.cpp 7c9dcc6186a8cccb0eb30ff59914a41961e47293 
>   src/tests/utils.hpp 1497013f43428269aa1e3c63e166c303af00483e 
>   src/tests/utils.cpp 0578e63bc46ea79b0508e656d0793710484b21ef 
> 
> Diff: https://reviews.apache.org/r/40553/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> James Peach
> 
>

Reply via email to