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 Wed, 25 Nov 2015 14:36:50 GMT

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



configure.ac (line 245)
<https://reviews.apache.org/r/40553/#comment167331>

    Please use spaces for indention.



configure.ac (line 247)
<https://reviews.apache.org/r/40553/#comment167332>

    Please set a default here. With that it e.g., becomes clearer what setting is actually
used, e.g., from `./configure --help`.



src/Makefile.am (line 1744)
<https://reviews.apache.org/r/40553/#comment167333>

    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.



src/tests/containerizer/memory_test_helper.cpp (line 197)
<https://reviews.apache.org/r/40553/#comment167345>

    Should probably be passed as an arg (`bool` or even better an `enum` value). You can always
pick a default.



src/tests/utils.hpp (line 37)
<https://reviews.apache.org/r/40553/#comment167335>

    I think `getX` would be more in line with other places in the code base.



src/tests/utils.hpp (line 38)
<https://reviews.apache.org/r/40553/#comment167350>

    These functions cannot promise to return *valid* paths; they should probably all return
`Try<string>` instead. Please also check their impls.



src/tests/utils.cpp (line 56)
<https://reviews.apache.org/r/40553/#comment167347>

    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.



src/tests/utils.cpp (line 64)
<https://reviews.apache.org/r/40553/#comment167339>

    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.



src/tests/utils.cpp (line 71)
<https://reviews.apache.org/r/40553/#comment167336>

    Should this be a JIRA issue?



src/tests/utils.cpp (line 91)
<https://reviews.apache.org/r/40553/#comment167340>

    dito.



src/tests/utils.cpp (line 108)
<https://reviews.apache.org/r/40553/#comment167341>

    dito.



src/tests/utils.cpp (line 125)
<https://reviews.apache.org/r/40553/#comment167342>

    dito.



src/tests/utils.cpp (line 141)
<https://reviews.apache.org/r/40553/#comment167343>

    dito.


- Benjamin Bannier


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


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