You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Bartek Plotka <bw...@gmail.com> on 2015/06/01 07:34:10 UTC
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 1, 2015, 5:34 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issue addressed. Rebased master HEAD.
Repository: mesos
Description
-------
This patch prepares Resource Estimator tests for modularized ResourceEstimator.
It includes:
1. Made oversubscription_test to be typed_test.
2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 1, 2015, 7:34 p.m., Jie Yu wrote:
> > src/tests/oversubscription_tests.cpp, line 96
> > <https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line96>
> >
> > This looks weired to me since we are actually creating a TestResourceEstimator, but TypeParam == NoopResourceEstimator.
> >
> > IMO, all the tests so far in this file does not work for an arbitrary resource estimator (i.e., the expectations are tied with the estimator). As a result, using a typed test doesn't seem to make sense to me.
> >
> > Thoughts?
That is a good point!
I agree that we do not test NoopResourceEstimator but the plumbing right now. However, is there anything bad in testing both module RE and normal RE in such typed test?
In fact these typed tests are going to be necessary when other ResourceEstimators will be implemented.
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86051
-----------------------------------------------------------
On June 1, 2015, 5:37 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:37 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On June 1, 2015, 12:34 p.m., Jie Yu wrote:
> > src/tests/oversubscription_tests.cpp, line 96
> > <https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line96>
> >
> > This looks weired to me since we are actually creating a TestResourceEstimator, but TypeParam == NoopResourceEstimator.
> >
> > IMO, all the tests so far in this file does not work for an arbitrary resource estimator (i.e., the expectations are tied with the estimator). As a result, using a typed test doesn't seem to make sense to me.
> >
> > Thoughts?
>
> Bartek Plotka wrote:
> That is a good point!
>
> I agree that we do not test NoopResourceEstimator but the plumbing right now. However, is there anything bad in testing both module RE and normal RE in such typed test?
> In fact these typed tests are going to be necessary when other ResourceEstimators will be implemented.
One thought; We need two different things:
1.a) Test the default behavior of a non-oversubscribe scenario i.e. NoopEstimator
1.b) Test modules work (same as 1.a) but using Module typed test
2.) Using the mock tests to control the estimates in a fine grained fashion (MockEstimator).
In other words, can we separate the two?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86051
-----------------------------------------------------------
On May 31, 2015, 10:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated May 31, 2015, 10:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86051
-----------------------------------------------------------
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment137916>
This looks weired to me since we are actually creating a TestResourceEstimator, but TypeParam == NoopResourceEstimator.
IMO, all the tests so far in this file does not work for an arbitrary resource estimator (i.e., the expectations are tied with the estimator). As a result, using a typed test doesn't seem to make sense to me.
Thoughts?
- Jie Yu
On June 1, 2015, 5:37 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:37 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On June 1, 2015, 1:46 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 225-226
> > <https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line225>
> >
> > We can merge these two with multiple put() on a queue :)
>
> Bartek Plotka wrote:
> That's true, but in previous issue you asked to delete the queue ;p
> https://reviews.apache.org/r/34816/#comment_container_137589
> IMO in these tests there are two or one promise at one time, so it is more clear - it describes the flow. E.g: We can specify than one is a normal estimation and second is reduced one. If there were more promises (in more complicated test case) then obviously the queue would be preferable. However i am not strongly tied to that approach - we can add queue.
That was not the point - that was about having estimate() on the test class, when the queue could be owned by the test body.
I apologize if that didn't came across.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86066
-----------------------------------------------------------
On May 31, 2015, 10:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated May 31, 2015, 10:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 1, 2015, 8:46 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 225-226
> > <https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line225>
> >
> > We can merge these two with multiple put() on a queue :)
That's true, but in previous issue you asked to delete the queue ;p
https://reviews.apache.org/r/34816/#comment_container_137589
IMO in these tests there are two or one promise at one time, so it is more clear - it describes the flow. E.g: We can specify than one is a normal estimation and second is reduced one. If there were more promises (in more complicated test case) then obviously the queue would be preferable. However i am not strongly tied to that approach - we can add queue.
> On June 1, 2015, 8:46 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 159-160
> > <https://reviews.apache.org/r/34816/diff/4/?file=975492#file975492line159>
> >
> > We wouldn't need this, if 'estimation' was a queue, right?
See below
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86066
-----------------------------------------------------------
On June 1, 2015, 5:37 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:37 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86066
-----------------------------------------------------------
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment137961>
Later on, we need to make this a queue (when giving more than one estimate).
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment137962>
We wouldn't need this, if 'estimation' was a queue, right?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment137963>
We can merge these two with multiple put() on a queue :)
- Niklas Nielsen
On May 31, 2015, 10:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated May 31, 2015, 10:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch prepares Resource Estimator tests for modularized ResourceEstimator.
>
> It includes:
> 1. Made oversubscription_test to be typed_test.
> 2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 2, 2015, 5:05 p.m., Jie Yu wrote:
> > src/tests/oversubscription_tests.cpp, lines 77-80
> > <https://reviews.apache.org/r/34816/diff/7/?file=976044#file976044line77>
> >
> > Any reason change the comments and the test name here?
Sorry - mistake during fixing git merge conflicts
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86246
-----------------------------------------------------------
On June 2, 2015, 7:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86246
-----------------------------------------------------------
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138169>
Any reason change the comments and the test name here?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138170>
No snake_case please
- Jie Yu
On June 2, 2015, 12:38 a.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 12:38 a.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 2, 2015, 4:24 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, line 797
> > <https://reviews.apache.org/r/34816/diff/7/?file=976043#file976043line797>
> >
> > How will this work, when we can't access the mocked resource estimator from the test body?
You were right - default MockResourceEstimator behaviour needed. Fixed that.
> On June 2, 2015, 4:24 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 93
> > <https://reviews.apache.org/r/34816/diff/7/?file=976044#file976044line93>
> >
> > Shouldn't we have 'using process::Queue;' somewhere above?
Actually - we have.
> On June 2, 2015, 4:24 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 267
> > <https://reviews.apache.org/r/34816/diff/7/?file=976044#file976044line267>
> >
> > Why did you change the cpu counts?
I thought - it would more intersting test case - to shrink slack resources. I reverted that (:
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86235
-----------------------------------------------------------
On June 2, 2015, 7:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86235
-----------------------------------------------------------
Looks good! Final questions, tiny nits and let's get this in!
src/tests/mesos.hpp
<https://reviews.apache.org/r/34816/#comment138156>
How will this work, when we can't access the mocked resource estimator from the test body?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138150>
One newline
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138157>
Shouldn't we have 'using process::Queue;' somewhere above?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138159>
Mind moving the comment above line 92 (before the chain?) Here and below
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138154>
fits on one line
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138151>
Why did you change the cpu counts?
- Niklas Nielsen
On June 1, 2015, 5:38 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 1, 2015, 5:38 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 2, 2015, 8:05 p.m., Vinod Kone wrote:
> > src/tests/mesos.hpp, lines 716-723
> > <https://reviews.apache.org/r/34816/diff/8/?file=976766#file976766line716>
> >
> > You want to use both ON_CALL and EXPECT_CALL. See TestAllocator in tests/mesos.hpp for the rationale.
+1
However, i can see quite a mess here in tests/mesos.hpp, because every other Test/Mock class (except TestAllocator) uses EXPECT_CALL and WillRepeatedly ):
> On June 2, 2015, 8:05 p.m., Vinod Kone wrote:
> > src/tests/oversubscription_tests.cpp, lines 90-91
> > <https://reviews.apache.org/r/34816/diff/8/?file=976767#file976767line90>
> >
> > You don't need to explicitly do this you setup both ON_CALL and EXPECT_CALL above.
I think there is a need - i want to call initialize only ONCE.
Without this line there will be *WillRepeatedly* action when i setup ON_CALL and EXPECT_CALL above without any restriction to call count.
However - i can change that to:
```
EXPECT_CALL(resourceEstimator, initialize())
.Times(1);
```
...to be more consistent. Is that more preferable?
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86271
-----------------------------------------------------------
On June 2, 2015, 7:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86271
-----------------------------------------------------------
src/tests/mesos.hpp
<https://reviews.apache.org/r/34816/#comment138227>
You want to use both ON_CALL and EXPECT_CALL. See TestAllocator in tests/mesos.hpp for the rationale.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138228>
You don't need to explicitly do this you setup both ON_CALL and EXPECT_CALL above.
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138230>
Why is this *action* pulled up? What if the schedule receives an offer before #194 gets executed?
As a general rule, an action should come after an expectation is set and before the expectation is evaluated.
- Vinod Kone
On June 2, 2015, 7:37 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 7:37 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86289
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On June 2, 2015, 9:19 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 9:19 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 9:19 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Dropped times(1) since it's implicitly done.
Repository: mesos
Description
-------
TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
> On June 2, 2015, 9:08 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 91
> > <https://reviews.apache.org/r/34816/diff/9/?file=976795#file976795line91>
> >
> > This is implicit and you can remove it :)
> >
> > Here and below
oh, sure (:
- Bartek
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86283
-----------------------------------------------------------
On June 2, 2015, 8:36 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 8:36 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/#review86283
-----------------------------------------------------------
Ship it!
src/tests/mesos.hpp
<https://reviews.apache.org/r/34816/#comment138252>
Any reason for WillByDefault(..)?
EXPECT_CALL(...)
.WillRepeatedly(Return(...));
Should work, right?
src/tests/oversubscription_tests.cpp
<https://reviews.apache.org/r/34816/#comment138253>
This is implicit and you can remove it :)
Here and below
- Niklas Nielsen
On June 2, 2015, 1:36 p.m., Bartek Plotka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34816/
> -----------------------------------------------------------
>
> (Updated June 2, 2015, 1:36 p.m.)
>
>
> Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
>
>
> Diffs
> -----
>
> src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
> src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
>
> Diff: https://reviews.apache.org/r/34816/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Bartek Plotka
>
>
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 8:36 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
issue addressed.
Repository: mesos
Description
-------
TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 7:37 p.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
All issues addressed.
Repository: mesos
Description
-------
TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 12:38 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Removed unused include
Repository: mesos
Description
-------
TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 12:20 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Repository: mesos
Description (updated)
-------
TestResourceEstimator changed to be a MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 12:18 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Moved estimation queue to each test from test fixture.
Repository: mesos
Description
-------
This patch prepares Resource Estimator tests for modularized ResourceEstimator.
It includes:
1. Made oversubscription_test to be typed_test.
2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 2, 2015, 12:02 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Issue addressed.
Repository: mesos
Description
-------
This patch prepares Resource Estimator tests for modularized ResourceEstimator.
It includes:
1. Made oversubscription_test to be typed_test.
2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka
Re: Review Request 34816: Changed TestResourceEstimator to be mocked.
Posted by Bartek Plotka <bw...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34816/
-----------------------------------------------------------
(Updated June 1, 2015, 5:37 a.m.)
Review request for mesos, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone.
Changes
-------
Fixed last issue.
Repository: mesos
Description
-------
This patch prepares Resource Estimator tests for modularized ResourceEstimator.
It includes:
1. Made oversubscription_test to be typed_test.
2. TestResourceEstimator changed to be a templated MockResourceEstimator with mocked methods - to inject resources and test plumbing between RE and Slave.
Diffs (updated)
-----
src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b
src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f
Diff: https://reviews.apache.org/r/34816/diff/
Testing
-------
make check
Thanks,
Bartek Plotka