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/02/16 01:22:09 UTC
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/
-----------------------------------------------------------
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
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 d2c242eae3169bff3b77770197a36f171cd668ba
Diff: https://reviews.apache.org/r/65679/diff/1/
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
>
>
Re: Review Request 65679: Removed direct unmock calls and added
missing mock call expectations.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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 Meng Zhu <mz...@mesosphere.io>.
> On March 14, 2018, 5:08 p.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Line 4982 (original), 5022 (patched)
> > <https://reviews.apache.org/r/65679/diff/4/?file=1968736#file1968736line5023>
> >
> > Is it guaranteed that this lambda will finish before `exitedExecutorMessage` is satisfied, so we can make sure that is function won't be called after the variables are destructed?
> >
> > If not, we probably should do the following instead:
> > ```
> > Future<Nothing> unmocked___run = process::dispatch(slave.get()->pid, [&] {
> > slave.get()->mock()->unmocked___run(
> > ...);
> >
> > return Nothing();
> > });
> > ```
> > And do `AWAIT_READY(unmocked___run)` at the end of the test.
Currently, it is guaranteed, as send `exitedExecutorMessage` is the last statement of the function. But I should add your suggestion to be future proof. But this piece of code is gone after I updated the test to use the mock authorizer. Dropping the issue.
> On March 14, 2018, 5:08 p.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 5179 (patched)
> > <https://reviews.apache.org/r/65679/diff/4/?file=1968736#file1968736line5180>
> >
> > Is it guaranteed that this lambda will finish before `executorShutdown` is satisfied, so we can make sure that is function won't be called after the variables are destructed?
> >
> > If not, we need to do the same thing I suggested above.
Currently, it is guaranteed, but let's add that to be future proof.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199223
-----------------------------------------------------------
On March 16, 2018, 12:07 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated March 16, 2018, 12:07 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 f76500ebdb67f131a57a3b5aaae8c952d019e354
>
>
> Diff: https://reviews.apache.org/r/65679/diff/5/
>
>
> 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 Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199223
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 4158-4160 (patched)
<https://reviews.apache.org/r/65679/#comment279523>
Can we do the following:
```
EXPECT_CALL(sched, executorLost(...))
.Times(AtMost(1));
```
And remove `executorLost`, since it's not in the original test?
src/tests/slave_tests.cpp
Lines 4671 (patched)
<https://reviews.apache.org/r/65679/#comment279526>
Since we dispatch `unmocked__run` into the slave context, we can just leave the `FutureSatisfy` as is, to be consistent with other tests.
src/tests/slave_tests.cpp
Lines 4863-4865 (patched)
<https://reviews.apache.org/r/65679/#comment279524>
Ditto.
src/tests/slave_tests.cpp
Lines 4986-4988 (patched)
<https://reviews.apache.org/r/65679/#comment279525>
Ditto.
src/tests/slave_tests.cpp
Line 4982 (original), 5022 (patched)
<https://reviews.apache.org/r/65679/#comment279527>
Is it guaranteed that this lambda will finish before `exitedExecutorMessage` is satisfied, so we can make sure that is function won't be called after the variables are destructed?
If not, we probably should do the following instead:
```
Future<Nothing> unmocked___run = process::dispatch(slave.get()->pid, [&] {
slave.get()->mock()->unmocked___run(
...);
return Nothing();
});
```
And do `AWAIT_READY(unmocked___run)` at the end of the test.
src/tests/slave_tests.cpp
Lines 5179 (patched)
<https://reviews.apache.org/r/65679/#comment279522>
Is it guaranteed that this lambda will finish before `executorShutdown` is satisfied, so we can make sure that is function won't be called after the variables are destructed?
If not, we need to do the same thing I suggested above.
src/tests/slave_tests.cpp
Lines 5382-5389 (original), 5431-5443 (patched)
<https://reviews.apache.org/r/65679/#comment279533>
The high-level guideline is try to avoid using/exposing `Promise` unless necessary. Let's return a `Nothing` in the lambda instead, as I suggested above.
- Chun-Hung Hsiao
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/4/
>
>
> 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 Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review197656
-----------------------------------------------------------
PASS: Mesos patch 65679 was successfully built and tested.
Reviews applied: `['65679']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65679
- Mesos Reviewbot Windows
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.
>
>
> 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 d2c242eae3169bff3b77770197a36f171cd668ba
>
>
> Diff: https://reviews.apache.org/r/65679/diff/1/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/
-----------------------------------------------------------
(Updated April 5, 2018, 3:35 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Added additional fix for `RemoveExecutorUponFailedLaunch`
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 (updated)
-----
src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
Diff: https://reviews.apache.org/r/65679/diff/10/
Changes: https://reviews.apache.org/r/65679/diff/9-10/
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/
-----------------------------------------------------------
(Updated April 4, 2018, 12:20 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Updated the patch to capture by value and added extra synchronization to avoid potential race conditions.
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 (updated)
-----
src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353
Diff: https://reviews.apache.org/r/65679/diff/9/
Changes: https://reviews.apache.org/r/65679/diff/8-9/
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/
-----------------------------------------------------------
(Updated March 21, 2018, 2:40 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Made format consistent.
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 (updated)
-----
src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354
Diff: https://reviews.apache.org/r/65679/diff/8/
Changes: https://reviews.apache.org/r/65679/diff/7-8/
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 21, 2018, 1:55 p.m., Chun-Hung Hsiao wrote:
> > src/tests/slave_tests.cpp
> > Lines 5137-5138 (patched)
> > <https://reviews.apache.org/r/65679/diff/7/?file=1984625#file1984625line5137>
> >
> > Just curious, why do you break the assigment into two lines here but not below? ;)
All right, let's throw them all into clang-formatter.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199703
-----------------------------------------------------------
On March 21, 2018, 2:40 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 2:40 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 f76500ebdb67f131a57a3b5aaae8c952d019e354
>
>
> Diff: https://reviews.apache.org/r/65679/diff/8/
>
>
> 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 Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199703
-----------------------------------------------------------
Ship it!
src/tests/slave_tests.cpp
Lines 5137-5138 (patched)
<https://reviews.apache.org/r/65679/#comment280112>
Just curious, why do you break the assigment into two lines here but not below? ;)
- Chun-Hung Hsiao
On March 21, 2018, 6:38 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated March 21, 2018, 6:38 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 f76500ebdb67f131a57a3b5aaae8c952d019e354
>
>
> Diff: https://reviews.apache.org/r/65679/diff/7/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/
-----------------------------------------------------------
(Updated March 21, 2018, 11:38 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 (updated)
-----
src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354
Diff: https://reviews.apache.org/r/65679/diff/7/
Changes: https://reviews.apache.org/r/65679/diff/6-7/
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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199606
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/slave_tests.cpp
Lines 5131-5132 (patched)
<https://reviews.apache.org/r/65679/#comment279969>
"We continue dispatching `___run()` to make sure the `CHECK()` in the continuation passes."
Here and elsewhere.
src/tests/slave_tests.cpp
Lines 5407 (patched)
<https://reviews.apache.org/r/65679/#comment279970>
s/slave/the/
s/call finish/call to finish/
src/tests/slave_tests.cpp
Lines 5408-5409 (patched)
<https://reviews.apache.org/r/65679/#comment279971>
"Otherwise, the objects we captured by reference and passed to `unmocked___run` may be destroyed in the middle of the function."
- Greg Mann
On March 16, 2018, 7:07 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65679/
> -----------------------------------------------------------
>
> (Updated March 16, 2018, 7:07 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 f76500ebdb67f131a57a3b5aaae8c952d019e354
>
>
> Diff: https://reviews.apache.org/r/65679/diff/6/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/
-----------------------------------------------------------
(Updated March 16, 2018, 12:07 p.m.)
Review request for mesos, Chun-Hung Hsiao and Greg Mann.
Changes
-------
Patch updated. Thanks for the informative review.
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 (updated)
-----
src/tests/slave_tests.cpp f76500ebdb67f131a57a3b5aaae8c952d019e354
Diff: https://reviews.apache.org/r/65679/diff/5/
Changes: https://reviews.apache.org/r/65679/diff/4-5/
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 Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review199267
-----------------------------------------------------------
src/tests/slave_tests.cpp
Lines 5179 (patched)
<https://reviews.apache.org/r/65679/#comment279562>
Could you add a comment here to explain that we run the continuation here to test if all checks in this continuation passes? Ditto for all other `unmocked____run` calls that doesn't generate any task status update or other side effects we check in tests.
- Chun-Hung Hsiao
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/4/
>
>
> 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 Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65679/#review197658
-----------------------------------------------------------
Patch looks great!
Reviews applied: [65679]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 16, 2018, 9: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, 9:22 a.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> 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 d2c242eae3169bff3b77770197a36f171cd668ba
>
>
> Diff: https://reviews.apache.org/r/65679/diff/1/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter=*SlaveTest* --gtest_repeat=-1 --gtest_break_on_failure` runs forever :)
>
>
> Thanks,
>
> Meng Zhu
>
>