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



Great to have unit tests for this! Mainly left a suggestion about using a JSON approach to build the protobuf.

Do we want to add another test here for attributes with multiple entries? (i.e. showing that we only look at the first entry)


src/tests/master/offer_constraints_filter_tests.cpp
Lines 72-76 (patched)
<https://reviews.apache.org/r/72742/#comment310694>

    Hm.. see my other comment below, might be able to avoid the `singleAttributeConstraintFilter` by using the same JSON approach to set up the protobuf in all the tests? It would also be nice to have all the OfferConstraintsFilter construction consistent (rather than some going through a helper and some not).



src/tests/master/offer_constraints_filter_tests.cpp
Lines 196-211 (patched)
<https://reviews.apache.org/r/72742/#comment310693>

    For this and the others above, it's often pretty hard for a reader to parse through what the protobuf is with setting it up in C++, is it possible to just do a json conversion here as we've done in some other places?
    
    ```
    Try<JSON::Object> json = JSON::parse<JSON::Object>(
        R"~(
        {
          "role_constraints": {
            "role1": {
              "attribute_constraints": [{
                "selector": {"attribute_name": "foo"},
                "predicate": {"exists": {}}
              }]
            },
            "role2": {
              "attribute_constraints": [{
                "selector": {"attribute_name": "bar"},
                "predicate": {"not_exists": {}}
              }]
            }
          }
        })~");
        
    ASSERT_SOME(json);
    
    Try<OfferConstraints> constraints = protobuf::parse(*json);
    
    ASSERT_SOME(constraints);
    ```
    
    This makes it really easy on the reader to see what the constraints look like.


- Benjamin Mahler


On Aug. 14, 2020, 4:59 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72742/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2020, 4:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10171
>     https://issues.apache.org/jira/browse/MESOS-10171
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added basic tests for the `OfferConstraintsFilter`.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 447db323875e4cad46000977f4a61600baff8f89 
>   src/tests/CMakeLists.txt cf579f8d6e5176e0d4eadabcbadcbb99aa6c737e 
>   src/tests/master/offer_constraints_filter_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/72742/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>