You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2016/02/25 20:40:47 UTC
Review Request 43908: Stout:[2/2] Added significant test coverage of
`os::rmdir`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/
-----------------------------------------------------------
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Repository: mesos
Description
-------
Stout:[2/2] Added significant test coverage of `os::rmdir`.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
Diff: https://reviews.apache.org/r/43908/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Alex Naparu <al...@outlook.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review120966
-----------------------------------------------------------
Ship it!
Ship It!
- Alex Naparu
On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review120825
-----------------------------------------------------------
Bad patch!
Reviews applied: [43908, 43907, 43906]
Failed command: ./support/apply-review.sh -n -r 43906
Error:
2016-02-26 05:06:40 URL:https://reviews.apache.org/r/43906/diff/raw/ [3197/3197] -> "43906.patch" [1]
error: patch failed: src/CMakeLists.txt:223
error: src/CMakeLists.txt: patch does not apply
Full log: https://builds.apache.org/job/mesos-reviewbot/11655/console
- Mesos ReviewBot
On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review121604
-----------------------------------------------------------
Ship it!
Ship It!
- Joris Van Remoortere
On March 2, 2016, 4 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated March 2, 2016, 4 a.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/
-----------------------------------------------------------
(Updated March 2, 2016, 4 a.m.)
Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
Repository: mesos
Description
-------
Stout:[2/2] Added significant test coverage of `os::rmdir`.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
Diff: https://reviews.apache.org/r/43908/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
> On March 1, 2016, 11:21 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/Makefile.am, line 48
> > <https://reviews.apache.org/r/43908/diff/1/?file=1271187#file1271187line48>
> >
> > style
I'd just like to clarify that I made this line up in `git diff` because it was off... I guess git is subtly different in how it presents diffs.
> On March 1, 2016, 11:21 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp, lines 214-297
> > <https://reviews.apache.org/r/43908/diff/1/?file=1271191#file1271191line214>
> >
> > I don't understand why these got removed.
> > They are not covered by the tests your introduced.
These tests are completely redundant. They exist in both `filesystem_tests.cpp` and `os_tests.cpp`. So I've removed them from `os.hpp`.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review121532
-----------------------------------------------------------
On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Alex Clemmer <cl...@gmail.com>.
> On March 1, 2016, 11:21 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp, line 172
> > <https://reviews.apache.org/r/43908/diff/1/?file=1271190#file1271190line172>
> >
> > What is this?
THe remnants of a merge of two commits, unfortunately.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review121532
-----------------------------------------------------------
On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 43908: Stout:[2/2] Added significant test coverage
of `os::rmdir`.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/43908/#review121532
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/Makefile.am (line 48)
<https://reviews.apache.org/r/43908/#comment183251>
style
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp (line 32)
<https://reviews.apache.org/r/43908/#comment183253>
We have `hashset<string>::EMPTY`.
`const hashset<string>& EMPTY = hashset<string>::EMPTY;`
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp (lines 51 - 91)
<https://reviews.apache.org/r/43908/#comment183256>
Can you add a TODO to parameterize this test?
It's the same code copy pasted with 1 difference: relative vs. absolute path.
3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp (line 172)
<https://reviews.apache.org/r/43908/#comment183257>
What is this?
3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp (lines 163 - 194)
<https://reviews.apache.org/r/43908/#comment183259>
I don't understand why these got removed.
They are not covered by the tests your introduced.
- Joris Van Remoortere
On Feb. 25, 2016, 7:40 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43908/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout:[2/2] Added significant test coverage of `os::rmdir`.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/Makefile.am 756aa29ed134bf10a645af7f6562f86dc8e488f5
> 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 3c65d0422dc6e198180d53d1c9e6cb2839137434
> 3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp d0592ef8a774d380e9df66b7e623eb72b29a28b3
> 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp PRE-CREATION
> 3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp a2bc5c40167896a3df2cfb5b1f3cf58c20ea1422
>
> Diff: https://reviews.apache.org/r/43908/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>