Re: Review Request 43969: Added test for Appc image fetcher.

2016-03-02 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On March 2, 2016, 9:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> ---
> 
> (Updated March 2, 2016, 9:47 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
> https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43969: Added test for Appc image fetcher.

2016-03-02 Thread Jojy Varghese 

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

(Updated March 2, 2016, 9:47 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-4748
https://issues.apache.org/jira/browse/MESOS-4748


Repository: mesos


Description
---

Added simple appc Fetcher test with mock HTTP image server.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 

Diff: https://reviews.apache.org/r/43969/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43969: Added test for Appc image fetcher.

2016-02-26 Thread Jojy Varghese 

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

(Updated Feb. 27, 2016, 4:05 a.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


Bugs: MESOS-4748
https://issues.apache.org/jira/browse/MESOS-4748


Repository: mesos


Description
---

Added simple appc Fetcher test with mock HTTP image server.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 

Diff: https://reviews.apache.org/r/43969/diff/


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43969: Added test for Appc image fetcher.

2016-02-26 Thread Jojy Varghese 


> On Feb. 27, 2016, 12:21 a.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, line 539
> > 
> >
> > We might want to serve multiple images. Can you make it more general so 
> > that we can serve any files under 'server' directory?

I thought this is a good start and we can expand as we go. We could add a map 
of `name->path` in the test fixture. I can add a TODO.


- Jojy


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


On Feb. 24, 2016, 11:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> ---
> 
> (Updated Feb. 24, 2016, 11:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
> https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43969: Added test for Appc image fetcher.

2016-02-26 Thread Jie Yu 

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




src/tests/containerizer/provisioner_appc_tests.cpp (line 240)


you can do return JSON::parse(...) here, right?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 241 - 265)


Can you use c++11 string literal here (to avoid backslashes and quotes).

http://en.cppreference.com/w/cpp/language/string_literal

See example here:

https://github.com/apache/mesos/blob/master/src/tests/containerizer/docker_spec_tests.cpp#L104



src/tests/containerizer/provisioner_appc_tests.cpp (line 455)


Can you call it TestAppcImageServer?



src/tests/containerizer/provisioner_appc_tests.cpp (line 479)


Ditto on using c++ string literals. You can also use strings::format to 
avoid inlining varaibles.

```
%s/TestAppcImageServer/image
```



src/tests/containerizer/provisioner_appc_tests.cpp (line 510)


We might want to serve multiple images. Can you make it more general so 
that we can serve any files under 'server' directory?



src/tests/containerizer/provisioner_appc_tests.cpp (line 528)


We avoid static non-pod. Can you just use:
```
const string serverDir = path::join(os::getcwd(), "server");
```



src/tests/containerizer/provisioner_appc_tests.cpp (line 550)


We don't use 'override' in our code base. Can you remove them for now for 
consistency?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 555 - 558)


I'd like this to be called by the test. We might want to prepare multiple 
images to test the code path of fetching dependencies.


- Jie Yu


On Feb. 24, 2016, 11:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43969/
> ---
> 
> (Updated Feb. 24, 2016, 11:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4748
> https://issues.apache.org/jira/browse/MESOS-4748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added simple appc Fetcher test with mock HTTP image server.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 
> 
> Diff: https://reviews.apache.org/r/43969/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 43969: Added test for Appc image fetcher.

2016-02-24 Thread Jojy Varghese 

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

Review request for mesos and Jie Yu.


Bugs: MESOS-4748
https://issues.apache.org/jira/browse/MESOS-4748


Repository: mesos


Description
---

Added simple appc Fetcher test with mock HTTP image server.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
9d9779ac04e32348ce0e28da54fd7aa039a7fbcd 

Diff: https://reviews.apache.org/r/43969/diff/


Testing
---

make check.


Thanks,

Jojy Varghese