> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 107
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line107>
> >
> >     Is this necessary? The defaultAgentResourcesString is 
> > "cpus:2;mem:1024;disk:1024;ports:[31000-32000]"
> >     Overriding that means that Mesos will auto-detect cpus/mem

Personally, I think it is better to be more explicit: when I read the test case 
code, I can see that resource specification for the slave, rather than relying 
on `defaultAgentResourcesString` which is defined someplace else.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 120
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line120>
> >
> >     s/by default/the default/

```
$ ag "be filtered for 5 seconds (by default)" src/tests | wc -l
      18
```

i.e., this text occurs in many tests. I think we should use the same text in 
each place. I'll open a separate review request to change them all.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 385
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line385>
> >
> >     Seems we don't have any tests with weights yet. Congrats on writing the 
> > first!

Seems like we need tests for allocation behavior in the presence of weights, at 
a bare minimum. I filed a JIRA: https://issues.apache.org/jira/browse/MESOS-4200


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 47
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line47>
> >
> >     Why is this an instance member method of RoleTest? Seems idempotent and 
> > stateless enough that it could be a static method, or even a freestanding 
> > helper function. If this is only called once, why is it even a function at 
> > all?
> >     
> >     Oh.. it's copied from `MasterQuotaTest::createRequestBody()`.
> >     
> >     Either find a way to actually merge the two implementations, or just 
> > inline this into the one test that uses it.

Lots of tests use stateless methods in their `MesosTest` sub-class to do things 
like create HTTP request bodies. Not sure why, but I thought I'd be consistent 
with the existing convention.

I just got rid of the function.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 165-166
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line165>
> >
> >     Can you make these "volumeId1" and "/container/path" so it's clearer 
> > what they're supposed to be?

"id1" and "path1" matches the other call sites of `createPersistentVolume()`, 
so I'd vote for consistency. If you'd like me to change all the call-sites in a 
separate review, let me know.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 251-259
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line251>
> >
> >     Why bother to send back the ack? Or even send the status back at all? 
> > As soon as you have a MockExecutor that receives a launchTask, you've 
> > verified that "when using implicit roles, a static reservation can be used 
> > to launch a task"

After you do `driver.acceptOffers({offer.id()}, {LAUNCH({taskInfo})}, 
filters);`, how do you verify that the master has received the launch task 
attempt before the test case shuts down? It seems to me you need some form of 
synchronization (and indeed, removing the code that waits for the status ack 
causes the test to fail).


- Neil


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


On Dec. 18, 2015, 8:01 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41225/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2015, 8:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test cases for implicit roles.
> 
> 
> Diffs
> -----
> 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41225/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Reply via email to