code review 967041: big: Add some tests

big: Add some tests

[eds, 2010-04-21 17:45:26]

Hello rsc, gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[gri, 2010-04-21 18:18:15]

http://codereview.appspot.com/967041/diff/2001/3001
File src/pkg/big/int_test.go (right):

http://codereview.appspot.com/967041/diff/2001/3001#newcode153
src/pkg/big/int_test.go:153: fromStringTest{"-0x10", 0, -16, true},
This list should have some tests for octals.

http://codereview.appspot.com/967041/diff/2001/3001#newcode175
src/pkg/big/int_test.go:175: func TestSetString(t *testing.T) {
I don't understand this test. It seems to test less than TestSetNewString.

http://codereview.appspot.com/967041/diff/2001/3002
File src/pkg/big/nat_test.go (right):

http://codereview.appspot.com/967041/diff/2001/3002#newcode15
src/pkg/big/nat_test.go:15: cmpTest{[]Word{0}, []Word{0}, 0},
please add cases for []Word{} and nil (all 4 combinations) - they should all act
as 0

http://codereview.appspot.com/967041/diff/2001/3002#newcode22
src/pkg/big/nat_test.go:22: cmpTest{[]Word{0, _M}, []Word{1, _M}, -1},
Please add one or two longer cases, perhaps with some non 0/1/_M numbers; for
completeness.

[eds, 2010-04-21 18:27:24]

http://codereview.appspot.com/967041/diff/2001/3001
File src/pkg/big/int_test.go (right):

http://codereview.appspot.com/967041/diff/2001/3001#newcode175
src/pkg/big/int_test.go:175: func TestSetString(t *testing.T) {
On 2010/04/21 18:18:15, gri wrote:
> I don't understand this test. It seems to test less than TestSetNewString.

The intention is to test SetString when reusing the same Int. This is the case
that broke the crypto/rsa tests last night. Maybe it would be better to keep the
other function as TestSetString and give this function a different name, like
TestSetStringReuse.

[gri, 2010-04-21 18:49:23]

On Wed, Apr 21, 2010 at 11:27 AM, <chickencha@gmail.com> wrote:

>
> http://codereview.appspot.com/967041/diff/2001/3001
> File src/pkg/big/int_test.go (right):
>
> http://codereview.appspot.com/967041/diff/2001/3001#newcode175
> src/pkg/big/int_test.go:175: func TestSetString(t *testing.T) {
> On 2010/04/21 18:18:15, gri wrote:
>
>> I don't understand this test. It seems to test less than
>>
> TestSetNewString.
>
> The intention is to test SetString when reusing the same Int. This is
> the case that broke the crypto/rsa tests last night. Maybe it would be
> better to keep the other function as TestSetString and give this
> function a different name, like TestSetStringReuse.


Alternatively, how about combining it with the existing test? Just do both
things, use a new Int and reuse an old Int - and perhaps factor out all the
compares. That way it's obvious that the test is really doing the same thing
just with two different Ints as starting point.

>
>
> http://codereview.appspot.com/967041/show
>

[eds, 2010-04-21 23:18:44]

Hello rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[eds, 2010-04-21 23:24:00]

Hello rsc, gri (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2010-04-22 00:12:05]

LGTM

On Wed, Apr 21, 2010 at 4:24 PM, <chickencha@gmail.com> wrote:

> Hello rsc, gri (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/967041/show
>

[gri, 2010-04-22 00:12:39]

*** Submitted as http://code.google.com/p/go/source/detail?r=7da8cab9ebee ***

big: Add some tests

R=rsc, gri
CC=golang-dev
http://codereview.appspot.com/967041

Committer: Robert Griesemer <gri@golang.org>