mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@apache.org>
Subject Re: Review Request 72840: Exposed offer constraints via the `/state` and `/frameworks` endpoints.
Date Fri, 11 Sep 2020 18:03:51 GMT


> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 914 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line915>
> >
> >     Remove this? and consider putting it on the assertion instead?
> >     
> >     E.g.
> >     
> >     ```
> >     ASSERT_SOME_EQ(JSON::protobuf(updatedConstraints), reportedConstraints) <<
"Expected: " << ... "\n vs actual: " << ...;
> >     ```
> >     
> >     Is this not printing out the two correctly upon failure?

Whoops, missed that line.

In fact, on failure `ASSERT_SOME_EQ` for `JSON:Object`s prints what we want without any custom
logging, like:
```
src/tests/master/update_framework_tests.cpp:939: Failure
Value of: (reportedConstraints).get()
  Actual: {"role_constraints":{"*":{"groups":[{"attribute_constraints":[{"predicate":{"not_exists":{}},"selector":{"attribute_name":"foo"}}]}]}}}
Expected: JSON::protobuf(updatedConstraints)
Which is: {"role_constraints":{"*":{"groups":[{"attribute_constraints":[{"predicate":{"not_exists":{}},"selector":{"attribute_name":"foo"}}]}]},"bar":{}}}
```


> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 919 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line920>
> >
> >     It's a little hard to see what it's supposed to look like based on this, seems
preferrable to me to check the expected json in some fashion?
> >     
> >     E.g.
> >     
> >     ```
> >     JSON::Value expected = ...
> >       "offer_constraints: { ... }"
> >     ```
> >     
> >     Like we do in other tests, that way, you can read the test and know what the
API response is supposed to look like?

This one compares JSONs (and prints them on failure, see above); as the invariant this aims
to check is that the returned constraints are the same as what we feed in, it doesn't make
much sense to define the expected value right here.

However, I should probably follow your earlier suggestion and convert the constraints definitions
to JSON.
Sent a follow-up patch for that: https://reviews.apache.org/r/72864


> On Sept. 10, 2020, 2:03 a.m., Benjamin Mahler wrote:
> > src/tests/master/update_framework_tests.cpp
> > Lines 921 (patched)
> > <https://reviews.apache.org/r/72840/diff/1/?file=2239297#file2239297line922>
> >
> >     Any reason not to just implement the check?

Added '/frameworks' too.


- Andrei


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


On Sept. 11, 2020, 8:03 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72840/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2020, 8:03 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10179
>     https://issues.apache.org/jira/browse/MESOS-10179
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Exposed offer constraints via the `/state` and `/frameworks` endpoints.
> 
> 
> Diffs
> -----
> 
>   src/master/readonly_handler.cpp b1336f9aa849e826a78c3fe4bb83e3efeb186a31 
>   src/tests/master/update_framework_tests.cpp 87bc3c7d23ea76118473d9a4ec3c77e7a9d5a97e

> 
> 
> Diff: https://reviews.apache.org/r/72840/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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