mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 72742: Added basic tests for the `OfferConstraintsFilter`.
Date Tue, 18 Aug 2020 18:36:51 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message