> On March 12, 2014, 7:57 p.m., Ben Mahler wrote: > > src/slave/containerizer/mesos_containerizer.cpp, lines 56-61 > > <https://reviews.apache.org/r/18974/diff/4/?file=517556#file517556line56> > > > > We tend to avoid introducing one-off "helper" functions to pull out > > arbitrary pieces of unique logic in the code. > > > > We're not achieving any simplification here. Instead we're strictly > > adding complexity. Now we have: > > > > (1) More code. > > (2) Readers of fetch() have to understand what buildCommand() does, and > > that it doesn't add the fetcher path. > > (3) Readers of mesos_containerizer.cpp need to understand what > > buildCommand() is for, who calls it (only 1 use of this function), and why > > we decided to pull it out (in this case only because we wanted to unit test > > this particular piece of logic). > > > > The unit tests for this are not checking that the command functions > > correctly, merely the format. > > > > So my question here is, rather than pulling out an arbitrary piece of > > fetch(), can fetch() be converted to a static function that we can test > > fully using local URIs? It looks like fetch is only using the 'flags' > > member. This way we can test the fetching in a more complete manner, > > including the output redirection. > > Dominic Hamon wrote: > If all of 'fetch' were replaced by a series of (well-named) helper > functions then fetch would be much easier to understand. The details of what > each helper does are irrelevant to someone reading the code. As such, this > would be simpler. I think the issue here is that the helper function is not > well-named, and it's only one that has been pulled out. > > The unit tests are testing the function which returns a well-formed and > particular string given the inputs. That's the point of a unit test. Testing > all of fetch is possible, but unhelpful given all the parameters and variants > that would need to be covered. > > Instead, I'd argue that fetch should be a series of calls to helper > functions, each of which should be unit tested. Also, that fetcher should > have its own unit test suite. > > Can fetch() be converted to a static function? Yes. But it should not be > tested in that form. > > Should fetch() have better test coverage? Yes.
Chatted with Dominic about this. We can simplify fetch() by having subprocess take an environment map and test in the Subprocess tests that the environment is correctly set in the forked subprocess. This way, fetch() only has to correctly construct the environment map from the commandInfo and other information, no escaping necessary. This conversion from CommandInfo -> environment map is something we could pull out as a clear "pure" transformation function, if we'd like to unit test it and possibly re-use it. In general, I would like us to take a step back and understand why code is complex when we attempt to simplify or when we struggle to test things. Having the right abstraction boundaries are essential to keeping our code base clear and maintainable. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18974/#review36964 ----------------------------------------------------------- On March 12, 2014, 6:33 p.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18974/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 6:33 p.m.) > > > Review request for mesos, Ian Downes and Vinod Kone. > > > Bugs: MESOS-1050 and MESOS-1063 > https://issues.apache.org/jira/browse/MESOS-1050 > https://issues.apache.org/jira/browse/MESOS-1063 > > > Repository: mesos-git > > > Description > ------- > > see summary > > > Diffs > ----- > > src/Makefile.am c18b311bc578e6e56a76be95fbafc8d7d7712787 > src/slave/containerizer/mesos_containerizer.cpp > 9bf9829fb38f17d9a2a0d3ab33d45d34cd5d3ea5 > src/tests/containerizer_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/18974/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Dominic Hamon > >
