> On 三月 20, 2016, 6:36 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 645-647
> > <https://reviews.apache.org/r/45046/diff/3/?file=1307412#file1307412line645>
> >
> >     I know you followed the existing pattern in this file but we shouldn't 
> > do mktemp() like this in the tests because if the test fails for whatever 
> > reason the temp file and the directory are leaked. 
> >     
> >     The FetcherTest fixture inherits from TemporaryDirectoryTest so that 
> > the test runs in a temporary sandbox.
> >     
> >     So you should do:
> >     
> >     ```
> >     Try<string> dir = os::mkdtemp("./XXXX"));
> >     ASSERT_SOME(dir);
> >     
> >     Try<string> path = os::mktemp(path::join(dir.get(), "XXXX"));
> >     ASSERT_SOME(path);
> >     ```
> >     
> >     Feel free to fix the other tests in this file in a subsequent review or 
> > just add a TODO for now.
> 
> Michael Browning wrote:
>     Happy to fix the other tests in this file. I think it'd be simpler to do 
> it in a separate review -- should I also create an issue for that?
> 
> Vinod Kone wrote:
>     no need to create an issue.

What about using `path::join(os::getcwd(), "XXXX");` to simplify the test? I 
saw that the `ROOT_LocalPullerSimpleCommand` is using such format. 
https://github.com/apache/mesos/blob/master/src/tests/containerizer/provisioner_docker_tests.cpp#L340


- Guangya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45046/#review124456
-----------------------------------------------------------


On 三月 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> -----------------------------------------------------------
> 
> (Updated 三月 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
>     https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -----
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> -------
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Reply via email to