mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Till Toenshoff <toensh...@me.com>
Subject Re: Review Request 47374: Separated mesos test helpers into a separate library.
Date Tue, 14 Jun 2016 00:26:03 GMT

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



Overall, I am not convinced this is reaching the goals we have just yet...

Let me try to specify our goals;
(a) we want to enable mesos-modules (tests) to reuse our test helpers/utils - things like
`cluster.cpp` etc. 
(b) we want to make sure that everything used within the modules is actually publicly exposed
to prevent creating dependencies on internal stuff that is not going through deprecations
(c) we try to be least disruptive on mesos

a. reached
b. not reached as we do not expose the headers - a module test using that library will still
have to tap into the non public mesos folders to get to the needed headers
c. reached - this patch only changes a makefile

So (b) is the culprit and solving it properly will likely require us to do some serious refactoring
of those headers (e.g. `src/tests/mesos.hpp`) to have a clean cut between stuff we want to
expose and things we can keep internal to mesos.

Having public test helpers available for module developers is great and very much needed -
but I also believe that it might need much more work.


src/Makefile.am (line 1930)
<https://reviews.apache.org/r/47374/#comment202406>

    Why would we want to install this static library together with our tests?
    
    The installation of tests was meant as provisioning tests; to make sure the host system
can successfully run mesos which is validated by running mesos-tests without having to build
them on that host.



src/Makefile.am (line 1933)
<https://reviews.apache.org/r/47374/#comment202407>

    I would propose to move this out of the if-clause and make it a general `noinst`.



src/Makefile.am (line 1963)
<https://reviews.apache.org/r/47374/#comment202411>

    Not yours but our indentation seems to be inconsistent here. We might want to clean that
up in another patch.



src/Makefile.am (lines 1971 - 1980)
<https://reviews.apache.org/r/47374/#comment202408>

    Good point, I very much agree.


- Till Toenshoff


On June 11, 2016, 12:03 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47374/
> -----------------------------------------------------------
> 
> (Updated June 11, 2016, 12:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, Kapil Arya, Jan Schlicht,
and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This gives external projects easier access to the test helpers used in
> mesos tests.  
> 
> For example, a module writer may want to write a test like 
> `src/tests/oversubscription_tests.cpp`.  To build and link against 
> this library, the module writer would mimic the build flags for tests:
> ```
> # Main test file is taken directly from Mesos.
> my_module_tests_SOURCES = \
>   $(MESOS)/src/tests/main.cpp
> 
> my_module_tests_CPPFLAGS = \
>   -I$(GMOCK)/include       \
>   -I$(GTEST)/include       \
>   -I$(MESOS)/include/mesos \
>   -I$(ZOOKEEPER)/include   \
>   -I$(ZOOKEEPER)/generated \
>   $(AM_CPPFLAGS)
>   
> my_module_tests_LDADD = \
>   $(MESOS)/3rdparty/.libs/libgmock.la \
>   $(MESOS)/src/.libs/libmesos.la      \
>   $(MESOS)/src/.libs/libmesos_tests.la
> ```
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
> 
> Diff: https://reviews.apache.org/r/47374/diff/
> 
> 
> Testing
> -------
> 
> make check on OSX, CentOS 7, Ubuntu 14
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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