----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25569/#review53246 -----------------------------------------------------------
Thanks for following up! Not your fault, but the current design of enable() seems a bit unfortunate, because we will print things excessively unless we use static variables as you've done here. What about the following instead? (1) Add a method called 'disabled()' that returns a list of disabled test names. (2) Inside 'disabled()', we can print the disablement messages once at the beginning, then loop over the tests to construct the list of disabled test names to return. (3) Inside 'Environment::Environment()', we use disabled() to construct the full 'disabled' string. Does that seem cleaner? The static variables seem good for now, modulo my comment below. Consider adding a TODO to clean this up if you go this route! src/tests/environment.cpp <https://reviews.apache.org/r/25569/#comment92803> You've introduced a `return true` case here when docker is "verified", that looks like a regression. How about encoding this tri-state situation (not initialized, not enabled, enabled) through a Result? ``` static Result<Docker> docker = None(); if (docker.isNone()) { Try<Docker> docker_ = Docker::create(flags.docker); if (docker_.isError()) { std::cerr << ... << std::endl; } // Unfortunately there is no Result<T> constructor yet that takes a Try<T>. docker = docker_.isSome() ? docker_.get() : Try<Docker>::error(docker_.error()); } if (docker.isError()) { return false; } ``` - Ben Mahler On Sept. 12, 2014, 4:31 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25569/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2014, 4:31 a.m.) > > > Review request for mesos and Ben Mahler. > > > Repository: mesos-git > > > Description > ------- > > Only perform docker validation once for tests > > > Diffs > ----- > > src/tests/environment.cpp 2274251aaf653d83c2d03ef2186763978067a747 > > Diff: https://reviews.apache.org/r/25569/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
