You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2018/03/01 19:19:57 UTC
Re: Review Request 65679: Removed direct unmock calls and added
missing mock call expectations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review198462
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 4870-4878 (patched)
<https://reviews.apache.org/r/65679/#comment278598>
To avoid flakiness, should we register these expectations before the call to `unmocked__run` above?
src/tests/slave_tests.cpp
Lines 5455 (patched)
<https://reviews.apache.org/r/65679/#comment278599>
Is this necessary? Perhaps we could eliminate the `Future<Nothing> failure;`?
- Greg Mann
On Feb. 16, 2018, 1:22 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2018, 1:22 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8611
> https://issues.apache.org/jira/browse/MESOS-8611
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Directly invoking unmock calls in the test process can potentially
> cause races with the real mock slave process. It is more robust to
> dispatch the unmock calls to the real mock slave process.
>
> Also added several mock expectations to avoid "uninteresting mock
> call" test warnings.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e253317a67019302f18afe11e2a314e716cec226
>
>
> Diff: https://reviews.apache.org/r/65679/diff/2/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 --gtest_break_on_failure` runs forever :)
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 65679: Removed direct unmock calls and added
missing mock call expectations.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 1, 2018, 7:19 p.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 5455 (patched)
> > <https://reviews.apache.org/r/65679/diff/2/?file=1968520#file1968520line5456>
> >
> > Is this necessary? Perhaps we could eliminate the `Future<Nothing> failure;`?
>
> Meng Zhu wrote:
> We either need to wait explicitly or set up the expectation as `atMost(1)`. I would prefer the explicit wait as it is unambiguous.
I think I prefer `AtMost(1)`, for two reasons:
1) It makes it clear to the reader that satisfaction of that expectation is not essential to the test.
2) It's a pattern that we use elsewhere in similar situations.
I don't think I've seen other situations where we AWAIT on a future just to make sure the expectation is satisfied, rather than using `AtMost(1)`, but perhaps I'm wrong?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review198462
-----------------------------------------------------------
On Feb. 16, 2018, 1:22 a.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated Feb. 16, 2018, 1:22 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8611
> https://issues.apache.org/jira/browse/MESOS-8611
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Directly invoking unmock calls in the test process can potentially
> cause races with the real mock slave process. It is more robust to
> dispatch the unmock calls to the real mock slave process.
>
> Also added several mock expectations to avoid "uninteresting mock
> call" test warnings.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e253317a67019302f18afe11e2a314e716cec226
>
>
> Diff: https://reviews.apache.org/r/65679/diff/3/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 --gtest_break_on_failure` runs forever :)
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 65679: Removed direct unmock calls and added
missing mock call expectations.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 1, 2018, 11:19 a.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 4870-4878 (patched)
> > <https://reviews.apache.org/r/65679/diff/2/?file=1968520#file1968520line4871>
> >
> > To avoid flakiness, should we register these expectations before the call to `unmocked__run` above?
Sounds good. Updated.
> On March 1, 2018, 11:19 a.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 5455 (patched)
> > <https://reviews.apache.org/r/65679/diff/2/?file=1968520#file1968520line5456>
> >
> > Is this necessary? Perhaps we could eliminate the `Future<Nothing> failure;`?
We either need to wait explicitly or set up the expectation as `atMost(1)`. I would prefer the explicit wait as it is unambiguous.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review198462
-----------------------------------------------------------
On Feb. 15, 2018, 5:22 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 5:22 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8611
> https://issues.apache.org/jira/browse/MESOS-8611
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Directly invoking unmock calls in the test process can potentially
> cause races with the real mock slave process. It is more robust to
> dispatch the unmock calls to the real mock slave process.
>
> Also added several mock expectations to avoid "uninteresting mock
> call" test warnings.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e253317a67019302f18afe11e2a314e716cec226
>
>
> Diff: https://reviews.apache.org/r/65679/diff/3/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 --gtest_break_on_failure` runs forever :)
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 65679: Removed direct unmock calls and added
missing mock call expectations.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On March 1, 2018, 11:19 a.m., Greg Mann wrote:
> > src/tests/slave_tests.cpp
> > Lines 5455 (patched)
> > <https://reviews.apache.org/r/65679/diff/2/?file=1968520#file1968520line5456>
> >
> > Is this necessary? Perhaps we could eliminate the `Future<Nothing> failure;`?
>
> Meng Zhu wrote:
> We either need to wait explicitly or set up the expectation as `atMost(1)`. I would prefer the explicit wait as it is unambiguous.
>
> Greg Mann wrote:
> I think I prefer `AtMost(1)`, for two reasons:
> 1) It makes it clear to the reader that satisfaction of that expectation is not essential to the test.
> 2) It's a pattern that we use elsewhere in similar situations.
>
> I don't think I've seen other situations where we AWAIT on a future just to make sure the expectation is satisfied, rather than using `AtMost(1)`, but perhaps I'm wrong?
OK, sounds good.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review198462
-----------------------------------------------------------
On Feb. 15, 2018, 5:22 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated Feb. 15, 2018, 5:22 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8611
> https://issues.apache.org/jira/browse/MESOS-8611
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Directly invoking unmock calls in the test process can potentially
> cause races with the real mock slave process. It is more robust to
> dispatch the unmock calls to the real mock slave process.
>
> Also added several mock expectations to avoid "uninteresting mock
> call" test warnings.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp e253317a67019302f18afe11e2a314e716cec226
>
>
> Diff: https://reviews.apache.org/r/65679/diff/4/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 --gtest_break_on_failure` runs forever :)
>
>
> Thanks,
>
> Meng Zhu
>
>