> On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 710 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line710> > > > > I know you just followed the example of other tests in the file, but > > this test (and other tests in this file) should really ensure that any temp > > files are properly cleaned up in even if the test fails. Current if the > > test fails for whatever reason, the cleanup on #756 won't be executed > > leaking this file. > > > > You could use 'environment->mkdtemp()' instead of 'os::mktemp()' to get > > the cleanup for free in the short term. > > > > s/os::mktemp()/environment->mkdtemp()/ > > > > In the long term, TemporaryDirectoryTest fixture should really have a > > mkdtemp() method on it that deletes temporary directories on TearDown. If > > you want to do this and fix the tests in this file, please do that in > > another review.
I've fixed this for the two new tests using environment->mkdtemp(). Should I submit a new JIRA for the TemporaryDirectoryTest fix? > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 708 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line708> > > > > s/archive/archived/ ? fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 709 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line709> > > > > kill extraneous space. > > > > s/tar gzip/tar gzip/ fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 728 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line728> > > > > unused? removed > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 751 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line751> > > > > s/chowntestuser/'chowntestuser'/ > > s/returned/returns/ fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 755 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line755> > > > > s/cleanup/Cleanup/ > > s/file/file./ fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 762 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line762> > > > > s/archive/archived/ ? fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 782 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line782> > > > > unused? removed > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 805 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line805> > > > > s/despite/Despite/ > > > > s/chowntestuser/'chowntestuser'/ > > > > s/process/fetching/ fixed as suggested > On April 13, 2015, 5:20 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, line 809 > > <https://reviews.apache.org/r/32975/diff/3/?file=925449#file925449line809> > > > > s/cleanup/Cleanup/ > > s/file/file./ fixed as suggested - Jim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32975/#review79898 ----------------------------------------------------------- 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 > >
