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