> 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 > >
