You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/03/07 16:59:13 UTC

Review Request 70156: Added helper to test for metrics values.

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

Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Repository: mesos


Description
-------

Added helper to test for metrics values.


Diffs
-----

  src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 


Diff: https://reviews.apache.org/r/70156/diff/1/


Testing
-------


Thanks,

Benno Evers

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Benno Evers <be...@mesosphere.com>.

> On March 7, 2019, 6:41 p.m., Joseph Wu wrote:
> > Hum... I wonder if it is worthwhile to make this a normal function call instead.
> > ```
> > <template T>
> > bool metricsEqual(string metric, T value) {
> >   JSON::Object metrics = Metrics();
> >   if (metrics.count(metric) != 1) {
> >     return false;
> >   }
> >   
> >   return metrics.at<JSON::Number>(metric)->as<T>() == value;
> > }
> > ```
> > With usage like:
> > ```
> > ASSERT_TRUE(metricsEqual("metric/foo", 1));
> > ```

Good point! That also allows getting rid of the annoying `ASSERT()` inside a statement called `EXPECT_[...]`.


> On March 7, 2019, 6:41 p.m., Joseph Wu wrote:
> > src/tests/utils.hpp
> > Lines 40 (patched)
> > <https://reviews.apache.org/r/70156/diff/1/?file=2129554#file2129554line40>
> >
> >     Is it necessary to make this a `do { ... } while (0);` instead of just enclosing this in a `{ ... }`?

The advantage of the `do-while` construct is that the compiler will give you an error if you forget the semicolon after `EXPECT_METRICS_EQ();`, apart from this I think the two are more or less equivalent.


> On March 7, 2019, 6:41 p.m., Joseph Wu wrote:
> > src/tests/utils.hpp
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/70156/diff/1/?file=2129554#file2129554line41>
> >
> >     I can imagine a name conflict if the test is particularly metric heavy.  i.e. if the test defines another `metrics` local variable.

That's fine, the macro is introducing a new scope.


- Benno


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


On March 7, 2019, 4:59 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> -----------------------------------------------------------
> 
> (Updated March 7, 2019, 4:59 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added helper to test for metrics values.
> 
> 
> Diffs
> -----
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70156/#review213530
-----------------------------------------------------------



Hum... I wonder if it is worthwhile to make this a normal function call instead.
```
<template T>
bool metricsEqual(string metric, T value) {
  JSON::Object metrics = Metrics();
  if (metrics.count(metric) != 1) {
    return false;
  }
  
  return metrics.at<JSON::Number>(metric)->as<T>() == value;
}
```
With usage like:
```
ASSERT_TRUE(metricsEqual("metric/foo", 1));
```


src/tests/utils.hpp
Lines 39 (patched)
<https://reviews.apache.org/r/70156/#comment299481>

    Nit: A glance at some other macros show that we space-align the `` characters to the right side.



src/tests/utils.hpp
Lines 40 (patched)
<https://reviews.apache.org/r/70156/#comment299480>

    Is it necessary to make this a `do { ... } while (0);` instead of just enclosing this in a `{ ... }`?



src/tests/utils.hpp
Lines 41 (patched)
<https://reviews.apache.org/r/70156/#comment299482>

    I can imagine a name conflict if the test is particularly metric heavy.  i.e. if the test defines another `metrics` local variable.


- Joseph Wu


On March 7, 2019, 8:59 a.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> -----------------------------------------------------------
> 
> (Updated March 7, 2019, 8:59 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added helper to test for metrics values.
> 
> 
> Diffs
> -----
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70156/
-----------------------------------------------------------

(Updated March 13, 2019, 5:45 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Changes
-------

Removed unnecessary double lookup.


Repository: mesos


Description
-------

This patch adds a new helper function to check
whether a given metric has some specified value.


Diffs (updated)
-----

  src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 


Diff: https://reviews.apache.org/r/70156/diff/3/

Changes: https://reviews.apache.org/r/70156/diff/2-3/


Testing
-------


Thanks,

Benno Evers

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70156/#review213611
-----------------------------------------------------------


Ship it!




Ship It!

- Joseph Wu


On March 8, 2019, 3:46 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> -----------------------------------------------------------
> 
> (Updated March 8, 2019, 3:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a new helper function to check
> whether a given metric has some specified value.
> 
> 
> Diffs
> -----
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70156/
-----------------------------------------------------------

(Updated March 8, 2019, 11:46 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Changes
-------

Turned macro into a function.


Repository: mesos


Description (updated)
-------

This patch adds a new helper function to check
whether a given metric has some specified value.


Diffs (updated)
-----

  src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 


Diff: https://reviews.apache.org/r/70156/diff/2/

Changes: https://reviews.apache.org/r/70156/diff/1-2/


Testing
-------


Thanks,

Benno Evers

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Benno Evers <be...@mesosphere.com>.

> On March 7, 2019, 11:49 p.m., Gastón Kleiman wrote:
> > src/tests/utils.hpp
> > Lines 39 (patched)
> > <https://reviews.apache.org/r/70156/diff/1/?file=2129554#file2129554line39>
> >
> >     Nit: I'd name this `EXPECT_METRIC_EQ`.

Fair point, I've used `metricEquals()` now that it isn't a macro anymore.


- Benno


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


On March 8, 2019, 11:46 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> -----------------------------------------------------------
> 
> (Updated March 8, 2019, 11:46 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch adds a new helper function to check
> whether a given metric has some specified value.
> 
> 
> Diffs
> -----
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70156/#review213540
-----------------------------------------------------------




src/tests/utils.hpp
Lines 39 (patched)
<https://reviews.apache.org/r/70156/#comment299492>

    Nit: I'd name this `EXPECT_METRIC_EQ`.


- Gastón Kleiman


On March 7, 2019, 8:59 a.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> -----------------------------------------------------------
> 
> (Updated March 7, 2019, 8:59 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added helper to test for metrics values.
> 
> 
> Diffs
> -----
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>