> On April 8, 2015, 9:57 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, lines 112-116 > > <https://reviews.apache.org/r/32975/diff/1/?file=920880#file920880line112> > > > > Add a note/todo here mentioning why you are setting these fields but > > not doing any asserts/expects on them. > > Jim Klucar wrote: > With the type_utils.cpp change, the EXPECT_EQ on lines 140/141 should be > checking the chown field. Should I still add text regarding how we can't > check this capability without assuming the user running the test has > permission to chown to some other user? > > Vinod Kone wrote: > Yes. Because the EXPECT on #140 just checks that the chown field is > properly propagated to fetcher environment but not that fetcher actually > skips chown if chown is false. > > If you write a FetcherTest (see examples later in the file) that actually > runs a fetcher process, would you able to test the semantics? I'm thinking > that you can create a non-existent user (random string) and set chown to > false. Once fetcher process finishes, you can check that the owner of the > fetched file hasn't changed to the user. Does that make sense? > > Jim Klucar wrote: > This makes sense. I'll create the test, back out the change, see the test > fail, then add the change back in.
I ended up creating two new tests. Both extract a framework, one sets chown true and expects a failure due to the unknown user name. The other sets chown to false and expects the fetcher to finish without error. - Jim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32975/#review79433 ----------------------------------------------------------- On April 13, 2015, 4:55 p.m., Jim Klucar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32975/ > ----------------------------------------------------------- > > (Updated April 13, 2015, 4:55 p.m.) > > > Review request for mesos. > > > Bugs: MESOS-1790 > https://issues.apache.org/jira/browse/MESOS-1790 > > > Repository: mesos > > > Description > ------- > > Added chown to CommandInfo.URI protocol buffer as an optional > boolean that defaults to true, the current chown behavior. > > The fetcher was updated to skip the os::chown operation if the chown > boolean is set to false. > > No documentation was updated. > > > Diffs > ----- > > include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 > src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b > src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 > src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e > > Diff: https://reviews.apache.org/r/32975/diff/ > > > Testing > ------- > > Unit testing this functionality is difficult because it would require that > the user running the test to have permission to chown a file to someone other > than themselves. I didn't want to add that as a requirement to build. I added > the new field to the existing test cases just to see that they populate. > > > Thanks, > > Jim Klucar > >
