Skip to content

Commit bd6799b

Browse files
madelynnbluemadelynnblue
committed
util/decimal: decrease allowed shift length
Improve decimal testing to include a 5s timeout. Also use subtests for better test messages. Test timeouts need to be much longer during race tests. Fixes #10928
1 parent d35e838 commit bd6799b

File tree

4 files changed

+172
-86
lines changed

4 files changed

+172
-86
lines changed

‎pkg/util/decimal/decimal.go‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -333,8 +333,15 @@ func Log(z *inf.Dec, x *inf.Dec, s inf.Scale) (*inf.Dec, error) {
333333

334334
// maxExp10 is the maximum number of digits exp10 is expected to return. Above
335335
// this it becomes very slow. However since the implementation for that lies
336-
// in the inf package, we instead check the length in this function.
337-
const maxExp10 = 500000
336+
// in the inf package, we instead check the length in this function. The
337+
// tests in TestDecimalLogN using strE (a very high-precision E value)
338+
// need just under 350000 to complete. They, however, could easily add
339+
// more precision because, although the shift value is high, it converges
340+
// quickly. If we need to improve this heuristic in the future (which is
341+
// designed to prevent slowness only), perhays maxExp10 should decrease as
342+
// loop.i increases. This would allow fast-converging calculations to have high
343+
// precision and prevent slow-converging calculations from taking all the CPU.
344+
const maxExp10 = 350000
338345

339346
// Loop over the Maclaurin series given above until convergence.
340347
for loop := newLoop("log", x, s, 40); ; {

‎pkg/util/decimal/decimal_test.go‎

Lines changed: 121 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,16 @@
1717
package decimal
1818

1919
import (
20+
"fmt"
2021
"math"
2122
"testing"
23+
"time"
2224

2325
"gopkg.in/inf.v0"
2426

2527
_ "github.com/cockroachdb/cockroach/pkg/util/log" // for flags
2628
"github.com/cockroachdb/cockroach/pkg/util/randutil"
29+
"github.com/cockroachdb/cockroach/pkg/util/timeutil"
2730
)
2831

2932
var floatDecimalEqualities = map[float64]*inf.Dec{
@@ -80,36 +83,51 @@ func testDecimalSingleArgFunc(
8083
s inf.Scale,
8184
tests []decimalOneArgTestCase,
8285
) {
83-
for i, tc := range tests {
84-
x, exp := new(inf.Dec), new(inf.Dec)
85-
x.SetString(tc.input)
86-
exp.SetString(tc.expected)
87-
88-
// Test allocated return value.
89-
z, err := f(nil, x, s)
90-
if err != nil {
91-
if tc.expected != err.Error() {
92-
t.Errorf("%d: expected error %s, got %s", i, tc.expected, err)
86+
for _, tc := range tests {
87+
t.Run(fmt.Sprintf("%s = %s", tc.input, tc.expected), func(t *testing.T) {
88+
x, exp := new(inf.Dec), new(inf.Dec)
89+
x.SetString(tc.input)
90+
exp.SetString(tc.expected)
91+
92+
// Test allocated return value.
93+
var z *inf.Dec
94+
var err error
95+
done := make(chan struct{}, 1)
96+
start := timeutil.Now()
97+
go func() {
98+
z, err = f(nil, x, s)
99+
done <- struct{}{}
100+
}()
101+
select {
102+
case <-done:
103+
t.Logf("execute duration: %s", timeutil.Since(start))
104+
case <-time.After(testFuncTimeout):
105+
t.Fatalf("timedout after %s", testFuncTimeout)
106+
}
107+
if err != nil {
108+
if tc.expected != err.Error() {
109+
t.Errorf("expected error %s, got %s", tc.expected, err)
110+
}
111+
return
112+
}
113+
if exp.Cmp(z) != 0 {
114+
t.Errorf("expected %s, got %s", exp, z)
93115
}
94-
continue
95-
}
96-
if exp.Cmp(z) != 0 {
97-
t.Errorf("%d: expected %s, got %s", i, exp, z)
98-
}
99116

100-
// Test provided decimal mutation.
101-
z.SetString("0.0")
102-
_, _ = f(z, x, s)
103-
if exp.Cmp(z) != 0 {
104-
t.Errorf("%d: expected %s, got %s", i, exp, z)
105-
}
117+
// Test provided decimal mutation.
118+
z.SetString("0.0")
119+
_, _ = f(z, x, s)
120+
if exp.Cmp(z) != 0 {
121+
t.Errorf("expected %s, got %s", exp, z)
122+
}
106123

107-
// Test same arg mutation.
108-
_, _ = f(x, x, s)
109-
if exp.Cmp(x) != 0 {
110-
t.Errorf("%d: expected %s, got %s", i, exp, x)
111-
}
112-
x.SetString(tc.input)
124+
// Test same arg mutation.
125+
_, _ = f(x, x, s)
126+
if exp.Cmp(x) != 0 {
127+
t.Errorf("expected %s, got %s", exp, x)
128+
}
129+
x.SetString(tc.input)
130+
})
113131
}
114132
}
115133

@@ -135,71 +153,84 @@ func testDecimalDoubleArgFunc(
135153
s inf.Scale,
136154
tests []decimalTwoArgsTestCase,
137155
) {
138-
for i, tc := range tests {
139-
x, y, exp := new(inf.Dec), new(inf.Dec), new(inf.Dec)
140-
if _, ok := x.SetString(tc.input1); !ok {
141-
t.Errorf("could not set decimal: %s", tc.input1)
142-
continue
143-
}
144-
if _, ok := y.SetString(tc.input2); !ok {
145-
t.Errorf("could not set decimal: %s", tc.input2)
146-
continue
147-
}
148-
149-
// Test allocated return value.
150-
z, err := f(nil, x, y, s)
151-
if err != nil {
152-
if tc.expected != err.Error() {
153-
t.Errorf("%d: expected error %s, got %s", i, tc.expected, err)
156+
for _, tc := range tests {
157+
t.Run(fmt.Sprintf("%s, %s = %s", tc.input1, tc.input2, tc.expected), func(t *testing.T) {
158+
x, y, exp := new(inf.Dec), new(inf.Dec), new(inf.Dec)
159+
if _, ok := x.SetString(tc.input1); !ok {
160+
t.Fatalf("could not set decimal: %s", tc.input1)
154161
}
155-
continue
156-
}
157-
if z == nil {
158-
if tc.expected != "nil" {
159-
t.Errorf("%d: expected %s, got nil", i, tc.expected)
162+
if _, ok := y.SetString(tc.input2); !ok {
163+
t.Fatalf("could not set decimal: %s", tc.input2)
160164
}
161-
continue
162-
} else if tc.expected == "nil" {
163-
t.Errorf("%d: expected nil, got %s", i, z)
164-
continue
165-
}
166-
if _, ok := exp.SetString(tc.expected); !ok {
167-
t.Errorf("%d: could not set decimal: %s", i, tc.expected)
168-
continue
169-
}
170-
if exp.Cmp(z) != 0 {
171-
t.Errorf("%d: expected %s, got %s", i, exp, z)
172-
}
173-
174-
// Test provided decimal mutation.
175-
z.SetString("0.0")
176-
_, _ = f(z, x, y, s)
177-
if exp.Cmp(z) != 0 {
178-
t.Errorf("%d: expected %s, got %s", i, exp, z)
179-
}
180165

181-
// Test first arg mutation.
182-
_, _ = f(x, x, y, s)
183-
if exp.Cmp(x) != 0 {
184-
t.Errorf("%d: expected %s, got %s", i, exp, x)
185-
}
186-
x.SetString(tc.input1)
166+
// Test allocated return value.
167+
var z *inf.Dec
168+
var err error
169+
done := make(chan struct{}, 1)
170+
start := timeutil.Now()
171+
go func() {
172+
z, err = f(nil, x, y, s)
173+
done <- struct{}{}
174+
}()
175+
select {
176+
case <-done:
177+
t.Logf("execute duration: %s", timeutil.Since(start))
178+
case <-time.After(testFuncTimeout):
179+
t.Fatalf("timedout after %s", testFuncTimeout)
180+
}
181+
if err != nil {
182+
if tc.expected != err.Error() {
183+
t.Errorf("expected error %s, got %s", tc.expected, err)
184+
}
185+
return
186+
}
187+
if z == nil {
188+
if tc.expected != "nil" {
189+
t.Errorf("expected %s, got nil", tc.expected)
190+
}
191+
return
192+
} else if tc.expected == "nil" {
193+
t.Errorf("expected nil, got %s", z)
194+
return
195+
}
196+
if _, ok := exp.SetString(tc.expected); !ok {
197+
t.Errorf("could not set decimal: %s", tc.expected)
198+
return
199+
}
200+
if exp.Cmp(z) != 0 {
201+
t.Errorf("expected %s, got %s", exp, z)
202+
}
187203

188-
// Test second arg mutation.
189-
_, _ = f(y, x, y, s)
190-
if exp.Cmp(y) != 0 {
191-
t.Errorf("%d: expected %s, got %s", i, exp, y)
192-
}
193-
y.SetString(tc.input2)
204+
// Test provided decimal mutation.
205+
z.SetString("0.0")
206+
_, _ = f(z, x, y, s)
207+
if exp.Cmp(z) != 0 {
208+
t.Errorf("expected %s, got %s", exp, z)
209+
}
194210

195-
// Test both arg mutation, if possible.
196-
if tc.input1 == tc.input2 {
197-
_, _ = f(x, x, x, s)
211+
// Test first arg mutation.
212+
_, _ = f(x, x, y, s)
198213
if exp.Cmp(x) != 0 {
199-
t.Errorf("%d: expected %s, got %s", i, exp, x)
214+
t.Errorf("expected %s, got %s", exp, x)
200215
}
201216
x.SetString(tc.input1)
202-
}
217+
218+
// Test second arg mutation.
219+
_, _ = f(y, x, y, s)
220+
if exp.Cmp(y) != 0 {
221+
t.Errorf("expected %s, got %s", exp, y)
222+
}
223+
y.SetString(tc.input2)
224+
225+
// Test both arg mutation, if possible.
226+
if tc.input1 == tc.input2 {
227+
_, _ = f(x, x, x, s)
228+
if exp.Cmp(x) != 0 {
229+
t.Errorf("expected %s, got %s", exp, x)
230+
}
231+
x.SetString(tc.input1)
232+
}
233+
})
203234
}
204235
}
205236

@@ -540,6 +571,9 @@ func TestDecimalPow(t *testing.T) {
540571
{"2", "-38", "argument too large"},
541572
{"10000000000", "500", "argument too large"},
542573
{"425644047350.89246", "74.4647211651881", "argument too large"},
574+
{"56051.85009165843", "98.23741371063426", "argument too large"},
575+
{"2306257620454.719", "49.18687811476825", "argument too large"},
576+
{"791018.4038517432", "155.94813858582634", "argument too large"},
543577
}
544578
testDecimalDoubleArgFunc(t, Pow, 16, tests)
545579
}
@@ -564,6 +598,9 @@ func TestDecimalPowDoubleScale(t *testing.T) {
564598
{"2", "-38", "0.000000000004"},
565599
{"10000000000", "500", "argument too large"},
566600
{"425644047350.89246", "74.4647211651881", "argument too large"},
601+
{"56051.85009165843", "98.23741371063426", "argument too large"},
602+
{"2306257620454.719", "49.18687811476825", "argument too large"},
603+
{"791018.4038517432", "155.94813858582634", "argument too large"},
567604
}
568605
testDecimalDoubleArgFunc(t, Pow, 32, tests)
569606
}

‎pkg/util/decimal/no_race_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright 2016 The Cockroach Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
12+
// implied. See the License for the specific language governing
13+
// permissions and limitations under the License.
14+
15+
// +build !race
16+
17+
package decimal
18+
19+
import "time"
20+
21+
const testFuncTimeout = time.Second * 5

‎pkg/util/decimal/race_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// Copyright 2016 The Cockroach Authors.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
12+
// implied. See the License for the specific language governing
13+
// permissions and limitations under the License.
14+
15+
// +build race
16+
17+
package decimal
18+
19+
import "time"
20+
21+
const testFuncTimeout = time.Minute

0 commit comments

Comments
 (0)