You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2016/11/30 12:41:52 UTC
Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/
-----------------------------------------------------------
Review request for mesos, Joseph Wu and Till Toenshoff.
Bugs: MESOS-4695
https://issues.apache.org/jira/browse/MESOS-4695
Repository: mesos
Description
-------
This test was checking two floating point values for equality by only
examining their integer part. Since one of the values went through
a number of conversions, even its integer part could have changed for
certain starting values.
This patch introduces an epsilon into the comparison to account for
possible changes in the integer part.
Diffs
-----
src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845
Diff: https://reviews.apache.org/r/54205/diff/
Testing
-------
`make check` passes.
I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
Thanks,
Benjamin Bannier
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/#review158673
-----------------------------------------------------------
Ship it!
Ship It!
- Till Toenshoff
On Dec. 8, 2016, 1:57 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54205/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:57 p.m.)
>
>
> Review request for mesos, Joseph Wu and Till Toenshoff.
>
>
> Bugs: MESOS-4695
> https://issues.apache.org/jira/browse/MESOS-4695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test was checking two floating point values for equality by only
> examining their integer part. Since one of the values went through
> a number of conversions, even its integer part could have changed for
> certain starting values.
>
> This patch introduces an epsilon into the comparison to account for
> possible changes in the integer part.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 967c7307b6fc8e9eb4490f51938bdcf286b5e7a6
>
> Diff: https://reviews.apache.org/r/54205/diff/
>
>
> Testing
> -------
>
> `make check` passes.
>
> I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/#review158534
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54205]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 8, 2016, 1:57 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54205/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 1:57 p.m.)
>
>
> Review request for mesos, Joseph Wu and Till Toenshoff.
>
>
> Bugs: MESOS-4695
> https://issues.apache.org/jira/browse/MESOS-4695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test was checking two floating point values for equality by only
> examining their integer part. Since one of the values went through
> a number of conversions, even its integer part could have changed for
> certain starting values.
>
> This patch introduces an epsilon into the comparison to account for
> possible changes in the integer part.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 967c7307b6fc8e9eb4490f51938bdcf286b5e7a6
>
> Diff: https://reviews.apache.org/r/54205/diff/
>
>
> Testing
> -------
>
> `make check` passes.
>
> I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/
-----------------------------------------------------------
(Updated Dec. 8, 2016, 2:57 p.m.)
Review request for mesos, Joseph Wu and Till Toenshoff.
Changes
-------
Address tillt's comment.
Bugs: MESOS-4695
https://issues.apache.org/jira/browse/MESOS-4695
Repository: mesos
Description
-------
This test was checking two floating point values for equality by only
examining their integer part. Since one of the values went through
a number of conversions, even its integer part could have changed for
certain starting values.
This patch introduces an epsilon into the comparison to account for
possible changes in the integer part.
Diffs (updated)
-----
src/tests/slave_tests.cpp 967c7307b6fc8e9eb4490f51938bdcf286b5e7a6
Diff: https://reviews.apache.org/r/54205/diff/
Testing
-------
`make check` passes.
I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
Thanks,
Benjamin Bannier
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/#review157405
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54205]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Nov. 30, 2016, 12:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54205/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2016, 12:41 p.m.)
>
>
> Review request for mesos, Joseph Wu and Till Toenshoff.
>
>
> Bugs: MESOS-4695
> https://issues.apache.org/jira/browse/MESOS-4695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test was checking two floating point values for equality by only
> examining their integer part. Since one of the values went through
> a number of conversions, even its integer part could have changed for
> certain starting values.
>
> This patch introduces an epsilon into the comparison to account for
> possible changes in the integer part.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845
>
> Diff: https://reviews.apache.org/r/54205/diff/
>
>
> Testing
> -------
>
> `make check` passes.
>
> I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 8, 2016, 2:35 p.m., Till Toenshoff wrote:
> > src/tests/slave_tests.cpp, line 1491
> > <https://reviews.apache.org/r/54205/diff/1/?file=1573012#file1573012line1491>
> >
> > Would it make sense to not cast into an int at all here, now that we got the epsilon covered?
Oh it definitely would.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/#review158520
-----------------------------------------------------------
On Dec. 8, 2016, 2:57 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54205/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 2:57 p.m.)
>
>
> Review request for mesos, Joseph Wu and Till Toenshoff.
>
>
> Bugs: MESOS-4695
> https://issues.apache.org/jira/browse/MESOS-4695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test was checking two floating point values for equality by only
> examining their integer part. Since one of the values went through
> a number of conversions, even its integer part could have changed for
> certain starting values.
>
> This patch introduces an epsilon into the comparison to account for
> possible changes in the integer part.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 967c7307b6fc8e9eb4490f51938bdcf286b5e7a6
>
> Diff: https://reviews.apache.org/r/54205/diff/
>
>
> Testing
> -------
>
> `make check` passes.
>
> I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 54205: Improved equality check in
SlaveTest.StateEndpoint.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54205/#review158520
-----------------------------------------------------------
src/tests/slave_tests.cpp (line 1491)
<https://reviews.apache.org/r/54205/#comment229284>
Would it make sense to not cast into an int at all here, now that we got the epsilon covered?
- Till Toenshoff
On Nov. 30, 2016, 12:41 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54205/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2016, 12:41 p.m.)
>
>
> Review request for mesos, Joseph Wu and Till Toenshoff.
>
>
> Bugs: MESOS-4695
> https://issues.apache.org/jira/browse/MESOS-4695
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This test was checking two floating point values for equality by only
> examining their integer part. Since one of the values went through
> a number of conversions, even its integer part could have changed for
> certain starting values.
>
> This patch introduces an epsilon into the comparison to account for
> possible changes in the integer part.
>
>
> Diffs
> -----
>
> src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845
>
> Diff: https://reviews.apache.org/r/54205/diff/
>
>
> Testing
> -------
>
> `make check` passes.
>
> I was not able to make this test fail even without the fix. After the fix it continued to work for me, even for a large number of test iterations.
>
>
> Thanks,
>
> Benjamin Bannier
>
>