mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Mathiske <be...@mesosphere.io>
Subject Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.
Date Wed, 02 Mar 2016 14:02:51 GMT

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




src/tests/cluster.cpp (line 134)
<https://reviews.apache.org/r/43613/#comment183384>

    Since flags are a parameter here, I'd prefer an Error instead of an ASSERT in this place
(and others). Yes, the ASSERT would work, too, in a way, but here is the difference in semantics
compared to the ASSERT in line 116 above. There an internal operation of the factory function
fails without any interdependence with the outside. Here, in line 134, an external input has
been erroneous. We should catch this outside the factory function or report it back to there.
I find this stylistically more clean.



src/tests/cluster.cpp (line 312)
<https://reviews.apache.org/r/43613/#comment183389>

    Note that an ASSERT is only the correct measure to use here if we return an Error then
and guard the factory function with an ASSERT in every test's top scope. Otherwise, CHECK
was correct, preventing potential access to a NULL pointer.


- Bernd Mathiske


On Feb. 26, 2016, 11:50 a.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43613/
> -----------------------------------------------------------
> 
> (Updated Feb. 26, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, and Michael
Park.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
>     https://issues.apache.org/jira/browse/MESOS-4633
>     https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of test objects
to the test body.
> 
> Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The `Slave`
object performs cleanup originally found in `cluster::Slave::stop`.  `cluster::Master::start`
and `cluster::Slave::start` were changed to factory methods.
> 
> 
> Diffs
> -----
> 
>   src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
>   src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 
> 
> Diff: https://reviews.apache.org/r/43613/diff/
> 
> 
> Testing
> -------
> 
> Tests are run at the end of this review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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