----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21734/#review46313 -----------------------------------------------------------
Thanks for simplifying the tests, much better! Almost there, just some clean ups to do. src/launcher/fetcher.cpp <https://reviews.apache.org/r/21734/#comment81618> Shouldn't this be just "file://localhost" src/launcher/fetcher.cpp <https://reviews.apache.org/r/21734/#comment81619> If FILE_URI_LOCALHOST is changed as above then no need for the -1. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81636> Remove, not needed anymore. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81637> Remove, not needed anymore. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81638> Remove, not needed anymore. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81621> I don't think you neet tests/mesos.hpp now, just tests/utils.hpp for TemporaryDirectoryTest. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81620> Drop this method and use the sandbox directory as created by TemporaryDirectoryTest. You'll need to repeat a few lines of code for each test but it'll be cleaner and easier to follow. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81626> Much, much simpler! src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81622> use sandbox directory. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81635> Add some newlines, please :-) src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81631> path::join("file://", testFile) + "+0N"? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81624> sandbox directory src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81634> aren't you using mesos::internal::tests? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81629> This should be ASSERT_SOME(fetcherProcess) otherwise you'll .get() on the next line even if isError()? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81627> Double chevron >>? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81640> You should check the Option<int> is some first: ASSERT_SOME(status.get()). src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81623> ditto src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81632> ditto path::join()? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81625> ditto src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81630> ditto ASSERT_ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81628> Double chevron >> ? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/21734/#comment81641> ditto ASSERT_SOME() first. - Ian Downes On June 16, 2014, 9:47 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21734/ > ----------------------------------------------------------- > > (Updated June 16, 2014, 9:47 p.m.) > > > Review request for mesos and Ian Downes. > > > Bugs: MESOS-390 > https://issues.apache.org/jira/browse/MESOS-390 > > > Repository: mesos-git > > > Description > ------- > > Handling file:// in the fetcher, and also handle case when localhost is used. > > > Diffs > ----- > > src/Makefile.am 3e623cc > src/launcher/fetcher.cpp c4425eb > src/tests/fetcher_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/21734/diff/ > > > Testing > ------- > > Added unit tests to test file:// and file://localhost. > make check. > > > Thanks, > > Timothy Chen > >
