You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2018/08/06 19:01:13 UTC
Review Request 68242: Added tests for `Resources` move semantics.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9110
https://issues.apache.org/jira/browse/MESOS-9110
Repository: mesos
Description
-------
See summary
Diffs
-----
src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
Diff: https://reviews.apache.org/r/68242/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 68242: Added tests for `Resources` move semantics.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/#review206926
-----------------------------------------------------------
PASS: Mesos patch 68242 was successfully built and tested.
Reviews applied: `['68197', '68242']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2105/mesos-review-68242
- Mesos Reviewbot Windows
On Aug. 6, 2018, 11:30 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 11:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
>
>
> Diff: https://reviews.apache.org/r/68242/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68242: Added tests for `Resources` move semantics.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/#review206923
-----------------------------------------------------------
Ship it!
src/tests/resources_tests.cpp
Lines 1081-1083 (original), 1122-1124 (patched)
<https://reviews.apache.org/r/68242/#comment290069>
A comment here to clarify why we're wrapping in a Resources()?
This should probably be Resource() non-plural to cover that case?
src/tests/resources_tests.cpp
Line 1251 (original), 1310 (patched)
<https://reviews.apache.org/r/68242/#comment290074>
Ditto above, comments on these would be helpful to the reader:
```
r += Resources(ports2); // Test +=(Resources&&).
```
src/tests/resources_tests.cpp
Lines 2114 (patched)
<https://reviews.apache.org/r/68242/#comment290073>
Newline above the comment here and below
src/tests/resources_tests.cpp
Line 3538 (original), 3655 (patched)
<https://reviews.apache.org/r/68242/#comment290070>
comment here about why the temporary is used?
- Benjamin Mahler
On Aug. 6, 2018, 11:30 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 11:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
>
>
> Diff: https://reviews.apache.org/r/68242/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68242: Added tests for `Resources` move semantics.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/
-----------------------------------------------------------
(Updated Aug. 6, 2018, 4:30 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9110
https://issues.apache.org/jira/browse/MESOS-9110
Repository: mesos
Description
-------
See summary
Diffs (updated)
-----
src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
Diff: https://reviews.apache.org/r/68242/diff/2/
Changes: https://reviews.apache.org/r/68242/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 68242: Added tests for `Resources` move semantics.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Aug. 6, 2018, 2:27 p.m., Benjamin Mahler wrote:
> > src/tests/resources_tests.cpp
> > Line 1069 (original), 1136 (patched)
> > <https://reviews.apache.org/r/68242/diff/1/?file=2069150#file2069150line1136>
> >
> > Is it possible to instead augment any existing tests of addition (e.g. this one) to also do rvalue addition? It should be pretty easy?
> >
> > E.g. here we could have r2 use += of rvalues, and have different sums as well:
> >
> > ```
> > Resources sum1 = r1 + r2;
> > Resources sum2 = Resources(r1) + r2;
> > Resources sum3 = r1 + Resources(r2);
> > Resources sum4 = Resources(r1) + Resources(r2);
> > ```
Done.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/#review206912
-----------------------------------------------------------
On Aug. 6, 2018, 4:30 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 4:30 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
>
>
> Diff: https://reviews.apache.org/r/68242/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 68242: Added tests for `Resources` move semantics.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68242/#review206912
-----------------------------------------------------------
src/tests/resources_tests.cpp
Lines 800 (patched)
<https://reviews.apache.org/r/68242/#comment290044>
I think `MoveConstruction` could the name for this and we only test all the move flavors (Resource, vector, repeated ptr field, Resources), so a bit of a merging of the two tests you have here.
It seems we should test addition separately per my other comment.
src/tests/resources_tests.cpp
Line 1069 (original), 1136 (patched)
<https://reviews.apache.org/r/68242/#comment290043>
Is it possible to instead augment any existing tests of addition (e.g. this one) to also do rvalue addition? It should be pretty easy?
E.g. here we could have r2 use += of rvalues, and have different sums as well:
```
Resources sum1 = r1 + r2;
Resources sum2 = Resources(r1) + r2;
Resources sum3 = r1 + Resources(r2);
Resources sum4 = Resources(r1) + Resources(r2);
```
- Benjamin Mahler
On Aug. 6, 2018, 7:01 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> -----------------------------------------------------------
>
> (Updated Aug. 6, 2018, 7:01 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary
>
>
> Diffs
> -----
>
> src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b
>
>
> Diff: https://reviews.apache.org/r/68242/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>