You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/07/02 00:44:54 UTC
Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/
-----------------------------------------------------------
Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
Bugs: mesos-2834
https://issues.apache.org/jira/browse/mesos-2834
Repository: mesos
Description
-------
perf: changed 'parse' interface to allow testing and added tests.
Diffs
-----
src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
Diff: https://reviews.apache.org/r/36115/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/
-----------------------------------------------------------
(Updated July 2, 2015, 11:42 p.m.)
Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
Bugs: mesos-2834
https://issues.apache.org/jira/browse/mesos-2834
Repository: mesos
Description
-------
perf: changed 'parse' interface to allow testing and added tests.
Diffs (updated)
-----
src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
Diff: https://reviews.apache.org/r/36115/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Chi Zhang <ch...@gmail.com>.
> On July 2, 2015, 12:29 a.m., Ian Downes wrote:
> > src/tests/perf_tests.cpp, line 87
> > <https://reviews.apache.org/r/36115/diff/1/?file=997707#file997707line87>
> >
> > ```cpp
> > foreach (const tuple<Version, string>& input, input1) {}
> > ```?
foreach doesn't work with tuple for me. I used the for (auto it : input) suggested by pbrett.
> On July 2, 2015, 12:29 a.m., Ian Downes wrote:
> > src/tests/perf_tests.cpp, lines 76-84
> > <https://reviews.apache.org/r/36115/diff/1/?file=997707#file997707line76>
> >
> > You're testing each version of parse here explicitly rather than testing that parse() does the right thing for different versions of input. I would construct different inputs corresponding to different perf versions and then verify that the *output* had the correct fields and values.
I am not completely clear about your thoughts. could you give an example maybe?
Also if I give v2 exact a v1 input "1,cycle,cgroup", it'd have no problem parsing it as value of 1, unit of cycle and event of cgroup.
parse can't detect the version out of the input, it uses the version info either given or from uname to delegate to the right extract, that's why I used a lot of verions to test the extract choose logic.
If I get your idea correctly, it's just a differernt way to organize the loops?
I am looping over all the versions for input of the same nature; you want to test one version at a time but loop over all the inputs (of different natures)?
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90177
-----------------------------------------------------------
On July 2, 2015, 11:42 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> -----------------------------------------------------------
>
> (Updated July 2, 2015, 11:42 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90177
-----------------------------------------------------------
src/tests/perf_tests.cpp (lines 76 - 84)
<https://reviews.apache.org/r/36115/#comment143173>
You're testing each version of parse here explicitly rather than testing that parse() does the right thing for different versions of input. I would construct different inputs corresponding to different perf versions and then verify that the *output* had the correct fields and values.
src/tests/perf_tests.cpp (line 87)
<https://reviews.apache.org/r/36115/#comment143172>
```cpp
foreach (const tuple<Version, string>& input, input1) {}
```?
- Ian Downes
On July 1, 2015, 3:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 3:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Chi Zhang <ch...@gmail.com>.
> On July 1, 2015, 11:41 p.m., Paul Brett wrote:
> > src/tests/perf_tests.cpp, line 93
> > <https://reviews.apache.org/r/36115/diff/1/?file=997707#file997707line93>
> >
> > ASSERT_SOME_TRUE
would need parse to be Try<bool> for it to compile.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90164
-----------------------------------------------------------
On July 2, 2015, 11:42 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> -----------------------------------------------------------
>
> (Updated July 2, 2015, 11:42 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90164
-----------------------------------------------------------
src/tests/perf_tests.cpp (line 87)
<https://reviews.apache.org/r/36115/#comment143151>
for(auto it : input1)
src/tests/perf_tests.cpp (line 93)
<https://reviews.apache.org/r/36115/#comment143152>
ASSERT_SOME_TRUE
src/tests/perf_tests.cpp (line 131)
<https://reviews.apache.org/r/36115/#comment143153>
Ditto.
src/tests/perf_tests.cpp (line 137)
<https://reviews.apache.org/r/36115/#comment143155>
Ditto.
src/tests/perf_tests.cpp (line 170)
<https://reviews.apache.org/r/36115/#comment143156>
Ditto.
src/tests/perf_tests.cpp (line 240)
<https://reviews.apache.org/r/36115/#comment143157>
Ditto.
- Paul Brett
On July 1, 2015, 10:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 36115: perf: changed 'parse' interface to allow
testing and added tests.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36115/#review90182
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36112, 36113, 36114, 36115]
All tests passed.
- Mesos ReviewBot
On July 1, 2015, 10:44 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36115/
> -----------------------------------------------------------
>
> (Updated July 1, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Ian Downes, Paul Brett, and Cong Wang.
>
>
> Bugs: mesos-2834
> https://issues.apache.org/jira/browse/mesos-2834
>
>
> Repository: mesos
>
>
> Description
> -------
>
> perf: changed 'parse' interface to allow testing and added tests.
>
>
> Diffs
> -----
>
> src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b
> src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02
> src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
>
> Diff: https://reviews.apache.org/r/36115/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>