mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Bannier" <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 40553: Enable mesos tests installation
Date Thu, 10 Dec 2015 15:55:48 GMT


> 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.
> 
> James Peach wrote:
>     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.

Agreed, let's not open up the possibility of callers making this even more complicated.


> 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.
> 
> James Peach wrote:
>     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.

Looking at the call sites I agree that just `string`s might be good enough.


> 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.
> 
> James Peach wrote:
>     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.

Yes, I see.


> 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.
> 
> James Peach wrote:
>     How would a test know whether to specify ```searchInstallationDirectory```? A global
command-line flag?

One could set the value of a static const bool variable in this TU depending on whether `MESOS_INSTALL_TESTS`
was defined. The benefit would be that no matter whether it was there or not, any compile
would check all branches in these functions (but very likely optimize away the dead branch).


- Benjamin


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


On Nov. 30, 2015, 6:43 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> -----------------------------------------------------------
> 
> (Updated Nov. 30, 2015, 6:43 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 b30a8d30076f3068fd7d5fc8ccea1982639e998a 
>   src/Makefile.am fd38cfa73d81a98c819378f99a766e2ddb7e1a04 
>   src/tests/containerizer/launch_tests.cpp c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
>   src/tests/containerizer/memory_test_helper.cpp 4a3de2e3c887aa6afc604588850e1386f92d8c11

>   src/tests/containerizer/mesos_containerizer_tests.cpp fe679354d04d68b68e168cd8c4eab23898f6532f

>   src/tests/containerizer/ns_tests.cpp 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
>   src/tests/containerizer/port_mapping_tests.cpp 9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0

>   src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
>   src/tests/fetcher_tests.cpp 99c494a2167ea9ec94e63d92980df17a4089f95a 
>   src/tests/health_check_tests.cpp b1454b085b36bb7c4d8ef012c764cd8466b4fb02 
>   src/tests/mesos.cpp a75182ec7f6d0c6063bbb36a6ccf4e8a4a81c8dd 
>   src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
>   src/tests/module_tests.cpp b5cfbcd4885a4f2c20b19e00958fedda81afa6e0 
>   src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 
>   src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
>   src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
>   src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 
> 
> Diff: https://reviews.apache.org/r/40553/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> James Peach
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message