code review 9926043: testing: fix rounding error in roundDown10

testing: fix rounding error in roundDown10

Fixes issue 5599.

Thanks to minux.ma for the suggested fix.

As we now have a harness to test testing internal functions I added some coverage for testing.roundUp, as it is the main consumer of roundDown10.

[dave_cheney.net, 2013-05-31 23:37:59]

Hello minux.ma@gmail.com, kr@xph.us (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2013-06-01 01:58:02]

Hello minux.ma@gmail.com, kr@xph.us (cc: golang-dev@googlegroups.com),

Please take another look.

[minux1, 2013-06-01 02:13:48]

LGTM.

[kr, 2013-06-01 02:17:31]

Should roundUp and roundDown10 return n if n
is already of the desired form? Doesn't seem
important one way or the other, but they should
at least be consistent.

https://codereview.appspot.com/9926043/diff/7001/src/pkg/testing/benchmark_te...
File src/pkg/testing/benchmark_test.go (right):

https://codereview.appspot.com/9926043/diff/7001/src/pkg/testing/benchmark_te...
src/pkg/testing/benchmark_test.go:18: {100, 10},
Here n is a power of 10.
Doesn't return n.

https://codereview.appspot.com/9926043/diff/7001/src/pkg/testing/benchmark_te...
src/pkg/testing/benchmark_test.go:39: {1000, 1000},
Here n is of the form [1eX, 2eX, 5eX].
Returns n.

https://codereview.appspot.com/9926043/diff/7001/src/pkg/testing/benchmark_te...
src/pkg/testing/benchmark_test.go:43: {5000, 10000},
Here n is of the form [1eX, 2eX, 5eX].
Doesn't return n.

[minux1, 2013-06-01 02:23:46]

On Sat, Jun 1, 2013 at 10:17 AM, <kr@xph.us> wrote:

> Should roundUp and roundDown10 return n if n
> is already of the desired form? Doesn't seem
> important one way or the other, but they should
> at least be consistent.
>
if we decide to do this, we should fix the docs (the docs is not clear
about this corner case
in roundDown10 (i'm assuming that the "down" there means the output should
always < input).

[r, 2013-06-01 03:07:55]

No, "round down" means "round to the nearest integer, downwards if
necessary."  If you round down 2.1, you get 2; if you round down 2,
you still get 2 because it's already rounded down.

-rob

[dave_cheney.net, 2013-06-01 04:35:32]

On 2013/06/01 03:07:55, r wrote:
> No, "round down" means "round to the nearest integer, downwards if
> necessary."  If you round down 2.1, you get 2; if you round down 2,
> you still get 2 because it's already rounded down.
> 
> -rob

PTAL. Thanks to r and minux for clarification. There was a mistake in my
original test data. 

I've reverted to khr's fix in 9915045 (with permission).

Please observe the latest change to the roundUp test.

[kr, 2013-06-01 06:12:54]

Analogously to "round down", "round up" must mean
"round to the nearest integer, upwards if necessary".

So, the tests for roundUp should reflect that.

https://codereview.appspot.com/9926043/diff/14001/src/pkg/testing/benchmark_t...
File src/pkg/testing/benchmark_test.go (right):

https://codereview.appspot.com/9926043/diff/14001/src/pkg/testing/benchmark_t...
src/pkg/testing/benchmark_test.go:38: {1, 2},
Should be {1, 1}

https://codereview.appspot.com/9926043/diff/14001/src/pkg/testing/benchmark_t...
src/pkg/testing/benchmark_test.go:41: {1000, 2000},
Should be {1000, 1000}

https://codereview.appspot.com/9926043/diff/14001/src/pkg/testing/benchmark_t...
src/pkg/testing/benchmark_test.go:45: {5000, 10000},
Likewise, {5000, 5000}

[dave_cheney.net, 2013-06-01 06:29:16]

PTAL. Code and test adjusted to match the specification in the comment.

[kr, 2013-06-01 06:32:50]

LGTM

[r, 2013-06-01 13:43:53]

LGTM

[dave_cheney.net, 2013-06-01 23:13:33]

*** Submitted as https://code.google.com/p/go/source/detail?r=a115231d557b ***

testing: fix rounding error in roundDown10

Fixes issue 5599.

Thanks to minux.ma for the suggested fix.

As we now have a harness to test testing internal functions I added some
coverage for testing.roundUp, as it is the main consumer of roundDown10.

R=minux.ma, kr, r
CC=golang-dev
https://codereview.appspot.com/9926043