> On March 12, 2014, 12: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.
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. > On March 12, 2014, 12:57 p.m., Ben Mahler wrote: > > src/tests/containerizer_tests.cpp, lines 1-17 > > <https://reviews.apache.org/r/18974/diff/4/?file=517557#file517557line1> > > > > Why did you introduce a new file here? Why have both: > > > > src/tests/containerizer.cpp > > src/tests/containerizer_tests.cpp src/tests/containerizer.cpp is a Containerizer for tests. src/tests/containerizer_tests.cpp are the tests for Containerizer. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18974/#review36964 ----------------------------------------------------------- On March 12, 2014, 11:33 a.m., Dominic Hamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18974/ > ----------------------------------------------------------- > > (Updated March 12, 2014, 11:33 a.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 > >
