Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/#review152054
---


Ship it!




LGTM

- Anand Mazumdar


On Oct. 10, 2016, 7:18 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52620/
> ---
> 
> (Updated Oct. 10, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4948
> https://issues.apache.org/jira/browse/MESOS-4948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated InverseOffers* maintenance tests to use the new scheduler mock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 77eb405ab7314da906bed9ec1d0018c24928d8d8 
> 
> Diff: https://reviews.apache.org/r/52620/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X 10.11 and Ubuntu 14.04.
> ```
> make check GTEST_FILTER='MasterMaintenanceTest.*' GTEST_REPEAT=1000 
> GTEST_BREAK_ON_FAILURE=1
> ```
> Noticed a segfault caused by getenv / setenv race (MESOS-3475), but it's 
> unrelated to this change and is reproducible on current master.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Ilya Pronin]

> On Oct. 7, 2016, 11:51 p.m., Anand Mazumdar wrote:
> > src/tests/master_maintenance_tests.cpp, lines 1169-1171
> > 
> >
> > We prefer to keep expectations close to the actual business logic for 
> > readability. Can you move this down to L1192?

I'm dropping this issue as discussed on IRC. The reason for it is that the 
framework will receive an inverse offer as soon as it subscribes. That's 
because as soon as the offer is sent out, the resources are "allocated", which 
causes the allocator to immediately send an inverse offer.


> On Oct. 7, 2016, 11:51 p.m., Anand Mazumdar wrote:
> > src/tests/master_maintenance_tests.cpp, lines 1529-1531
> > 
> >
> > Similar comment as per previous test. Move this expectation.

Dropping for the same reason as in previous test.


- Ilya


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/#review151890
---


On Oct. 10, 2016, 8:18 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52620/
> ---
> 
> (Updated Oct. 10, 2016, 8:18 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4948
> https://issues.apache.org/jira/browse/MESOS-4948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated InverseOffers* maintenance tests to use the new scheduler mock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 77eb405ab7314da906bed9ec1d0018c24928d8d8 
> 
> Diff: https://reviews.apache.org/r/52620/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X 10.11 and Ubuntu 14.04.
> ```
> make check GTEST_FILTER='MasterMaintenanceTest.*' GTEST_REPEAT=1000 
> GTEST_BREAK_ON_FAILURE=1
> ```
> Noticed a segfault caused by getenv / setenv race (MESOS-3475), but it's 
> unrelated to this change and is reproducible on current master.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Ilya Pronin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/
---

(Updated Oct. 10, 2016, 8:18 p.m.)


Review request for mesos and Anand Mazumdar.


Bugs: MESOS-4948
https://issues.apache.org/jira/browse/MESOS-4948


Repository: mesos


Description
---

Updated InverseOffers* maintenance tests to use the new scheduler mock.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
77eb405ab7314da906bed9ec1d0018c24928d8d8 

Diff: https://reviews.apache.org/r/52620/diff/


Testing (updated)
---

Tested on OS X 10.11 and Ubuntu 14.04.
```
make check GTEST_FILTER='MasterMaintenanceTest.*' GTEST_REPEAT=1000 
GTEST_BREAK_ON_FAILURE=1
```
Noticed a segfault caused by getenv / setenv race (MESOS-3475), but it's 
unrelated to this change and is reproducible on current master.


Thanks,

Ilya Pronin

Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/#review151923
---



Patch looks great!

Reviews applied: [52620]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Oct. 7, 2016, 5:29 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52620/
> ---
> 
> (Updated Oct. 7, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4948
> https://issues.apache.org/jira/browse/MESOS-4948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated InverseOffers* maintenance tests to use the new scheduler mock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 77eb405ab7314da906bed9ec1d0018c24928d8d8 
> 
> Diff: https://reviews.apache.org/r/52620/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OS X 10.11 and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/#review151890
---



Thanks Ilya. Looks pretty good!

- Mostly minor comments around moving expectations.
- Renaming some `EXPECT_EQ` to `ASSERT_EQ`. They are not related to this patch 
per se and I am fine if you want to create another patch to address them.

Also, can you run the maintenance tests with `--gtest_repeat=1000 
--gtest_break_on_failure` to ensure we do not introduce any flakiness to the 
tests as part of this refactoring?


src/tests/master_maintenance_tests.cpp (lines 1159 - 1161)


We prefer to keep expectations close to the actual business logic for 
readability. Can you move this down to L1192?



src/tests/master_maintenance_tests.cpp (lines 1189 - 1191)


Move this below the `launchTask()` expectation on L1198



src/tests/master_maintenance_tests.cpp (line 1222)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1251 - 1252)


Move this above L1273



src/tests/master_maintenance_tests.cpp (line 1281)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1316 - 1317)


Move this before L1338



src/tests/master_maintenance_tests.cpp (line 1346)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1380 - 1381)


You can remove this expectation i.e., the `master` would be deleted on the 
stack *after* the scheduler library, so the library won't ever see the 
disconnected callback. This was not the case earlier when this test was written 
but we can get rid of this now.



src/tests/master_maintenance_tests.cpp (lines 1496 - 1498)


Similar comment as per previous test. Move this expectation.



src/tests/master_maintenance_tests.cpp (line 1540)


s/firstUpdate/update1

To be consistent on how we name these variables else where in our code.



src/tests/master_maintenance_tests.cpp (line 1541)


s/firstUpdate/update2



src/tests/master_maintenance_tests.cpp (line 1580)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1594 - 1595)


Nit: No need to create the temporary `updateStatus`, directly do 
`update1->status().state()`



src/tests/master_maintenance_tests.cpp (lines 1615 - 1616)


Ditto



src/tests/master_maintenance_tests.cpp (lines 1670 - 1671)


Move this above L1689



src/tests/master_maintenance_tests.cpp (line 1695)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1705 - 1706)


Move this before L1725



src/tests/master_maintenance_tests.cpp (line 1731)


Not yours: s/EXPECT_EQ/ASSERT_EQ



src/tests/master_maintenance_tests.cpp (lines 1742 - 1743)


Kill this as per my earlier comment.


- Anand Mazumdar


On Oct. 7, 2016, 5:29 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52620/
> ---
> 
> (Updated Oct. 7, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4948
> https://issues.apache.org/jira/browse/MESOS-4948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated InverseOffers* maintenance tests to use the new scheduler mock.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 77eb405ab7314da906bed9ec1d0018c24928d8d8 
> 
> Diff: https://reviews.apache.org/r/52620/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OS X 10.11 and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52620: Updated InverseOffers* maintenance tests to use the new scheduler mock.

[Ilya Pronin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52620/
---

(Updated Oct. 7, 2016, 6:29 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Reposted the same patch to retrigger ReviewBot verification. Previous 
verification has failed presumingly because of flacky LocalStoreTestWithTar 
(MESOS-5139).


Bugs: MESOS-4948
https://issues.apache.org/jira/browse/MESOS-4948


Repository: mesos


Description
---

Updated InverseOffers* maintenance tests to use the new scheduler mock.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
77eb405ab7314da906bed9ec1d0018c24928d8d8 

Diff: https://reviews.apache.org/r/52620/diff/


Testing
---

`make check` on OS X 10.11 and Ubuntu 14.04.


Thanks,

Ilya Pronin