Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Till Toenshoff]

> On June 14, 2016, 12:26 a.m., Till Toenshoff wrote:
> > 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.

I would suggest we create a JIRA for a refactoring of the headers for the 
test-helpers which would make sure that we have a minimal, stable and clean cut 
of what we need vs. what we want to keep internal to Mesos. Sounds good?


- Till


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


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

Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Till Toenshoff]

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


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)


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



src/Makefile.am (line 1963)


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)


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

Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Till Toenshoff]

> On June 12, 2016, 9:52 a.m., Kapil Arya wrote:
> > src/Makefile.am, lines 1950-1951
> > 
> >
> > I don't think we should be embedding absolute paths to source/build 
> > directories in a library that we are planning to install on the system. 
> > What is the rationale behind it?

My guess, getting `flags.cpp` as added to this library to compile. However, I 
believe installing this library is not something we need or want to do. And the 
point you are raising Kapil adds to that believe :).


- Till


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


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

Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Kapil Arya]

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




src/Makefile.am (lines 1950 - 1951)


I don't think we should be embedding absolute paths to source/build 
directories in a library that we are planning to install on the system. What is 
the rationale behind it?



src/Makefile.am (lines 1954 - 1955)


ditto.



src/Makefile.am (lines 1971 - 1980)


At some point we should create a few common `_CPPFLAGS`, `_LDFLAGS` and 
`_DEPENDENCIES` variables instead of repeating it for every library/binary.



src/Makefile.am (lines 1982 - 1985)


Why this split?


- Kapil Arya


On June 10, 2016, 8:03 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47374/
> ---
> 
> (Updated June 10, 2016, 8:03 p.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
> 
>

Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Adam B]

> On May 23, 2016, 5:09 p.m., Adam B wrote:
> > src/Makefile.am, lines 1937-1938
> > 
> >
> > Why does libmesos_tests_la_SOURCES need to include qos_controllers code?
> 
> Joseph Wu wrote:
> I don't remember the exact reason, but one of my previous iterations (not 
> published) broke on this module.  It's unnecessary for the current 
> implementation.

I looked it up. LoadQoSController isn't used by Mesos (by default), since only 
the NoopQoSController is referenced in non-test code (as a default). Both are 
included in oversubscription_tests.cpp


- Adam


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


On June 10, 2016, 5:03 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47374/
> ---
> 
> (Updated June 10, 2016, 5:03 p.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
> 
>

Re: Review Request 47374: Separated mesos test helpers into a separate library.

[Joseph Wu]

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

(Updated June 10, 2016, 5:03 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, Kapil Arya, Jan 
Schlicht, and Till Toenshoff.


Changes
---

Removed "experiment" label after some builds + writing module tests in a 
separate repository.
Updated description with some insights gained from the above.


Summary (updated)
-

Separated mesos test helpers into a separate library.


Repository: mesos


Description (updated)
---

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 (updated)
---

make check on OSX, CentOS 7, Ubuntu 14


Thanks,

Joseph Wu