Fix charRange to allow picking the last item #36
Conversation
|
@lavalamp Could you please rerun the tests on the second commit for Go 1.13? |
fuzz_test.go
Outdated
| @@ -504,3 +505,37 @@ func TestFuzz_SkipPattern(t *testing.T) { | |||
| return 5, true | |||
| }) | |||
| } | |||
|
|
|||
| func Test_charRange_choose(t *testing.T) { | |||
| // We run 10,000 iterations of charRange.choose and verify 'a', 'z' (edge cases) and 'm' | |||
There was a problem hiding this comment.
Suggestion: make a fake rand, e.g.:
type alwaysLast struct {
*rand.Rand
}
func (a alwaysLast) Int63n(n int64) int64 {
return n-1
}
(you can imagine various extensions to this to test other edge cases)
Then you can test the edge cases instead of trying a bunch of times.
fuzz.go
Outdated
| @@ -478,7 +478,7 @@ type charRange struct { | |||
| // choose returns a random unicode character from the given range, using the | |||
| // given randomness source. | |||
| func (r *charRange) choose(rand *rand.Rand) rune { | |||
There was a problem hiding this comment.
I know it's unrelated, but can you change this to r *rand.Rand? Hiding the package name with the variable name is confusing and gave me a minor panic attack just now because this code should not be making calls to the built in rand package loose functions.
|
Thanks for the fix, I have a suggestion! |
punkeel
force-pushed
the
charrange-pick-last-item
branch
from
47e23f3
to
df5e810
Compare
|
Thanks for the suggestions! I could not find a way to pass a mocked *rand.Rand, so I've introduced a new interface |
punkeel
force-pushed
the
charrange-pick-last-item
branch
from
df5e810
to
5abc911
Compare
|
Bonus: added a constant seed to all tests, to (hopefully) help stabilize them. |
fuzz_test.go
Outdated
| type alwaysFirst struct { | ||
| } | ||
|
|
||
| func (a alwaysFirst) Int63n(n int64) int64 { |
There was a problem hiding this comment.
I don't super object to making the interface like you did, but go's embedding feature makes it pretty easy to produce a *rand.Rand with only a single method overridden like this:
type customInt63 struct {
*rand.Rand
mode int63mode
}
func (c customInt63) Int63n(n int64) int64 {
switch c.mode {
case modeFirst: return 0
case modeLast: return n-1
default: return c.Rand.Int63n(n)
}
}
There was a problem hiding this comment.
I couldn't get the type x struct { *rand.Rand } code to work, hence why I introduced the interface. I'll try harder
There was a problem hiding this comment.
Oh wait-- rand.Rand isn't an interface. You're right, that's not going to work.
There was a problem hiding this comment.
Yeah let's stick with the interface you made and just split the seeding of the tests into another PR and we can merge this.
There was a problem hiding this comment.
Switched to use a custom object with a "mode" instead of two objects.
fuzz_test.go
Outdated
| @@ -42,8 +43,10 @@ func TestFuzz_basic(t *testing.T) { | |||
| }{} | |||
|
|
|||
| failed := map[string]int{} | |||
| f := New(). | |||
| RandSource(rand.NewSource(10)) | |||
There was a problem hiding this comment.
I appreciate the desire to make the tests deterministic, but I'm torn-- I want them to pass with all seeds (and I think they don't right now), I'm worried that sticking to a seed that makes them pass might obscure a legitimate problem. Can you separate these changes into a separate PR so we can get your original fix in sooner? :)
There was a problem hiding this comment.
Sure can - I included it in this PR as the tests were "randomly" failing
Change charRange#choose to accept an interface, int63nPicker, exposing only Int63n(int64) int64. This allows the test to pass a custom RNG, returning a constant value (0 or the last possible int). Change charRange#choose to act on an immutable copy of charRange.
Before this change, choose would never pick the last character of the set. This is not the expected behavior: if the range is a-z, we expect a,b,...,y,z to be picked in a uniform way.
punkeel
force-pushed
the
charrange-pick-last-item
branch
from
5abc911
to
ca99ddd
Compare
|
LGTM, rerunning the test |
Before this change, choose would never pick the last character of the set. This is not the expected behavior: if the range is a-z, we expect a,b,...,y,z to be picked in a uniform way.