You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@mesos.apache.org by Andrei Sekretenko <as...@mesosphere.io> on 2019/04/23 18:54:36 UTC

Review Request 70534: Added tests for UPDATE_FRAMEWORK.

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
-------

Added tests for UPDATE_FRAMEWORK.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/1/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['70530', '70531', '70532', '70533', '70534']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3266/mesos-review-70534

Relevant logs:

- [mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3266/mesos-review-70534/logs/mesos-tests.log):

```
I0426 04:27:37.686568 69352 master.cpp:3373] Disconnecting agent e8051502-c696-4aa8-a963-57ba79fad735-S0 at slave(512)@192.10.1.4:62295 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0426 04:27:37.686568 69352 master.cpp:3392] Deactivating agent e8051502-c696-4aa8-a963-57ba79fad735-S0 at slave(512)@192.10.1.4:62295 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0426 04:27:37.687580 78548 hierarchical.cpp:392] Removed framework e8051502-c696-4aa8-a963-57ba79fad735-0000
I0426 04:27:37.687580 78548 hierarchical.cpp:829] Agent e8051502-c696-4aa8-a963-57ba79fad735-S0 deactivated
I0426 04:27:37.688581 69352 containerizer.cpp:2576] Destroying container cb5c31b7-4c1f-4a7e-ae6b-d87717915443 in RUNNING state
I0426 04:27:37.689572 69352 containerizer.cpp:3278] Transitioning the state of container cb5c31b7-4c1f-4a7e-ae6b-d87717915443 from RUNNING to DESTROYING
I0426 04:27:37.689572 69352 launcher.cpp:161] Asked to destroy container cb5c31b7-4c1f-4a7e-ae6b-d87717915443
W0426 04:27:37.691577 28596 process.cpp:1423] Failed to recv on socket WindowsFD::Type::SOCKET=10320 to peer '192.10.1.4:64708': IO failed with error code: The specified network name is no longer available.

W0426 04:27:37.692569 28596 process.cpp:838] Failed to recv on socket WindowsFD::Type::SOCKET=10284 to peer '192.10.1.4:64709': IO f[       OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (787 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (809 ms total)

[----------] Global test environment tear-down
[==========] 1174 tests from 110 test cases ran. (591267 ms total)
[  PASSED  ] 1171 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchManifest
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 3 FAILED TESTS
  YOU HAVE 233 DISABLED TESTS

ailed with error code: The specified network name is no longer available.

I0426 04:27:37.768610 81004 containerizer.cpp:3117] Container cb5c31b7-4c1f-4a7e-ae6b-d87717915443 has exited
I0426 04:27:37.800587 80336 master.cpp:1135] Master terminating
I0426 04:27:37.802618 81944 hierarchical.cpp:680] Removed agent e8051502-c696-4aa8-a963-57ba79fad735-S0
I0426 04:27:39.024624 28596 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On April 23, 2019, 7:05 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated April 23, 2019, 7:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for UPDATE_FRAMEWORK.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70408, 70377, 70379, 70530, 70531, 70532, 70533, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 23, 2019, 7:05 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated April 23, 2019, 7:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for UPDATE_FRAMEWORK.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['70408', '70377', '70379', '70530', '70531', '70532', '70533', '70534']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3246/mesos-review-70534

Relevant logs:

- [mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3246/mesos-review-70534/logs/mesos-tests.log):

```
I0423 22:30:37.016196 75760 master.cpp:3368] Disconnecting agent 2712aadd-b7f0-422f-b8ae-a899c96bdfb0-S0 at slave(510)@192.10.1.4:57800 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0423 22:30:37.016196 75760 master.cpp:3387] Deactivating agent 2712aadd-b7f0-422f-b8ae-a899c96bdfb0-S0 at slave(510)@192.10.1.4:57800 (windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0423 22:30:37.017210 75380 hierarchical.cpp:392] Removed framework 2712aadd-b7f0-422f-b8ae-a899c96bdfb0-0000
I0423 22:30:37.017210 75380 hierarchical.cpp:829] Agent 2712aadd-b7f0-422f-b8ae-a899c96bdfb0-S0 deactivated
I0423 22:30:37.018199 67380 containerizer.cpp:2576] Destroying container ab95a0c9-47af-4055-a364-12c2556f635a in RUNNING state
I0423 22:30:37.018199 67380 containerizer.cpp:3278] Transitioning the state of container ab95a0c9-47af-4055-a364-12c2556f635a from RUNNING to DESTROYING
I0423 22:30:37.019191 67380 launcher.cpp:161] Asked to destroy container ab95a0c9-47af-4055-a364-12c2556f635a
W0423 22:30:37.020185 74508 process.cpp:1423] Failed to recv on socket WindowsFD::Type::SOCKET=15216 to peer '192.10.1.4:60227': IO failed with error code: The specified n[       OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (784 ms)
[----------] 1 test from IsolationFlag/MemoryIsolatorTest (802 ms total)

[----------] Global test environment tear-down
[==========] 1172 tests from 110 test cases ran. (581750 ms total)
[  PASSED  ] 1169 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchManifest
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 3 FAILED TESTS
  YOU HAVE 233 DISABLED TESTS

etwork name is no longer available.

W0423 22:30:37.021200 74508 process.cpp:838] Failed to recv on socket WindowsFD::Type::SOCKET=15176 to peer '192.10.1.4:60228': IO failed with error code: The specified network name is no longer available.

I0423 22:30:37.113555 75380 containerizer.cpp:3117] Container ab95a0c9-47af-4055-a364-12c2556f635a has exited
I0423 22:30:37.144559 75876 master.cpp:1135] Master terminating
I0423 22:30:37.145747 75380 hierarchical.cpp:680] Removed agent 2712aadd-b7f0-422f-b8ae-a899c96bdfb0-S0
I0423 22:30:38.125605 74508 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On April 23, 2019, 7:05 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated April 23, 2019, 7:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for UPDATE_FRAMEWORK.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70666, 70667, 70668, 70669, 70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 17, 2019, 8:19 a.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 17, 2019, 8:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.


> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 417 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line417>
> >
> >     Can you explain what this is testing?

Split this test into two, tried to come up with better names for them and added a comment to each - I hope now it is more readable.


> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 429-430 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line429>
> >
> >     Why is this customization needed?

Not needed at all - removed it. Also tried to remove other remaining customization wherever possible.


- Andrei


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


On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 28, 2019, 5:36 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

> On May 24, 2019, 3:59 p.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 434-438 (patched)
> > <https://reviews.apache.org/r/70534/diff/7/?file=2146074#file2146074line434>
> >
> >     This looks racy?
> >     
> >     The expectation should be set up prior to the agent starting, but do we even care to test that there's an event for it in this test? It seems to be the responsibility of other tests to care about agent lifecycle events.

Indeed, this is misplaced and has at least two races: a high-level one (setting expectation vs the actual event) and a low-level one (calling EXPECT_CALL vs calling the mock method). I'll fix this and take care of similar problems elsewhere in these patches.

The intention behind this AWAIT was to keep a fixed ordering between `allocator->addSlave()` and `allocator->updateFramework()`. 
I do not think it is a good idea to have a test in which these two calls occur in some arbitrary (not random, but arbitrary!) order.
Currently only the case "update after a slave is added" is tested. If we need to test a second case, it probably makes sense to have a second test.

I'm also not sure if waiting for AGENT_ADDED is the best way to ensure the ordering (waiting for dispatch on `MesosAllocatorProcess::addSlave` could probably be another option).

Even if we settle on waiting for AGENT_ADDED, I'll have to write a better comment.

- Andrei

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

On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Benjamin Mahler <bm...@apache.org>.

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



Overall looking pretty good, thanks! Can you be sure to run the test in repetition to ensure there's no flakyness?


src/tests/master/update_framework_tests.cpp
Lines 88 (patched)
<https://reviews.apache.org/r/70534/#comment302217>

    {}; can go on the previous line



src/tests/master/update_framework_tests.cpp
Lines 93 (patched)
<https://reviews.apache.org/r/70534/#comment302213>

    It seems strange to pass the `id` here.. can't we use the one in info and ensure that callers set it?



src/tests/master/update_framework_tests.cpp
Lines 98-103 (patched)
<https://reviews.apache.org/r/70534/#comment302212>

    In general I would suggest we prefer the following syntax pattern:
    
    ```
     *call.mutable_framework_id() = id;
     *call.mutable_update_framework()->mutable_framework_info() = info;
     *call.mutable_update_framework()->mutable_framework_info()
       ->mutable_id() = id;
    ```
    
    Because it allows moves to happen when the rhs is an rvalue:
    
    ```
    *msg.mutable_foo() = func(); // will move :)
    msg.mutable_foo()->CopyFrom(func()); // will copy :(
    ```



src/tests/master/update_framework_tests.cpp
Lines 124 (patched)
<https://reviews.apache.org/r/70534/#comment302214>

    pull this up into the previous line?



src/tests/master/update_framework_tests.cpp
Lines 131 (patched)
<https://reviews.apache.org/r/70534/#comment302215>

    newline



src/tests/master/update_framework_tests.cpp
Lines 134 (patched)
<https://reviews.apache.org/r/70534/#comment302216>

    newline



src/tests/master/update_framework_tests.cpp
Lines 145 (patched)
<https://reviews.apache.org/r/70534/#comment302218>

    s/C/c/



src/tests/master/update_framework_tests.cpp
Lines 147-149 (patched)
<https://reviews.apache.org/r/70534/#comment302211>

    How about a CHECK_EQ with a helpful `<<` message so that the reader knows why this is failing from the logs without having to come in here and read the code?



src/tests/master/update_framework_tests.cpp
Lines 176 (patched)
<https://reviews.apache.org/r/70534/#comment302219>

    s/FrameworkInfosDiff/diff/?



src/tests/master/update_framework_tests.cpp
Lines 182 (patched)
<https://reviews.apache.org/r/70534/#comment302220>

    This seems odd, can't the caller fill this in for the initial FrameworkInfo so that we don't ignore it? if the `id` is set to two different values, we want to catch that.



src/tests/master/update_framework_tests.cpp
Lines 197-198 (patched)
<https://reviews.apache.org/r/70534/#comment302221>

    Can you declare this lower down closer to where it's used? We can also pass in the DEFAULT_FRAMEWORK_INFO **with** `id` assigned based on the subscription.
    
    Ditto for the other tests



src/tests/master/update_framework_tests.cpp
Lines 219 (patched)
<https://reviews.apache.org/r/70534/#comment302222>

    newline, ditto for the other tests



src/tests/master/update_framework_tests.cpp
Lines 230 (patched)
<https://reviews.apache.org/r/70534/#comment302236>

    newline, ditto other tests



src/tests/master/update_framework_tests.cpp
Lines 325 (patched)
<https://reviews.apache.org/r/70534/#comment302237>

    A role not in the whitelist? Don't we have a test for that somewhere already? Or are you thinking specifically when an UPDATE_FRAMEWORK uses a non-whitelisted role?



src/tests/master/update_framework_tests.cpp
Lines 326 (patched)
<https://reviews.apache.org/r/70534/#comment302238>

    Ditto here, I guess we test this already somewhere (maybe) but this is just to make sure the UPDATE_FRAMEWORK path goes throug that check?



src/tests/master/update_framework_tests.cpp
Lines 332 (patched)
<https://reviews.apache.org/r/70534/#comment302239>

    exit code?



src/tests/master/update_framework_tests.cpp
Lines 391 (patched)
<https://reviews.apache.org/r/70534/#comment302234>

    remove this?



src/tests/master/update_framework_tests.cpp
Lines 397 (patched)
<https://reviews.apache.org/r/70534/#comment302235>

    newline



src/tests/master/update_framework_tests.cpp
Lines 417 (patched)
<https://reviews.apache.org/r/70534/#comment302224>

    Can you explain what this is testing?



src/tests/master/update_framework_tests.cpp
Lines 421 (patched)
<https://reviews.apache.org/r/70534/#comment302223>

    newline



src/tests/master/update_framework_tests.cpp
Lines 424-425 (patched)
<https://reviews.apache.org/r/70534/#comment302228>

    See my comment below, it seems we don't need this?



src/tests/master/update_framework_tests.cpp
Lines 429-430 (patched)
<https://reviews.apache.org/r/70534/#comment302226>

    Why is this customization needed?



src/tests/master/update_framework_tests.cpp
Lines 434-438 (patched)
<https://reviews.apache.org/r/70534/#comment302227>

    This looks racy?
    
    The expectation should be set up prior to the agent starting, but do we even care to test that there's an event for it in this test? It seems to be the responsibility of other tests to care about agent lifecycle events.



src/tests/master/update_framework_tests.cpp
Lines 454 (patched)
<https://reviews.apache.org/r/70534/#comment302229>

    For consistency, we put the .WillOnce, .Times, etc on the next line, can you do a sweep in this file?



src/tests/master/update_framework_tests.cpp
Lines 456 (patched)
<https://reviews.apache.org/r/70534/#comment302230>

    I hadn't considered the case where a framework has no roles, I suppose we can allow that..



src/tests/master/update_framework_tests.cpp
Lines 469 (patched)
<https://reviews.apache.org/r/70534/#comment302232>

    newline



src/tests/master/update_framework_tests.cpp
Lines 472 (patched)
<https://reviews.apache.org/r/70534/#comment302231>

    s/.get()./->/ (please do a scan of this file)
    
    Seems like 1 offer is expected given the single rescind expectation below, might as well ensure it's 1 here?



src/tests/master/update_framework_tests.cpp
Lines 499 (patched)
<https://reviews.apache.org/r/70534/#comment302233>

    Pass `masterFlags.allocation_interval` instead


- Benjamin Mahler


On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Benjamin Mahler <bm...@apache.org>.

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


Fix it, then Ship it!




Nice tests, thank you Andrei!


src/tests/master/update_framework_tests.cpp
Lines 150 (patched)
<https://reviews.apache.org/r/70534/#comment302617>

    s/update/newInfo/



src/tests/master/update_framework_tests.cpp
Lines 150 (patched)
<https://reviews.apache.org/r/70534/#comment302618>

    how about: s/update/newInfo/



src/tests/master/update_framework_tests.cpp
Lines 157-158 (patched)
<https://reviews.apache.org/r/70534/#comment302614>

    This seems a little brittle, as we probably should be failing FrameworkInfo validation when there is an UNKNOWN capability. Seems like a bug that it's not getting checked
    
    How about REGION_AWARE?



src/tests/master/update_framework_tests.cpp
Lines 164-169 (patched)
<https://reviews.apache.org/r/70534/#comment302615>

    Remove "without_resources"? I'm not sure what it's trying to communicate, but it seems to carry too much context from somewhere else, is it named this way because of a test?



src/tests/master/update_framework_tests.cpp
Lines 170 (patched)
<https://reviews.apache.org/r/70534/#comment302619>

    newline



src/tests/master/update_framework_tests.cpp
Lines 174 (patched)
<https://reviews.apache.org/r/70534/#comment302620>

    s/std::string/string/ here and below



src/tests/master/update_framework_tests.cpp
Lines 274 (patched)
<https://reviews.apache.org/r/70534/#comment302621>

    As an aside, the error above says "Updating" and this one says "Changing", should probably make the language consistent



src/tests/master/update_framework_tests.cpp
Lines 370-375 (patched)
<https://reviews.apache.org/r/70534/#comment302622>

    This looks racy..?
    
    The agent may get added before the subscriber subscribes, in which case the AGENT_ADDED event wouldn't be sent because the agent gets included in the initial SUBSCRIBE state.



src/tests/master/update_framework_tests.cpp
Lines 493 (patched)
<https://reviews.apache.org/r/70534/#comment302624>

    Why resume and re-pause the clock?



src/tests/master/update_framework_tests.cpp
Lines 512 (patched)
<https://reviews.apache.org/r/70534/#comment302625>

    Why resume?



src/tests/master/update_framework_tests.cpp
Lines 562-565 (patched)
<https://reviews.apache.org/r/70534/#comment302631>

    ```
      Future<Event::Rescind> rescind;
      EXPECT_CALL(*scheduler, rescind(_, _))
        .WillOnce(FutureArg<1>(&rescind));
    ```



src/tests/master/update_framework_tests.cpp
Lines 569-570 (patched)
<https://reviews.apache.org/r/70534/#comment302629>

    ```
      // TODO(asekretenko): Add a more in-depth check that the
      // allocator does what it should.
    ```


- Benjamin Mahler


On June 3, 2019, 8:14 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated June 3, 2019, 8:14 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/9/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated June 3, 2019, 8:14 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
-------

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/9/

Changes: https://reviews.apache.org/r/70534/diff/8-9/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 28, 2019, 5:36 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 70671, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 28, 2019, 5:36 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 28, 2019, 5:36 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 28, 2019, 5:36 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
-------

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/8/

Changes: https://reviews.apache.org/r/70534/diff/7-8/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 21, 2019, 2:10 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
-------

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/7/

Changes: https://reviews.apache.org/r/70534/diff/6-7/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 21, 2019, 1:56 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
-------

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/6/

Changes: https://reviews.apache.org/r/70534/diff/5-6/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 17, 2019, 3:19 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-----------------

Added tests for the V1 UPDATE_FRAMEWORK call.


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


Repository: mesos


Description (updated)
-------

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/5/

Changes: https://reviews.apache.org/r/70534/diff/4-5/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 7, 2019, 12:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 7, 2019, 12:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.


> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 98 (patched)
> > <https://reviews.apache.org/r/70534/diff/4/?file=2143581#file2143581line98>
> >
> >     What does the presence of agent reservations matter to testing UPDATE_FRAMEWORK?

Indeed, reservations are not necessary for testing that allocator gets the update.

We can update from no roles to one role and check for an offer - I rewrote the test.


> On May 12, 2019, 5:44 a.m., Benjamin Mahler wrote:
> > src/tests/update_framework_tests.cpp
> > Lines 390-421 (patched)
> > <https://reviews.apache.org/r/70534/diff/4/?file=2143581#file2143581line390>
> >
> >     Is there no mock for this yet? How was the event streaming API tested without it?
> >     
> >     How about pulling this out in front of this patch into its own header so that we can review it on its own? It's independent and adds to the complexity of this review.

It was tested by explicitly reading out every event (example: https://github.com/apache/mesos/blob/91fa416bf0d6191bb1a5e1fa3c883e23c98e892c/src/tests/api_tests.cpp#L3396).
Surely, each time a new event type was introduced, one had to modify unrelated tests: https://reviews.apache.org/r/61262/diff/8#index_header

Moved this mock into https://reviews.apache.org/r/70671/


- Andrei


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


On May 17, 2019, 3:19 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 17, 2019, 3:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

Posted by Benjamin Mahler <bm...@apache.org>.

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



Thanks for the tests!

Having a test file just for a specific scheduler::Call doesn't fit our existing tests well. It seems reasonable to have its own file, but we could badly use some organization. This seems to be a "master test" but it does seem nice to cleanly have its own file.

How about a tests/master/ folder under which we move the master tests, as well as this test?


src/tests/update_framework_tests.cpp
Lines 88-94 (patched)
<https://reviews.apache.org/r/70534/#comment301791>

    These types of functions lead to low readability. When reading tests, I can't intuit what this function does and I need to read the code to know that it puts in a special role.
    
    In general, if we can't come up with a function that has some clear meaning, we will inline the code. This means that while we will have duplication, the test will be more readable (the reader doesn't need to jump up to this function and read what it does to understand the test).



src/tests/update_framework_tests.cpp
Lines 96-102 (patched)
<https://reviews.apache.org/r/70534/#comment301794>

    We generally try to avoid such heavy context test fixtures if possible, since they reduce readability.
    
    The reader of any of the tests below needs a lot of context about what the fixture has set up to understand how the test works.
    
    If possible it's better to compose pieces together to achieve the test.
    
    For example, I imagine the whitelist is only relevant to a few of the tests, so no need to have it for all of them.
    
    The agent reservations, I'm not quite sure how they're relevant, there's no indication of why we need them here so I would have to read all the tests to find out.
    
    The scheduler, well, we can just use it in each test, no need to have a fixture for it.



src/tests/update_framework_tests.cpp
Lines 98 (patched)
<https://reviews.apache.org/r/70534/#comment301793>

    What does the presence of agent reservations matter to testing UPDATE_FRAMEWORK?



src/tests/update_framework_tests.cpp
Lines 177 (patched)
<https://reviews.apache.org/r/70534/#comment301792>

    On the other hand, this function seems rather intuitive. I could figure out what it does from the call site. However.. I would get a bit confused from the fact that it returns a single FrameworkInfo, since the master can have several.
    
    How about returning a vector of FrameworkInfo here and the caller ASSERTs that its size is 1.



src/tests/update_framework_tests.cpp
Lines 390-421 (patched)
<https://reviews.apache.org/r/70534/#comment301795>

    Is there no mock for this yet? How was the event streaming API tested without it?
    
    How about pulling this out in front of this patch into its own header so that we can review it on its own? It's independent and adds to the complexity of this review.



src/tests/update_framework_tests.cpp
Lines 477 (patched)
<https://reviews.apache.org/r/70534/#comment301796>

    Do you have a bad indentation setting in your code editor? There seem to be a lot of 4 space indents in this chain of patches


- Benjamin Mahler


On May 7, 2019, 12:13 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 7, 2019, 12:13 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for the UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 7, 2019, 12:13 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
-------

Added a comment and TODO for the MasterAPISubscriber based on the review description; cleared the description.


Summary (updated)
-----------------

Added tests for the UPDATE_FRAMEWORK call.


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


Repository: mesos


Description (updated)
-------

Added tests for the UPDATE_FRAMEWORK call.


Diffs (updated)
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/4/

Changes: https://reviews.apache.org/r/70534/diff/3-4/


Testing
-------


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.

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



Patch looks great!

Reviews applied: [70583, 70531, 70530, 70532, 70533, 70534]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On May 2, 2019, 4:06 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> -----------------------------------------------------------
> 
> (Updated May 2, 2019, 4:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds tests for the UPDATE_FRAMEWORK call.
> 
> The reason behind implementing the MasterAPISubscriber class with mock methods is the need to wait for interesting API events while skipping uninteresting ones. 
> This is needed to properly test the FRAMEWORK_UPDATED event sent by the master (for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip everything else).
> 
> I'm in doubts about the proper location for the MasterAPISubscriber. On one hand, it is used only by these tests and the implementation is by no means complete; on the other hand, this might be useful in other tests of the master API behaviour. 
> Probably, turning the MasterAPISubscriber into a general-purpose mock (and moving it into another file at that point) should be a separate task.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated May 2, 2019, 4:06 p.m.)

Review request for mesos and Benjamin Mahler.

Changes
-------

Added tests for updates sent by the master to slaves and API subscribers.

Summary (updated)
-----------------

Added tests for UPDATE_FRAMEWORK.

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

Repository: mesos

Description
-------

This patch adds tests for the UPDATE_FRAMEWORK call.

The reason behind implementing the MasterAPISubscriber class with mock methods is the need to wait for interesting API events while skipping uninteresting ones.
This is needed to properly test the FRAMEWORK_UPDATED event sent by the master (for that I need to wait for AGENT_ADDED and FRAMEWORK_UPDATED, and to skip everything else).

I'm in doubts about the proper location for the MasterAPISubscriber. On one hand, it is used only by these tests and the implementation is by no means complete; on the other hand, this might be useful in other tests of the master API behaviour.
Probably, turning the MasterAPISubscriber into a general-purpose mock (and moving it into another file at that point) should be a separate task.

Diffs (updated)
-----

src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/update_framework_tests.cpp PRE-CREATION

Diff: https://reviews.apache.org/r/70534/diff/3/

Changes: https://reviews.apache.org/r/70534/diff/2-3/

Testing
-------

Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the UPDATE_FRAMEWORK call.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

(Updated May 2, 2019, 4:04 p.m.)

Review request for mesos and Benjamin Mahler.

Changes
-------

Added tests for updates sent by the master to slaves and API subscribers.

Summary (updated)
-----------------

Added tests for the UPDATE_FRAMEWORK call.

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

Repository: mesos

Description (updated)
-------

This patch adds tests for the UPDATE_FRAMEWORK call.

Diffs (updated)
-----

src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/update_framework_tests.cpp PRE-CREATION

Diff: https://reviews.apache.org/r/70534/diff/2/

Changes: https://reviews.apache.org/r/70534/diff/1-2/

Testing
-------

Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for UPDATE_FRAMEWORK.

Posted by Andrei Sekretenko <as...@mesosphere.io>.

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

(Updated April 23, 2019, 7:05 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
-------

Added tests for UPDATE_FRAMEWORK.


Diffs
-----

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/1/


Testing
-------


Thanks,

Andrei Sekretenko